-
-
Notifications
You must be signed in to change notification settings - Fork 221
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
Resolve redundant nav menu on first submit click without any change #34658
Conversation
42019c9
to
2f97159
Compare
2f97159
to
7e8f6c6
Compare
corehq/apps/cloudcare/static/cloudcare/js/formplayer/menus/views/query.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good once Ethan's comment is addressed.
var self = this; | ||
// Gather error messages | ||
var invalidFields = []; | ||
self._getChildren().forEach(function (childView) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@orangejenny I am running across an issue where self._getChildren()
returns me an empty [] when I run search due to which error display doesn't work correctly. On looking at the code this method populates children using this.children
which returns empty to me. Any idea how can I debug this ? I am quite blocked on figuring out when and where this.children
gets its value from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably know this.children
is populated by Marionette, it's part of this being a CollectionView
. The view lifecycle is event driven, so it can be tricky to debug, but there are callbacks for the various events. Check out the CollectionView docs and the more general View lifecycle docs. (Make sure you're in the right version of docs - a search engine took me to the v2 docs, but we're on v4 and this area has changed a good deal.)
I poked around on this branch, just adding console statements in executeSearch
, performSubmit
, etc. and adding a few handlers to the QueryListView
like onBeforeDestroy
that just contained console statements. Seems the QueryListView
is getting destroyed as part of the menu:query
response (because it calls listMenus
which eventually calls showMenu
and then this code calls show
on the main and sidebar regions, and show
causes the view to get destroyed and a new one created). I believe that's why children
is empty when this code runs. But I'm not immediately sure how to fix it. You might be able to do something like moving displayErrors
into onRender
or onRenderChildren
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that gives me a bunch of context to proceed on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To close the loop, I tried putting displayErrors
into onRender
or onRenderChildren
but it has unintended side-effect of us showing errors the very first time we show the query screen without user doing any interaction. As such I am not sure how to fix it but we can potentially explore 2 ways -
- Use a flag to indentify first load of the screen in the
urlObject
that bypassesdisplayErrors
- Changing code so that we don't call
show
on page if the response type is same as last response type. This might mean us storing last response type inurlObject
or session storage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of having some sort of explicit signal indicating whether there has been a user-initiated submission. Can we get that information from the formplayer response? Is that what you're suggesting with the response type idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get that information from the formplayer response?
Not really, as FP doesn't really know whether a request is a result of user clicking search or something else. It's possible though that Web Apps can pass a flag to FP that FP preserves in the response but would think that Web Apps should be able to tell that independently without needing this flag to be set on FP.
-> Is that what you're suggesting with the response type idea?
No, I was suggesting to not re-render pages on Web Apps if the new response type is same as the last i.e. if we are on a case list screen and FP replies with a response that corresponds to the case list screen, we should not need to re-render the pages/views completely by calling show
on ui elements but should be able to just update the date on those UI elements.
catch up to base
catch up to base
catch up to base branch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional changes look good
Thanks, @MartinRiese. I requested a QA check for regression issues just to be safe. |
@@ -728,23 +728,33 @@ hqDefine("cloudcare/js/formplayer/menus/views/query", [ | |||
submitAction: function (e) { | |||
var self = this; | |||
e.preventDefault(); | |||
self.performSubmit(); | |||
self.performSubmit(formplayerConstants.requestInitiatedByTagsMapping.USER_CLICK_SUBMIT); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm the original/current use case of the initiatedBy
is to add tags to DD (this PR) The use case here isn't really the intended purpose and I guess this is minor but each unique DD tag does add additional cost. Maybe it's better off to make this a new argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call, no need to add tags that no one is asking for.
catch up to base
catch up to base
catch up to base branch
QA is complete on this. pinging for final review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving commits since the last approval
if (userClickSubmit === formplayerConstants.USER_CLICK_SUBMIT) { | ||
self.displayErrors(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since the body of this is only one line, and it's only used once, what would you think about just moving self.displayErrors();
into submitAction
and dropping the userClickSubmit
arg?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed here 8be4ffa
if (!self.inputsHaveErrors()) { | ||
self.executeSearch(initiatedBy).done(function (response) { | ||
self.updateModels(response); | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice refactor
@@ -783,34 +806,16 @@ hqDefine("cloudcare/js/formplayer/menus/views/query", [ | |||
validateAllFields: function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still in use anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not anymore. looks like we can remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed here 75a6181
Product Description
Jira
Technical Summary
Earlier we were doing 2 calls on clicking submit -
validateAllFields
which in turn fires anavigate_menu
request withexecute
set to false onqueryData
execute
trueAlthough we really only need the request in step 2 as FP validates all requests irrespective of value of
execute
flag. This PR therefore removes the first call and handles the case search request with a Marionetterequest
instead of an event to be able to receive a promise back from theexecuteSearch
call.Feature Flag
CASE_CLAIM
Safety Assurance
Safety story
Automated test coverage
Not much, relies on QA automation scripts
QA Plan
QA ticket
Rollback instructions
Labels & Review