Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set session cookie for user's preferred language #1648

Closed
wants to merge 2 commits into from

Conversation

avazirna
Copy link
Contributor

Product Description

This PR sets a session cookie to store the user's preferred language for the current session. This cookie is to be used to override the user's favourite language in subsequent HTTP requests, making sure that the chosen language persists throughout the session.
This change also requires changes to the frontend, so another PR will be linked to this one.

Safety Assurance

Safety story

Automated test coverage

QA Plan

Special deploy instructions

  • This PR can be deployed after merge with no further considerations.

Rollback instructions

  • This PR can be reverted after deploy with no further considerations.

Review

  • The set of people pinged as reviewers is appropriate for the level of risk of the change.

@avazirna avazirna requested a review from shubham1g5 November 25, 2024 13:26
@avazirna avazirna force-pushed the store-session-cookie-for-preferred-language branch from 9e01acc to 870aaf9 Compare November 25, 2024 13:27
Copy link

codecov bot commented Nov 25, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.

Project coverage is 70.20%. Comparing base (bb7cdbb) to head (870aaf9).
Report is 39 commits behind head on master.

Files with missing lines Patch % Lines
...ommcare/formplayer/application/MenuController.java 83.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1648      +/-   ##
============================================
- Coverage     70.22%   70.20%   -0.02%     
+ Complexity     2007     2005       -2     
============================================
  Files           254      254              
  Lines          7902     7905       +3     
  Branches        745      744       -1     
============================================
+ Hits           5549     5550       +1     
  Misses         2070     2070              
- Partials        283      285       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@avazirna avazirna marked this pull request as ready for review November 26, 2024 08:09
@avazirna avazirna changed the title Store session cookie for user's preferred language Set session cookie for user's preferred language Nov 26, 2024
@avazirna avazirna requested a review from snopoke November 26, 2024 09:06
@CookieValue(Constants.POSTGRES_DJANGO_SESSION_ID) String authToken,
HttpServletRequest request) throws Exception {
@CookieValue(Constants.POSTGRES_DJANGO_SESSION_ID) String authToken,
@CookieValue(value = FORMPLAYER_SESSION_LANGUAGE, required = false) String sessionPrefLang,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Constants is already imported:

Suggested change
@CookieValue(value = FORMPLAYER_SESSION_LANGUAGE, required = false) String sessionPrefLang,
@CookieValue(Constants.FORMPLAYER_SESSION_LANGUAGE, required = false) String sessionPrefLang,

Comment on lines +174 to +176
if (sessionPrefLang == null || !sessionPrefLang.equals(sessionNavigationBean.getLocale())) {
response.addCookie(new Cookie(FORMPLAYER_SESSION_LANGUAGE, sessionNavigationBean.getLocale()));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow what's going on here. We're resetting the cookie value if it doesn't match what's being sent in the request body. But the value in the request body is coming from the cookie right (unless the cookie is empty in which case the first part of the condition is met)?

@avazirna
Copy link
Contributor Author

The decision was to use the localStorage instead of cookies, so this is no longer needed.

@avazirna avazirna closed this Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants