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

Jump to bottom

Filename broadcast bug fix 2 #35108

Merged
merged 7 commits into from
Sep 17, 2024
Merged

Filename broadcast bug fix 2 #35108

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
de4dbfc
Remove function call to non function object attribute + search for co…
minhaminha Sep 5, 2024
74b766d
Lint fixes
minhaminha Sep 9, 2024
3bf0501
Change function to a let
minhaminha Sep 9, 2024
3990539
PR suggested changes
minhaminha Sep 9, 2024
f85775a
Add a check to ensure filename is appeneded to the correct input ques…
minhaminha Sep 11, 2024
f708664
Lint fix
minhaminha Sep 11, 2024
a32b5b1
Check that ix and binding matches before assigning filename
minhaminha Sep 16, 2024
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
2 changes: 1 addition & 1 deletion corehq/apps/cloudcare/static/cloudcare/js/form_entry/entries.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
'use strict';

Check warning on line 1 in corehq/apps/cloudcare/static/cloudcare/js/form_entry/entries.js

View workflow job for this annotation

GitHub Actions / Lint Javascript 

'use strict' is unnecessary inside of modules
hqDefine("cloudcare/js/form_entry/entries", [
'jquery',
'knockout',
Expand Down Expand Up @@ -940,7 +940,7 @@
FileEntry.prototype.constructor = EntrySingleAnswer;
FileEntry.prototype.onPreProcess = function (newValue) {
var self = this;
if (newValue === "" && self.question.filename()) {
if (newValue === "" && self.question.filename) {
self.question.hasAnswered = true;
self.fileNameDisplay(self.question.filename());
} else if (newValue !== constants.NO_ANSWER && newValue !== "") {
Expand Down
16 changes: 12 additions & 4 deletions corehq/apps/cloudcare/static/cloudcare/js/form_entry/form_ui.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
'use strict';

Check warning on line 1 in corehq/apps/cloudcare/static/cloudcare/js/form_entry/form_ui.js

View workflow job for this annotation

GitHub Actions / Lint Javascript 

'use strict' is unnecessary inside of modules
hqDefine("cloudcare/js/form_entry/form_ui", [
'jquery',
'knockout',
Expand Down Expand Up @@ -691,13 +691,21 @@
element.serverError(null);
}

if (allChildren) {
for (let i = 0; i < allChildren.length; i++) {
if (allChildren[i].control >= constants.CONTROL_IMAGE_CHOOSE) {
allChildren[i].filename = element.answer();
var findChildAndSetFilename = function (children) {
Copy link
Contributor

Choose a reason for hiding this comment

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

lets use const and let where possible.

for (var i = 0; i < children.length; i++) {
Jtang-1 marked this conversation as resolved.
Show resolved Hide resolved
var child = children[i];
if (child.children && child.children.length > 0) {
findChildAndSetFilename(child.children);
} else if (child.control >= constants.CONTROL_IMAGE_CHOOSE) {
Jtang-1 marked this conversation as resolved.
Show resolved Hide resolved
child.filename = element.answer();
Copy link
Contributor

Choose a reason for hiding this comment

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

I realize i'm not completely understanding this. Does element represent a single question? It seems to me that this for loop will overwrite all filenames to be the same value - is that true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep as far as I can tell element does represent a single question. And thanks for bringing up the concern - I did some testing and found that there is a pretty convoluted edge case where if you have 2 input questions of the same type AND one them is broadcasting their filename to a text question AND they're all (2 input questions and a text question) contained in the same question tile AND the input question that's not broadcasting is placed before the the broadcasting question, then it'll assign the filename to the wrong input question. I tried a bunch of different variations of this format but this is strangely the only instance where it doesn't work as you'd expect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll see if there's something obvious I missed that can more assuredly pick out the right child to assign the filename to

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just added a check that will only assign filename to input questions that match the broadcast style (or lack thereof) of the question you just answered. Testing locally seems to work fine (questions without any style at all don't need this - this is specifically for input questions within the same question tile block that needs updating frequently like when there's a conditionally visible question) but I'm going to ask QA to have another look at it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, there's still a part i'm confused on. Is this the appearance attribute this work is related to? So element represents the text question and has an appearance attribute receive... and if so, why would element.style.raw().includes('broadcast') ever be true?

Is what you're doing checking that the topic portion of the appearance attribute matches between element and child

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep that's the broadcasting logic I'm dealing with. Element in this case is representing the input question and needs the filename to properly populate the text next to the upload button (this resolved the previous bug we had where having it broadcast to a hidden question in the same question tile would wipe the uploaded file the first time). I'm not sure what you mean by topic but yea I'm comparing the style attribute between element and child to make sure it targets the correct input question, if there are multiple. I guess in that regard I can just make sure they're completely equal instead of checking whether they both contain 'broadcast' or not but I feel like the latter method emphasizes that this is specifically to handle for certain edge cases associated with the filename broadcasting logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm ok the only part i'm still not understanding is, if there are multiple input questions broadcasting (such that they each have a broadcast appearance attribute), would this conditional incorrectly match to all of them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

theoretically I think so... but that would mean there would have to be two or more input questions broadcasting to one or more receiving text questions present in a single question tile (basically all in a row) which doesn't make sense to do in a form.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I'm not even sure you can have more than broadcasting/receiving pair in a question tile? In any case, I think the best fix for this (assigning filename based on question/child id) is a bit out of scope for this relatively small issue since it'll probably require additional formplayer changes too. How would you feel about keeping track of this as a backlog item to revisit in the future?

break;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: i think we can just return here?

}
}
};
if (allChildren && allChildren.length > 0) {
findChildAndSetFilename(allChildren);
}

response.children = allChildren;
self.fromJS(response);
}
Expand Down
Loading