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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
7884a8f
abstract to displayErrors
shubham1g5 May 15, 2024
bc6e2cf
abstract to updateModels
shubham1g5 May 15, 2024
7e8f6c6
convert menu:query to request and have it reply with a promise
shubham1g5 May 22, 2024
bff1adc
correct self reference
shubham1g5 May 27, 2024
1a5d3ab
return promise from selectMenu
shubham1g5 May 29, 2024
b1c40bc
Merge branch 'master' into redundantNavMenuOnSearch
Robert-Costello Nov 1, 2024
65d72b5
add const to represent user clicking submit button
Robert-Costello Nov 1, 2024
d2cf66d
display errors only if user clicks submit
Robert-Costello Nov 1, 2024
f0f42f5
Merge branch 'master' into redundantNavMenuOnSearch
Robert-Costello Nov 1, 2024
fc86704
Merge branch 'master' into redundantNavMenuOnSearch
Robert-Costello Nov 4, 2024
be0ac93
use separate const from datadog tags
Robert-Costello Nov 7, 2024
517530d
Merge branch 'master' into redundantNavMenuOnSearch
Robert-Costello Nov 7, 2024
055e2b5
lint
Robert-Costello Nov 7, 2024
73e8ad1
add method to check for input errors and only execute search if there…
Robert-Costello Dec 17, 2024
e351a11
Merge branch 'master' into redundantNavMenuOnSearch
Robert-Costello Dec 17, 2024
d6381f4
Merge branch 'master' into redundantNavMenuOnSearch
Robert-Costello Dec 30, 2024
8be4ffa
remove unneeded arg and consolidate logic
Robert-Costello Jan 1, 2025
75a6181
remove unused func
Robert-Costello Jan 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 66 additions & 49 deletions corehq/apps/cloudcare/static/cloudcare/js/formplayer/menus/views/query.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -746,20 +746,28 @@ hqDefine("cloudcare/js/formplayer/menus/views/query", [

performSubmit: function (initiatedBy) {
var self = this;
self.validateAllFields().done(function () {
FormplayerFrontend.trigger(
"menu:query",
self.getAnswers(),
self.options.sidebarEnabled,
initiatedBy
);
if (self.smallScreenEnabled && self.options.sidebarEnabled) {
$('#sidebar-region').collapse('hide');
}
sessionStorage.submitPerformed = true;
self.executeSearch(initiatedBy).done(function (response) {
self.updateModels(response);
self.displayErrors();
});
},

executeSearch: function (initiatedBy) {
var self = this;
var request = FormplayerFrontend.getChannel().request(
"menu:query",
self.getAnswers(),
self.options.sidebarEnabled,
initiatedBy
);

if (self.smallScreenEnabled && self.options.sidebarEnabled) {
$('#sidebar-region').collapse('hide');
}
sessionStorage.submitPerformed = true;
return request;
},

updateSearchResults: function () {
var self = this;
var invalidRequiredFields = [];
Expand Down Expand Up @@ -796,34 +804,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

var self = this;
var promise = $.Deferred();
var invalidFields = [];
var updatingModels = self.updateModelsForValidation || self._updateModelsForValidation();

$.when(updatingModels).done(function (response) {
// Gather error messages
self._getChildren().forEach(function (childView) {
if (!childView.isValid()) {
invalidFields.push(childView.model.get('text'));
}
});

// Display error messages
FormplayerFrontend.trigger('clearNotifications');
if (invalidFields.length) {
var errorHTML = gettext("Please check the following fields:");
errorHTML += "<ul>" + _.map(invalidFields, function (f) {
return "<li>" + DOMPurify.sanitize(f) + "</li>";
}).join("") + "</ul>";
FormplayerFrontend.trigger('showError', errorHTML, true, false);
}

var invalidFields = self.displayErrors();
if (invalidFields.length) {
promise.reject(response);
} else {
promise.resolve(response);
}
});

return promise;
},

Expand All @@ -842,32 +832,59 @@ hqDefine("cloudcare/js/formplayer/menus/views/query", [
urlObject.setRequestInitiatedByTag(initiatedByTag);
var fetchingPrompts = FormplayerFrontend.getChannel().request("app:select:menus", urlObject);
$.when(fetchingPrompts).done(function (response) {
// Update models based on response
if (response.queryResponse) {
_.each(response.queryResponse.displays, function (responseModel, i) {
self._getChildModels()[i].set({
error: responseModel.error,
required: responseModel.required,
required_msg: responseModel.required_msg,
});
});
} else {
_.each(response.models, function (responseModel, i) {
const childModels = self._getChildModels();
childModels[i].set({
error: responseModel.get('error'),
required: responseModel.get('required'),
required_msg: responseModel.get('required_msg'),
});
});
}
self.updateModels(response);
promise.resolve(response);

});
sessionStorage.validationInProgress = false;
return promise;
},

displayErrors: function () {
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.

if (!childView.isValid()) {
invalidFields.push(childView.model.get('text'));
}
});

// Display error messages
FormplayerFrontend.trigger('clearNotifications');
if (invalidFields.length) {
var errorHTML = gettext("Please check the following fields:");
errorHTML += "<ul>" + _.map(invalidFields, function (f) {
return "<li>" + DOMPurify.sanitize(f) + "</li>";
}).join("") + "</ul>";
FormplayerFrontend.trigger('showError', errorHTML, true, false);
}
return invalidFields;
},

updateModels: function (response) {
var self = this;
// Update models based on response
if (response.queryResponse) {
_.each(response.queryResponse.displays, function (responseModel, i) {
self._getChildModels()[i].set({
error: responseModel.error,
required: responseModel.required,
required_msg: responseModel.required_msg,
});
});
} else {
_.each(response.models, function (responseModel, i) {
const childModels = self._getChildModels();
childModels[i].set({
error: responseModel.get('error'),
required: responseModel.get('required'),
required_msg: responseModel.get('required_msg'),
});
});
}
},

setStickyQueryInputs: function () {
formplayerUtils.setStickyQueryInputs(this.getAnswers());
},
Expand Down
6 changes: 3 additions & 3 deletions corehq/apps/cloudcare/static/cloudcare/js/formplayer/router.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ hqDefine("cloudcare/js/formplayer/router", [
// We can't do any menu navigation without an appId
FormplayerFrontend.trigger("apps:list");
} else {
menusController.selectMenu(urlObject);
return menusController.selectMenu(urlObject);
}
},
listUsers: function (page, query) {
Expand Down Expand Up @@ -226,7 +226,7 @@ hqDefine("cloudcare/js/formplayer/router", [
API.listMenus();
});

FormplayerFrontend.on("menu:query", function (queryDict, sidebarEnabled, initiatedByTag) {
FormplayerFrontend.getChannel().reply("menu:query", function (queryDict, sidebarEnabled, initiatedByTag) {
var urlObject = utils.currentUrlToObject();
var queryObject = _.extend(
{
Expand All @@ -243,7 +243,7 @@ hqDefine("cloudcare/js/formplayer/router", [
urlObject.setRequestInitiatedByTag(initiatedByTag);
let encodedUrl = utils.objectToEncodedUrl(urlObject.toJson());
sessionStorage.removeItem('selectedValues');
API.listMenus(encodedUrl);
return API.listMenus(encodedUrl);
});

FormplayerFrontend.on('restore_as:list', function () {
Expand Down
2 changes: 1 addition & 1 deletion corehq/apps/cloudcare/static/cloudcare/js/formplayer/spec/utils_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ hqDefine("cloudcare/js/formplayer/spec/utils_spec", [
assert.equal(response.type, "query");
assert.deepEqual(_.pluck(response.displays, 'id'), ['dob']);

FormplayerFrontend.trigger("menu:query", {dob: "2010-01-19"});
FormplayerFrontend.getChannel().request("menu:query", {dob: "2010-01-19"});
let url = Utils.currentUrlToObject();
assert.deepEqual(url.selections, ['1', 'action 0']);
assert.deepEqual(_.keys(url.queryData), ["search_command.m1"]);
Expand Down
2 changes: 1 addition & 1 deletion docs/web_apps.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,7 @@ Marionette `integrates with Backbone.Radio <https://marionettejs.com/docs/master

Although you can namespace channels, web apps uses a single ``formplayer`` channel for all messages, which is accessed using ``FormplayerFrontend.getChannel()``. You'll see calls to get the channel and then call ``request`` to get at a variety of global-esque data, especially the current user. All of these requests are handled by ``reply`` callbacks defined in ``FormplayerFrontend``.

``FormplayerFrontend`` also supports events, which behave similarly. Events are triggered directly on the ``FormplayerFrontend`` object, which defines ``on`` handlers. We tend to use events for navigation and do namespace some of them with ``:``, leading to events like ``menu:select``, ``menu:query``, and ``menu:show:detail``. Some helper events are not namespaced, such as ``showError`` and ``showSuccess``.
``FormplayerFrontend`` also supports events, which behave similarly. Events are triggered directly on the ``FormplayerFrontend`` object, which defines ``on`` handlers. We tend to use events for navigation and do namespace some of them with ``:``, leading to events like ``menu:select`` and ``menu:show:detail``. Some helper events are not namespaced, such as ``showError`` and ``showSuccess``.

Routing, URLs, and Middleware
=============================
Expand Down
Loading