End-of-form navigation manual linking advanced mode #33950

esoergel · 2024-01-05T14:49:47Z

Product Description

Adds an "advanced mode" feature flag for end of form navigation manual linking.

Without the feature flag:

With the feature flag:

Technical Summary

https://dimagi-dev.atlassian.net/browse/USH-3966
This PR incorporates work @MartinRiese did a bit ago to include a broader set of modules in the set of linkable targets for EoF nav.

EoF navigation involves populating the stack to navigate through the app to the desired target. Some forms and modules expect certain variables (like case IDs) to be available in the session. Automatic linking attempts to align existing session variables with those needed to navigate to the target. Where that isn't possible, the app builder must provide an xpath expression for each of these datum IDs. This manual linking is also sometimes used when automatic linking doesn't do what's desired.

However, we can't always reliably get the set of datum IDs needed to get to a target without going through the full build process, especially when child and shadow modules are involved, as the datum IDs are modified at various stages in the build process.

This PR introduces an "advanced mode" feature flag for manual linking that essentially gives up trying to predict the IDs needed, and requires the user to specify all sets of IDs and xpath expressions needed to navigate to the endpoint. This will likely be a pain to use, though build validation should catch errors here.

Depending on how this is used, we may consider an extension of this work where advanced mode is used only on a case-by-case basis, so app builders could decide whether to enable it per module, and otherwise use the existing manual linking UI (we'd need to ensure switching between the two doesn't break anything).

Feature Flag

This adds a new feature flag:
FORM_LINK_ADVANCED_MODE - USH: Form linking advanced mode

Safety Assurance

Safety story

Testing on staging. The UI itself is pretty small, it uses an existing widget for specifying key/value pairs. Everything should be sufficiently hidden behind the feature flag that I'm not too concerned about affecting projects that don't enable the FF.

Automated test coverage

none

QA Plan

none

Rollback instructions

This PR can be reverted after deploy with no further considerations

Labels & Review

Risk label is set correctly
The set of people pinged as reviewers is appropriate for the level of risk of the change

Shouldn't we also require selecting the datum for the parent module? Not sure how that works - to follow up on.

MartinRiese · 2024-01-08T15:12:30Z

corehq/toggles/__init__.py

@@ -742,6 +742,18 @@ def _ensure_valid_randomness(randomness):
    [NAMESPACE_DOMAIN],
 )

+FORM_LINK_ADVANCED_MODE = StaticToggle(


will this be a temporary FF? We really are pretty quick to add new ones aren't we?

I don't see an alternative, unfortunately (though I'm open to ideas!). This feature is generally available for some reason, even though it's pretty brittle. I was thinking about trying to integrate the advanced mode into the normal manual mode as mentioned in the description, and may do that if delivery advocates for it. However, I don't think it's a good idea to increase the set of linkable targets to GA audiences, knowing that they don't work unless you are able to figure out the "advanced mode" linking stuff.

That makes sense. Curious if you would see any value in have a parent FF like "experimental" to group these kinds of FF and give them a bit more context?

You mean like a separate feature flag tag? I don't know, would something in the description make sense? This seems fairly in line with other custom feature flags to me - what's the information you're thinking about conveying?

MartinRiese · 2024-01-08T15:14:15Z

corehq/apps/app_manager/static/app_manager/js/forms/form_workflow.js

