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

Resolve redundant nav menu on first submit click without any change #34658

Merged
merged 18 commits into from
Jan 1, 2025

Conversation

shubham1g5
Copy link
Contributor

@shubham1g5 shubham1g5 commented May 22, 2024

Product Description

Jira

Technical Summary

Earlier we were doing 2 calls on clicking submit -

  1. validateAllFields which in turn fires a navigate_menu request with execute set to false on queryData
  2. The actual case search request with execute true

Although 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 Marionette request instead of an event to be able to receive a promise back from the executeSearch call.

Feature Flag

CASE_CLAIM

Safety Assurance

Safety story

  • Tested locally, the change should trigger workflows in basic workflows if incorrect
  • Will put on staging for a bit to have QA automation scripts catch anything expected

Automated test coverage

Not much, relies on QA automation scripts

QA Plan

QA ticket

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

@dimagimon dimagimon added the Risk: Medium Change affects files that have been flagged as medium risk. label May 22, 2024
@shubham1g5 shubham1g5 force-pushed the redundantNavMenuOnSearch branch from 42019c9 to 2f97159 Compare May 23, 2024 06:16
@shubham1g5 shubham1g5 force-pushed the redundantNavMenuOnSearch branch from 2f97159 to 7e8f6c6 Compare May 23, 2024 06:23
@shubham1g5 shubham1g5 added the product/custom Change will only impact users on a single project label May 23, 2024
@shubham1g5 shubham1g5 marked this pull request as ready for review May 23, 2024 06:46
Copy link
Contributor

@orangejenny orangejenny left a 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) {
Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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 -

  1. Use a flag to indentify first load of the screen in the urlObject that bypasses displayErrors
  2. 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 in urlObject or session storage.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

@MartinRiese MartinRiese left a 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

@Robert-Costello Robert-Costello added the awaiting QA QA in progress. Do not merge label Nov 6, 2024
@Robert-Costello
Copy link
Contributor

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);
Copy link
Contributor

@Jtang-1 Jtang-1 Nov 7, 2024

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?

Copy link
Contributor

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.

@Robert-Costello Robert-Costello added QA Passed and removed awaiting QA QA in progress. Do not merge labels Dec 30, 2024
@Robert-Costello
Copy link
Contributor

QA is complete on this. pinging for final review.

Copy link
Contributor

@esoergel esoergel left a 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

Comment on lines 741 to 743
if (userClickSubmit === formplayerConstants.USER_CLICK_SUBMIT) {
self.displayErrors();
}
Copy link
Contributor

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

changed here 8be4ffa

Comment on lines +736 to +740
if (!self.inputsHaveErrors()) {
self.executeSearch(initiatedBy).done(function (response) {
self.updateModels(response);
});
}
Copy link
Contributor

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 () {
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

removed here 75a6181

@Robert-Costello Robert-Costello merged commit 1a215f1 into master Jan 1, 2025
12 checks passed
@Robert-Costello Robert-Costello deleted the redundantNavMenuOnSearch branch January 1, 2025 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/custom Change will only impact users on a single project QA Passed Risk: Medium Change affects files that have been flagged as medium risk.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants