[HOLD for payment 2024-09-06] [$250] There is no selected background after selecting Submission frequency as monthly

[m-natarajan]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 9.0.21-0
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @ZhenjaHorbach
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1723812299682989

Action Performed:

1. Open account settings
2. Open workspaces and choose any workspace
3. In more features turn on Workflows and open it
4. Turn on Delay submissions and open Submission frequency
5. Choose monthly

Expected Result:

There should be selected background

Actual Result:

Notice that there is a check mark next to monthly, but there is no selected background

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

2024-08-16.14.43.26.mov

Recording.2024.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01010057c5f8be0ff8
• Upwork Job ID: 1824501452626526630
• Last Price Increase: 2024-08-16
• Automatic offers:
• ZhenjaHorbach | Reviewer | 103626776
• Krishna2323 | Contributor | 103626778

Issue Owner

Current Issue Owner: @

[melvin-bot]

Triggered auto assignment to @puneetlath (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[ZhenjaHorbach]

@puneetlath
I reported this issue
Can I be a reviewer here please?

[Krishna2323]

Edited by proposal-police: This proposal was edited at 2024-08-20 21:37:23 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

There is no selected background after selecting Submission frequency as monthly

What is the root cause of that problem?

• shouldUpdateFocusedIndex isn't passed to SelectionList.
  

  App/src/pages/workspace/workflows/WorkspaceAutoReportingFrequencyPage.tsx

  Lines 135 to 140 in a07cce4

  	<SelectionList
  	ListItem={RadioListItem}
  	sections={[{data: autoReportingFrequencyItems}]}
  	onSelectRow={onSelectAutoReportingFrequency}
  	initiallyFocusedOptionKey={autoReportingFrequency}
  	/>

• The second visual glitch mentioned here occurs because the footer content is rendered before the option is marked as selected. This happens because we update the isMonthlyFrequency state to true as soon as an option is selected. However, the option is only marked as selected when frequencyKey === autoReportingFrequency, and autoReportingFrequency is derived from the Onyx state.

App/src/pages/workspace/workflows/WorkspaceAutoReportingFrequencyPage.tsx

Lines 105 to 106 in 4e01691

	isSelected: frequencyKey === autoReportingFrequency,
	footerContent: isMonthlyFrequency && frequencyKey === CONST.POLICY.AUTO_REPORTING_FREQUENCIES.MONTHLY ? monthlyFrequencyDetails() : null,

App/src/pages/workspace/workflows/WorkspaceAutoReportingFrequencyPage.tsx

Lines 60 to 63 in 4e01691

	if (item.keyForList === CONST.POLICY.AUTO_REPORTING_FREQUENCIES.MONTHLY) {
	setIsMonthlyFrequency(true);
	return;
	}

What changes do you think we should make in order to solve the problem?

• We should pass shouldUpdateFocusedIndex to update the focused index on every selection change.
• For the second bug we can remove the local state isMonthlyFrequency and use the frequencyKey === autoReportingFrequency check directly.

What alternative solutions did you explore? (Optional)

Result for the visual glitch mention here

Before

freq_before.mp4

After

freq_after.mp4

[cretadn22]

Edited by proposal-police: This proposal was edited at 2024-08-16 16:18:58 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

There is no selected background after selecting Submission frequency as monthly

What is the root cause of that problem?

App/src/pages/workspace/workflows/WorkspaceAutoReportingFrequencyPage.tsx

Lines 135 to 140 in a07cce4

	<SelectionList
	ListItem={RadioListItem}
	sections={[{data: autoReportingFrequencyItems}]}
	onSelectRow={onSelectAutoReportingFrequency}
	initiallyFocusedOptionKey={autoReportingFrequency}
	/>

We don't pass shouldUpdateFocusedIndex, shouldSingleExecuteRowSelect props

What changes do you think we should make in order to solve the problem?

For the main bug: We need to pass shouldUpdateFocusedIndex, shouldSingleExecuteRowSelect props

For the new bug, I suggest creating a new state and using it instead of autoReportingFrequency.

const [isSelected, setIsSelected] = useState(autoReportingFrequency)

We also need to call setIsSelected(item.keyForList) in the onSelectAutoReportingFrequency function.

We can add a new Save button and only call the API when the user clicks this button. This pattern has been confirmed by the design team here and can also be applied to the WorkspaceAutoReportingFrequencyPage.

What alternative solutions did you explore? (Optional)

[daledah]

Edited by proposal-police: This proposal was edited at 2024-08-16 19:36:01 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

A check mark next to monthly, but there is no selected background

What is the root cause of that problem?

We just updated isSelected and not updated the focused index in here

App/src/components/SelectionList/BaseSelectionList.tsx

Lines 449 to 455 in 9812479

	onSelectRow={() => {
	if (shouldSingleExecuteRowSelect) {
	singleExecution(() => selectRow(item, index))();
	} else {
	selectRow(item);
	}
	}}

What changes do you think we should make in order to solve the problem?

We should add 2 props shouldUpdateFocusedIndex and shouldSingleExecuteRowSelect to SelectionList here

App/src/pages/workspace/workflows/WorkspaceAutoReportingFrequencyPage.tsx

Lines 135 to 140 in 9812479

	<SelectionList
	ListItem={RadioListItem}
	sections={[{data: autoReportingFrequencyItems}]}
	onSelectRow={onSelectAutoReportingFrequency}
	initiallyFocusedOptionKey={autoReportingFrequency}
	/>

OPTIONAL: We should pass index to function selectRow in here to reset focusedIndex when props shouldSingleExecuteRowSelect = false

App/src/components/SelectionList/BaseSelectionList.tsx

Line 453 in 9812479

selectRow(item);

When we call SetWorkspaceAutoReportingFrequency we should set isLoading = true and after the API call is done, it needs to be reset isLoading = false

and we'll disable options when isLoading=true or display them in some way to let users know they need to wait

What alternative solutions did you explore? (Optional)

[cretadn22]

@daledah Your proposal is the same as my proposal 😄

[Krishna2323]

Proposal Updated

• Added a note.

[daledah]

@cretadn22 I see there is a difference in the OPTIONAL part. In the future when we pass props shouldSingleExecuteRowSelect = false, we can still recalculate the focusIndex.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01010057c5f8be0ff8

[melvin-bot]

Current assignee @ZhenjaHorbach is eligible for the External assigner, not assigning anyone new.

[ZhenjaHorbach]

@Krishna2323 @cretadn22 @daledah
Thanks for your proposals !
All your proposals are correct
But after selecting Submission frequency as monthly we can see that check mark sets with a delay, which is why we see an unpleasant visual bug
I think we should cover this in this issue
Can you please update your proposals to account for that?

[cretadn22]

Updated proposal to address the new bug mentioned by @ZhenjaHorbach

[daledah]

@ZhenjaHorbach The bug happened because when we were calling the API SetWorkspaceAutoReportingFrequency and we updated it again with a different value

Updated proposal

Screen.Recording.2024-08-17.at.02.29.12.mov

[ZhenjaHorbach]

Sorry to confuse you 😅
I meant this bug
Since the only way to reproduce it is selecting Submission frequency as monthly
I think it should be fixed.
As you can see we have check mark with a delay

2024-08-16.21.47.41.mov

[ZhenjaHorbach]

@ZhenjaHorbach The bug happened because when we were calling the API SetWorkspaceAutoReportingFrequency and we updated it again with a different value

Updated proposal

Screen.Recording.2024-08-17.at.02.29.12.mov

@daledah
But actually thanks for mentioning this bug
But in this case I think it does not concern this issue

[ZhenjaHorbach]

But on the other hand
What do you think ? puneetlath
We have two additional issues

1. We have check mark with a delay when select Submission frequency as monthly

#47574 (comment)

2. When we quickly change Submission frequencies several times
   We will see that on the screen Submission frequencies will change one by one until we get the latest current Submission frequency

#47574 (comment)

Or should we fix only the current issue described in the Action Performed?

[melvin-bot]

@puneetlath, @ZhenjaHorbach Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[puneetlath]

Hmm, I think if they all touch this same flow, then let's just fix them here.

[ZhenjaHorbach]

@Krishna2323 @cretadn22 @daledah
Could you update your proposals, please ?

[Krishna2323]

When we quickly change Submission frequencies several times
We will see that on the screen Submission frequencies will change one by one until we get the latest current Submission frequency

@ZhenjaHorbach, I believe this is not related to our current issue, the current bug is related to the UI, so I guess we can solve the other 2 bugs.

[ZhenjaHorbach]

When we quickly change Submission frequencies several times
We will see that on the screen Submission frequencies will change one by one until we get the latest current Submission frequency

@ZhenjaHorbach, I believe this is not related to our current issue, the current bug is related to the UI, so I guess we can solve the other 2 bugs.

It makes sense to me !

[Krishna2323]

@ZhenjaHorbach, I updated my proposal to fix the second visual glitch mentioned here. Please review and let me know when you have some time. Thanks

[ZhenjaHorbach]

@Krishna2323
Thanks for update !
I agree that IsMonthlyFrequency is a bit redundant in this case
So I'm happy to go with this proposal

🎀👀🎀 C+ reviewed

[melvin-bot]

Current assignee @puneetlath is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[melvin-bot]

📣 @ZhenjaHorbach 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @Krishna2323 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[Krishna2323]

@ZhenjaHorbach, PR ready for review ^

[ZhenjaHorbach]

Looks like production deploy automation failed: This should be on [HOLD for Payment 2024-09-06] according to prod deploy checklist, confirmed in slack

[puneetlath]

Ah ok cool. @ZhenjaHorbach can you complete the checklist in that case?

[ZhenjaHorbach]

BugZero Checklist

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@ZhenjaHorbach] The PR that introduced the bug has been identified. Link to the PR:

#42019

• [ @ZhenjaHorbach] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:

https://github.com/Expensify/App/pull/42019/files#r1747587652

• [@ZhenjaHorbach] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

NA

• [@ZhenjaHorbach] Determine if we should create a regression test for this bug.
• [@ZhenjaHorbach] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Since it's a minor issue with a simple fix that we use in many places
I'm not sure if regression tests are needed here

[ZhenjaHorbach]

Ah ok cool. @ZhenjaHorbach can you complete the checklist in that case?

Done !

[Krishna2323]

@puneetlath, bump for payments 🙏🏻

[puneetlath]

Great, thanks everyone. All paid!