Make language slug available via session metadata (formplayer branch) #1441
Conversation
minhaminha
requested review from
snopoke,
MartinRiese,
Robert-Costello and
Jtang-1
| @@ -279,7 +279,7 @@ public void setID(int recordid) { | |||
| this.recordid = recordid; | |||
| } | |||
|
|
|||
| public abstract DataInstance initialize(InstanceInitializationFactory initializer, String instanceId); | |||
| public abstract DataInstance initialize(InstanceInitializationFactory initializer, String instanceId, String locale); | |||
There was a problem hiding this comment.
Could you not create a concrete method with the original signature which calls the abstract one with the locale being null? Then you would not have to pass the null everywhere.
There was a problem hiding this comment.
Very true! This did indeed get rid of some unnecessary null usage 🎉
…es) to avoid putting so many null values into argument lists
|
duplicate this PR |
There was a problem hiding this comment.
Sorry for jumping in late, but think it's critical to be consistent around code patterns we are following for adding different session parameters. Also would appreciate if you can update the doc here to reflect the new parameter. Thanks!
| @@ -9,6 +9,7 @@ public class EntityScreenContext extends ScreenContext{ | |||
| private final int mSortIndex; | |||
| private final int mCasesPerPage; | |||
| private final String[] mSelectedValues; | |||
| private final String mLocale; | |||
There was a problem hiding this comment.
think this should be part of the super class ScreenContext as it is not something specific to entity screens.
| @@ -236,15 +236,15 @@ protected TreeElement loadFixtureRoot(ExternalDataInstance instance, | |||
| } | |||
| } | |||
|
|
|||
| protected InstanceRoot setupSessionData(ExternalDataInstance instance) { | |||
| protected InstanceRoot setupSessionData(ExternalDataInstance instance, String locale) { | |||
There was a problem hiding this comment.
can we follow the same pattern as window_width to pass the locale to this method to avoid unnecessary method signature changes and be consistent in general around how we are passing session parameters.
There was a problem hiding this comment.
Hi Shubham thanks for getting on this so quick after coming back online! I can make these changes but what would the process of PR-ing them be? I guess I need to make a new branch to merge into the formplayer branch but can I just push the extra commit to the auto-opened/still-open PR for merging into the master branch?
There was a problem hiding this comment.
can I just push the extra commit to the #1443 for merging into the master branch
Yeah that sounds fine to me.
There was a problem hiding this comment.
addressed this and the above comment here. I'll make the doc changes once everything is deployed!
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/applanguageTechnical Summary
Associated formplayer PR
This is the commcare-core side of the changes which:
EntityScreenContextclass to be able to carry and get the stored locale value (first initialized from withinnavigateSessionWithAuth), which allows it to be accessed during different parts of the response creation process.generateRootpath such thatlocaleis eventually added to the session root under metadata.setupSessionData, which eventually leads to callingaddMetadata, now also with a required 'locale' arguement.navigateSessionWithAuthrequest to use null values instead).Safety Assurance
Safety story
This shouldn't impact existing data since the objects surrounding session context is constantly being recreated with updated information from formplayer and most of the time the
localevalue will be set to 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
Rollback instructions
Review
Duplicate PR
Automatically duplicate this PR as defined in contributing.md.
(Doing this after this PR gets approved)