Make language slug available via session metadata #1630

minhaminha · 2024-10-08T10:19:50Z

Product Description

Users will now be able to reference the current language slug (i.e. 'en', 'es', etc.) on the screen by referencing instance('commcaresession')/session/context/applanguage

Technical Summary

Jira Ticket

Associated commcare-core PR

This is the formplayer side of the changes. All this code does is ensures that EntityScreenContext has the right locale (a string) value provided on initialization and a null value for any instance that's not directly related to the workflow kicked off by a navigateSessionWithAuth request. Because this process continually recreates the context object, its the ideal entry point to insert this value which will be propagated to the eventual generateRoot function (actual code surrounding this part and further explanation will be in the commcare-core PR).

To be complete this PR also covers an instance of ExternalDataInstance initialization that will now optionally accept a locale argument.

Safety Assurance

Safety story

This shouldn't impact existing data since the EntityScreenContext object is constantly being recreated with updated information and most of the time the locale value will be null.

QA Plan

No plans for QA but will have delivery (who request this feature) do some testing to ensure it's doing what they want.

Special deploy instructions

This PR can be deployed after merge with no further considerations. (Of course the commcare-core PR will be merged first and this PR updated with those changes before merging/deploying).

Rollback instructions

This PR can be reverted after deploy with no further considerations. (Ideally should be rolled back with attached commcare-core PR?)

Review

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

codecov · 2024-10-16T21:47:28Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.22%. Comparing base (c069207) to head (6b0ad02).
Report is 27 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1630      +/-   ##
============================================
+ Coverage     70.16%   70.22%   +0.05%     
- Complexity     1999     2003       +4     
============================================
  Files           254      254              
  Lines          7881     7899      +18     
  Branches        741      742       +1     
============================================
+ Hits           5530     5547      +17     
- Misses         2069     2070       +1     
  Partials        282      282

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

Robert-Costello · 2024-10-17T16:26:44Z

src/main/java/org/commcare/formplayer/session/FormplayerInstanceInitializer.java