@@ -143,6 +144,7 @@ hqDefine('app_manager/js/forms/form_workflow', function () {
        self.formId = ko.observable(formId);
        self.autoLink = ko.observable();
        self.allowManualLinking = ko.observable();
+        self.advancedMode = hqImport("hqwebapp/js/toggles").toggleEnabled('FORM_LINK_ADVANCED_MODE');


maybe advancedLinkingMode would make it clearer what this does.

MartinRiese · 2024-01-08T15:15:46Z

corehq/apps/app_manager/static/app_manager/js/forms/form_workflow.js

+                gettext("Set datums required to navigate to the selected form or menu"),
+                {key: gettext("Datum ID"), value: gettext("XPath Expression")}  // placeholders
+            );
+            let datumsDict = {};


I think this can be a const. I don't see you reassigning it.

I was thinking of it as something that would be mutated (ie, not a constant), but reading over this page, I think I'm coming to agree with you.

You should understand const declarations as "create a variable whose identity remains constant", not "whose value remains constant"

Many style guides (including MDN's) recommend using const over let whenever a variable is not reassigned in its scope. This makes the intent clear that a variable's type (or value, in the case of a primitive) can never change. Others may prefer let for non-primitives that are mutated.

Javascript is frustrating sometimes.

MartinRiese · 2024-01-08T15:21:26Z

corehq/apps/app_manager/static/app_manager/js/forms/form_workflow.js

+            });
+            self.advancedModeDatumsElement.val(datumsDict);
+            self.advancedModeDatumsElement.on("change", function () {
+                self.serializedDatums(JSON.stringify(_.map(this.val(), function (xpath, name) {


Just making sure I understand this correctly. If we are not in advanced mode self.serializedDatums will be used by the formId element. But you can only be in "normal" or "advanced". So there is no conflict? Is that correct?

That's right - serializedDatums will be controlled by one or the other. I do wish this had further separation though, like some sort of composable element that treats the variable stuff as a widget where you can swap between one or the other. Though I think if I put much more work into this UI I'd probably drop this widget entirely and work on combining the two UIs (maybe, I don't know - I don't trust the old UI, and that might be worse, for instance if the displayed case type is wrong).

I feel like having a follow up story/pr for cleaning up the config ui would be reasonable

gherceg

@nospame and I reviewed this. It looks good so far and left a couple of clarifying comments.

gherceg · 2024-01-09T19:41:02Z

corehq/apps/app_manager/views/forms.py

+        if '.' in form_id:
+            module_id, form_id = form_id.split('.')


Naive question, but is it a fair assumption that the form_id will always be in the format "module.form" (or "module") and not "parent.child.form_id"?

that's correct - it's set here:

commcare-hq/corehq/apps/app_manager/views/forms.py

Line 953 in f51b264

'unique_id': f'{candidate_module.unique_id}.{candidate_form.unique_id}',

(and a little above that for modules, which don't contain the .)

gherceg · 2024-01-09T20:26:45Z

corehq/apps/app_manager/views/forms.py

@@ -981,7 +981,7 @@ def _module_hierarchy(module):
 def get_form_datums(request, domain, app_id):
    form_id = request.GET.get('form_id')
    try:
-        datums = _get_form_datums(domain, app_id, form_id)
+        datums = _get_form_link_datums(domain, app_id, form_id)


If get_form_datums also just returns "form_link_datums", does it make sense to rename that as well? This is coming from a place of not understanding the details of _get_form_link_datums and why we only return datums where requires_selection is true as someone reading through this code for the first time. Maybe it could be useful to document that somewhere?

yeah I think it makes sense to rename the view (and URL and what not) too. Or at least keep them consistent. I'll do that.

Though I think I might actually walk back the changes to this function since it doesn't apply to advanced mode datums and modules aren't linkable aside from in that mode that. This work has seen a decent bit of churn as we try to get something working (it's still being tested on staging right now, actually, though I think the current issues are on the formplayer side of things). An earlier version intended to extend the manual linking to modules, but it just didn't work in practice (we know that the existing version of this feature has edge cases that fail).

why we only return datums where requires_selection is true

This is documented a bit here:

commcare-hq/corehq/apps/app_manager/models.py

Lines 780 to 794 in f51b264

class FormLink(DocumentSchema):

"""

FormLinks are advanced end of form navigation configuration, used when a module's

post_form_workflow is WORKFLOW_FORM.

They allow the user to specify one or more XPath expressions, each with either

a module id or form id. The user will be sent to the first module/form whose

expression evaluates to true. If none of the conditions is met, the workflow specified

in the module's post_form_workflow_fallback is executed.

xpath: XPath condition that must be true in order to execute link

form_id: ID of next form to open, mutually exclusive with module_unique_id

form_module_id: ID of the form's module (this is used for shadow modules)

module_unique_id: ID of next module to open, mutually exclusive with form_id

datums: Any user-provided datums, necessary when HQ can't figure them out automatically

And in a broader context on readthedocs.

The gist of it is that to navigate to a form or menu, you have to provide datums that "require selection", like choosing a case from a case list. Those that don't require selection don't need to be provided manually, they can be inferred.

esoergel · 2024-01-26T23:00:48Z

Closing in favor of #34037, which squashes and reorganizes the commits so they're easier to review, and removes some unneeded changes from this PR.

MartinRiese and others added 10 commits January 5, 2024 15:53

Allow any child for linking

92f26ad

reference form_link for clarity

24acd2d

Fetch module datums too

858afdd

Assume these new modules are not auto linkable

84e01be

Add test for datum selection for child module

6711125

Shouldn't we also require selecting the datum for the parent module? Not sure how that works - to follow up on.

Group together manual linking HTML

f0bdcb6

Add feature flagged manual linking "advanced mode"

27dc030

Add "advanced mode" editor

ff69ae1

Add test demonstrating that suite validation catches errors here

487564a

Make new form link targets available only via FF

f636daf

esoergel force-pushed the es/riese/linking branch 2 times, most recently from 02c7214 to fcbd8ab Compare January 5, 2024 21:17

esoergel requested a review from MartinRiese January 5, 2024 21:20

esoergel added 2 commits January 5, 2024 16:34

lint

05fcd19

Fix form_workflow test module import stuff

22b9d9b

esoergel force-pushed the es/riese/linking branch from fcbd8ab to 22b9d9b Compare January 5, 2024 21:34

MartinRiese reviewed Jan 8, 2024

View reviewed changes

gherceg reviewed Jan 9, 2024

View reviewed changes

esoergel added 2 commits January 19, 2024 21:53

Make manual datums take precendence

8363801

Include root module in command ID

903456d

esoergel mentioned this pull request Jan 26, 2024

End-of-form navigation manual linking advanced mode #34037

Merged

3 tasks

esoergel closed this Jan 26, 2024

esoergel deleted the es/riese/linking branch January 26, 2024 23:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

End-of-form navigation manual linking advanced mode #33950

End-of-form navigation manual linking advanced mode #33950

esoergel commented Jan 5, 2024 •

edited

Loading

MartinRiese Jan 8, 2024

esoergel Jan 9, 2024

MartinRiese Jan 9, 2024

esoergel Jan 10, 2024

MartinRiese Jan 8, 2024

MartinRiese Jan 8, 2024

esoergel Jan 9, 2024

MartinRiese Jan 8, 2024

esoergel Jan 9, 2024

MartinRiese Jan 9, 2024

gherceg left a comment

gherceg Jan 9, 2024

esoergel Jan 10, 2024 •

edited

Loading

gherceg Jan 9, 2024

esoergel Jan 10, 2024

esoergel commented Jan 26, 2024

	class FormLink(DocumentSchema):
	"""
	FormLinks are advanced end of form navigation configuration, used when a module's
	post_form_workflow is WORKFLOW_FORM.

	They allow the user to specify one or more XPath expressions, each with either
	a module id or form id. The user will be sent to the first module/form whose
	expression evaluates to true. If none of the conditions is met, the workflow specified
	in the module's post_form_workflow_fallback is executed.

	xpath: XPath condition that must be true in order to execute link
	form_id: ID of next form to open, mutually exclusive with module_unique_id
	form_module_id: ID of the form's module (this is used for shadow modules)
	module_unique_id: ID of next module to open, mutually exclusive with form_id
	datums: Any user-provided datums, necessary when HQ can't figure them out automatically

End-of-form navigation manual linking advanced mode #33950

End-of-form navigation manual linking advanced mode #33950

Conversation

esoergel commented Jan 5, 2024 • edited Loading

Product Description

Technical Summary

Feature Flag

Safety Assurance

Safety story

Automated test coverage

QA Plan

Rollback instructions

Labels & Review

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

gherceg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esoergel Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esoergel commented Jan 26, 2024

esoergel commented Jan 5, 2024 •

edited

Loading

esoergel Jan 10, 2024 •

edited

Loading