Filename broadcast bug fix 2 #35108

minhaminha · 2024-09-09T13:47:28Z

Product Description

Continuation of this PR that addressed the initial problem of filename not being propagated to hidden text questions within the same question tile.

Technical Summary

Jira Ticket

I hadn't realized the extent to which a single question can be buried under parent questions / nested under additional question tiles so all this adds is a recursion logic that searches for any questions of file type input (control >= 10) throughout the form question tree and sets the correct filename based on the ix and binding properties.

Feature Flag

WEB_APPS_UPLOAD_QUESTIONS

Safety Assurance

Safety story

Tested on staging - QA will test again on prod. Does the same thing as before but checks children questions to ensure the right question gets the right filename.

QA Plan

QA will test again on prod. (previous 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

…rrect child entry to append filename to

MartinRiese · 2024-09-09T17:19:14Z

corehq/apps/cloudcare/static/cloudcare/js/form_entry/form_ui.js

-                    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) {


lets use const and let where possible.

Jtang-1

looks good, couple comments

corehq/apps/cloudcare/static/cloudcare/js/form_entry/form_ui.js

Jtang-1 · 2024-09-09T18:15:35Z

corehq/apps/cloudcare/static/cloudcare/js/form_entry/form_ui.js

+                            findChildAndSetFilename(child.children);
+                        } else if (child.control >= constants.CONTROL_IMAGE_CHOOSE) {
+                            child.filename = element.answer();
+                            break;


nit: i think we can just return here?

corehq/apps/cloudcare/static/cloudcare/js/form_entry/form_ui.js

Jtang-1 · 2024-09-09T20:39:22Z

corehq/apps/cloudcare/static/cloudcare/js/form_entry/form_ui.js

+                        if (child.children && child.children.length > 0) {
+                            findChildAndSetFilename(child.children);
+                        } else if (inputControl.includes(child.control)) {
+                            child.filename = element.answer();


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?

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.

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

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.

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

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.

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?

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.

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?

…tion based on broadcast styles or lack thereof

Jtang-1

looks good! Thank you for answering my questions

Jtang-1 · 2024-09-16T18:01:40Z

corehq/apps/cloudcare/static/cloudcare/js/form_entry/form_ui.js

+                    for (let child of children) {
+                        if (child.children && child.children.length > 0) {
+                            findChildAndSetFilename(child.children);
+                        } else if (inputControl.includes(child.control) && element.binding() === child.binding &&


What is this binding property?

not a 100% sure what it's used by but as far as I can tell its the "path" to the question. in the simplest instance it'll just be /data/<question_name> but it'll always end with the question name

interesting... in that case, is there a reason to check equality for both binding and ix? From what i remember, ix is the key used for knockout mapping to identify elements. Did you find that checking for just ix equality isn't strict enough?

Yep, at least in the few sample forms I've set up with inputs in different broadcasting/question tile configurations I've found that it does sometimes get a little confused when just checking for ix.

Remove function call to non function object attribute + search for co…

de4dbfc

…rrect child entry to append filename to

minhaminha added the product/feature-flag Change will only affect users who have a specific feature flag enabled label Sep 9, 2024

minhaminha requested review from orangejenny, MartinRiese, AddisonDunn, stephherbers, Robert-Costello, Jtang-1 and nospame as code owners September 9, 2024 13:47

dimagimon added the Risk: Medium Change affects files that have been flagged as medium risk. label Sep 9, 2024

Lint fixes

74b766d

MartinRiese reviewed Sep 9, 2024

View reviewed changes

Change function to a let

3bf0501

Jtang-1 approved these changes Sep 9, 2024

View reviewed changes

PR suggested changes

3990539

Jtang-1 reviewed Sep 9, 2024

View reviewed changes

Add a check to ensure filename is appeneded to the correct input ques…

f85775a

…tion based on broadcast styles or lack thereof

minhaminha added the awaiting QA QA in progress. Do not merge label Sep 11, 2024

minhaminha added 2 commits September 11, 2024 12:10

Lint fix

f708664

Check that ix and binding matches before assigning filename

a32b5b1

Jtang-1 approved these changes Sep 16, 2024

View reviewed changes

minhaminha added QA Passed and removed awaiting QA QA in progress. Do not merge labels Sep 17, 2024

minhaminha merged commit a4afa7c into master Sep 17, 2024
12 checks passed

minhaminha deleted the ml/broadcast-bug branch September 17, 2024 19:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filename broadcast bug fix 2 #35108

Filename broadcast bug fix 2 #35108

minhaminha commented Sep 9, 2024 •

edited

Loading

MartinRiese Sep 9, 2024

Jtang-1 left a comment

Jtang-1 Sep 9, 2024

Jtang-1 Sep 9, 2024

minhaminha Sep 10, 2024

minhaminha Sep 10, 2024

minhaminha Sep 11, 2024

Jtang-1 Sep 12, 2024

minhaminha Sep 12, 2024

Jtang-1 Sep 12, 2024

minhaminha Sep 13, 2024

minhaminha Sep 13, 2024

Jtang-1 left a comment •

edited

Loading

Jtang-1 Sep 16, 2024

minhaminha Sep 16, 2024

Jtang-1 Sep 16, 2024

minhaminha Sep 17, 2024

Filename broadcast bug fix 2 #35108

Filename broadcast bug fix 2 #35108

Conversation

minhaminha commented Sep 9, 2024 • edited Loading

Product Description

Technical Summary

Feature Flag

Safety Assurance

Safety story

QA Plan

Rollback instructions

Labels & Review

Choose a reason for hiding this comment

Jtang-1 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jtang-1 left a comment • edited Loading

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

minhaminha commented Sep 9, 2024 •

edited

Loading

Jtang-1 left a comment •

edited

Loading