+        String appLang = locale;
+        if (locale == null) {
+            String[] locales = Localization.getGlobalLocalizerAdvanced().getAvailableLocales();
+            appLang =  locales[locales.length-1];


this grabs the last locale created? is that always right one?

thanks so much for pointing this out, totally forgot to go back to this. The locales list is always as follows: ['default' (like literally just a string called default), , <all other slugs in order the were added]. The locales.length-1 was previously a placeholder and wouldn't have worked if there were more than one languages. Added a similar comment in the code too to avoid confusion.

shubham1g5

This will need test coverage before the merge, you can look at the tests added in #1596 for an example.

shubham1g5 · 2024-10-21T16:19:22Z

src/main/java/org/commcare/formplayer/application/MenuController.java

@@ -165,6 +167,7 @@ public EntityDetailListResponse getDetails(@RequestBody SessionNavigationBean se
    public BaseResponseBean navigateSessionWithAuth(@RequestBody SessionNavigationBean sessionNavigationBean,
            @CookieValue(Constants.POSTGRES_DJANGO_SESSION_ID) String authToken,
            HttpServletRequest request) throws Exception {
+        String locale = sessionNavigationBean.getLocale();


How does HQ get to know of current locale to send here ?

Honestly not too sure! I just assumed all navigation request included the locale in the request body which gets set when initializing the sessionNavigationBean. Happy to be told otherwise but it's been accurately giving me the correct locale so far.

did you mean HQ was already sending the current locale in the request body without us having to make a corresponding HQ change here ?

yes I believe so!

shubham1g5 · 2024-10-21T16:22:18Z

src/main/java/org/commcare/formplayer/session/FormplayerInstanceInitializer.java

+        if (appLang == null || !Arrays.asList(locales).contains(appLang)) {
+            // the ordering is always ['default', <true default slug>, <all other slugs in order they were added>]
+            if (locales.length >= 2) {
+                appLang =  locales[1];
+            } else {
+                // to pass tests
+                appLang = locales[0];
+            }
+        }


My read of the code here is you are trying to get the current selected locale ? If so, think we can use Localization.getCurrentLocale() directly.

I wish I had known about this function sooner because this is doing exactly what I was looking to do from the start, without having to do all the unnecessary steps of passing the locale to the root generator function... I might end up reverting the commcare-core PR :(

partial revert PR

…the simpler Localization.getCurrentLocale()

shubham1g5 · 2024-10-23T09:59:17Z

src/main/java/org/commcare/formplayer/beans/AuthenticatedRequestBean.java

@@ -22,6 +22,7 @@ public class AuthenticatedRequestBean {
    private int timezoneOffsetMillis = -1;
    private String timezoneFromBrowser = null;
    private String windowWidth;
+    private String locale;


This doesn't seem like it's getting used anymore, think we can remove all bean model changes with the updated implementation.

this is solely so that the test can pass!

nevermind! this was removed

shubham1g5

I don't think the tests are set up correctly. Since we are no longer dependent on locale being passed from the request body or request bean , locale should no longer be assigned to the bean for testing. Also we should add additional testing that incorporates changing of locale in between a test and check whether the xpath expression returns new changed locale correctly.

Also double confirming that we have tested the new implementation on an app somewhere ?

shubham1g5 · 2024-10-23T18:27:15Z

src/test/java/org/commcare/formplayer/tests/BaseTestClass.java

@@ -828,6 +828,7 @@ <T> T sessionNavigateWithSelectedValues(String[] selections, String testName, St
        sessionNavigationBean.setSelections(selections);
        sessionNavigationBean.setSelectedValues(selectedValues);
        sessionNavigationBean.setWindowWidth(windowWidth);
+        sessionNavigationBean.setLocale(locale);


How is this working given sessionNavigationBean does no longer have setLocale method ?

minhaminha · 2024-10-23T18:58:12Z

After some discussion, because delivery wants this feature to be usable ASAP, Shubham and I agreed to add (proper) test coverage for this change in a separate PR. I'll link it back to this one once it's up.

Make language slug available via session metadata

21d0582

minhaminha requested review from shubham1g5, AddisonDunn, Robert-Costello and Jtang-1 October 8, 2024 10:19

minhaminha mentioned this pull request Oct 10, 2024

Make language slug available via session metadata (formplayer branch) dimagi/commcare-core#1441

Merged

3 tasks

minhaminha added 2 commits October 16, 2024 17:38

Update references

c52535c

Update references (actually related to my changes)

848b727

Robert-Costello reviewed Oct 17, 2024

View reviewed changes

minhaminha added 2 commits October 18, 2024 15:01

Make sure default slug is always correct + remove unnecessary null arg

fdac882

Fix tests

3bd8611

Robert-Costello approved these changes Oct 18, 2024

View reviewed changes

shubham1g5 requested changes Oct 21, 2024

View reviewed changes

minhaminha added 2 commits October 22, 2024 20:24

Partially reverse previous attempts to find locale in favor of using …

d6be27e

…the simpler Localization.getCurrentLocale()

Add test

82681c3

shubham1g5 mentioned this pull request Oct 23, 2024

Partially reverse "Make language slug available via session metadata" dimagi/commcare-core#1447

Merged

3 tasks

shubham1g5 requested changes Oct 23, 2024

View reviewed changes

minhaminha added 2 commits October 23, 2024 09:29

Update references

abc2cfa

Remove unncessary locale use for test

37eab50

Robert-Costello approved these changes Oct 23, 2024

View reviewed changes

shubham1g5 requested changes Oct 23, 2024

View reviewed changes

Remove test temporarily

6b0ad02

shubham1g5 approved these changes Oct 23, 2024

View reviewed changes

minhaminha merged commit 6072202 into master Oct 23, 2024
6 checks passed

minhaminha deleted the ml/lang-slug-metadata branch October 23, 2024 18:59

minhaminha mentioned this pull request Nov 1, 2024

Add test for language slug in metadata feature #1637

Draft

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make language slug available via session metadata #1630

Make language slug available via session metadata #1630

minhaminha commented Oct 8, 2024 •

edited

Loading

codecov bot commented Oct 16, 2024 •

edited

Loading

Robert-Costello Oct 17, 2024

minhaminha Oct 18, 2024

shubham1g5 left a comment

shubham1g5 Oct 21, 2024

minhaminha Oct 22, 2024

shubham1g5 Oct 22, 2024

minhaminha Oct 22, 2024

shubham1g5 Oct 21, 2024

minhaminha Oct 22, 2024

minhaminha Oct 23, 2024

shubham1g5 Oct 23, 2024

minhaminha Oct 23, 2024

minhaminha Oct 23, 2024

shubham1g5 left a comment

shubham1g5 Oct 23, 2024

minhaminha commented Oct 23, 2024 •

edited

Loading

Make language slug available via session metadata #1630

Make language slug available via session metadata #1630

Conversation

minhaminha commented Oct 8, 2024 • edited Loading

Product Description

Technical Summary

Safety Assurance

Safety story

QA Plan

Special deploy instructions

Rollback instructions

Review

codecov bot commented Oct 16, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubham1g5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubham1g5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minhaminha commented Oct 23, 2024 • edited Loading

minhaminha commented Oct 8, 2024 •

edited

Loading

codecov bot commented Oct 16, 2024 •

edited

Loading

minhaminha commented Oct 23, 2024 •

edited

Loading