Open m-natarajan opened 1 month ago
melvin-bot[bot]
commented
1 month ago Triggered auto assignment to @JmillsExpensify (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.
daledah
commented
1 month ago not found page is displayed briefly
In here, we have logic to calculate currentApprovalWorkflow base on approvalWorkflows and firstApprover get from route params
When we change the first approval, we only update the data of approvalWorkflows without updating the route params so currentApprovalWorkflow will be empty => shouldShowNotFoundView = true
We should close the modal before updating the approvalWorkflows data using InteractionManager.runAfterInteractions
here
Navigation.dismissModal();
InteractionManager.runAfterInteractions(() => {
Workflow.updateApprovalWorkflow(route.params.policyID, approvalWorkflow, membersToRemove, approversToRemove);
});
DylanDylann
commented
1 month ago @JmillsExpensify I discovered this bug and have more context because I was also the main C+ on the approval workflow feature. I am happy to take this issue as C+ if it is external
This bug is handled on removing flow on https://github.com/Expensify/App/issues/48089 and we also fix for this case where users update the first approver
bernhardoj
commented
1 month ago A not found is shown after updating the first approver.
The not found will show if the current approval workflow isn't found. https://github.com/Expensify/App/blob/faf48fcaaf9bb1a8dca6e997e3e1a85606a97ace/src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsEditPage.tsx#L92-L93
The current approval workflow finds the workflow where the approver is the same as the first approver route params. https://github.com/Expensify/App/blob/faf48fcaaf9bb1a8dca6e997e3e1a85606a97ace/src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsEditPage.tsx#L87
When we open the page with the first approver of A, then update it to B, A won't be in the approver list anymore, so current approval workflow can't be found.
Instead of using navigation focused state to prevent the not found page (which is not reliable IMO), we can prevent the current approval workflow data to be updated when we are submitting the update. To do that, we need 2 new ref.
const isUpdatedRef = useRef(false);
const prevApprovalData = useRef({});isUpdatedRef tells whether the user updates the data or not. prevApprovalData contains the previous approval workflow data.
Set isUpdatedRef to true when updating the data.
https://github.com/Expensify/App/blob/faf48fcaaf9bb1a8dca6e997e3e1a85606a97ace/src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsEditPage.tsx#L41-L55
Store the approval data to prevApprovalData so we can use it later and return the prevApprovalData if isUpdatedRef is true.
const {currentApprovalWorkflow, defaultWorkflowMembers, usedApproverEmails} = useMemo(() => {
if (!policy || !personalDetails) {
return {};
}
if (isUpdatedRef.current) {
return prevApprovalData.current;
}
const defaultApprover = policy?.approver ?? policy.owner;
const firstApprover = route.params.firstApproverEmail;
const result = convertPolicyEmployeesToApprovalWorkflows({...});
const approvalData = {
defaultWorkflowMembers: result.availableMembers,
usedApproverEmails: result.usedApproverEmails,
currentApprovalWorkflow: result.approvalWorkflows.find((workflow) => workflow.approvers.at(0)?.email === firstApprover),
};
prevApprovalData.current = approvalData;
return approvalData;
}, [personalDetails, policy, route.params.firstApproverEmail]);Prevent the not found from showing if the page isn't focused if we prefer that way.
const shouldShowNotFoundView = isFocused && ...@JmillsExpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
melvin-bot[bot]
commented
1 month ago Job added to Upwork: https://www.upwork.com/jobs/~021851668069597317065
melvin-bot[bot]
commented
1 month ago Triggered auto assignment to Contributor-plus team member for initial proposal review - @thesahindia (External)
JmillsExpensify
commented
1 month ago Opened up to C+ for review. Please make sure to see the comment from @DylanDylann in the review process, as this bug report wouldn't exist unless he reported it.
FitseTLT
commented
1 month ago Edited by proposal-police: This proposal was edited at 2024-10-31 12:39:04 UTC.
[$250] Not found page appear briefly after updating the first approver
While we are editing a workflow we keep track of the currently edited workflow in onyx with the key ONYXKEYS.APPROVAL_WORKFLOW
https://github.com/Expensify/App/blob/664e657f3c5d5f1ce4c55d09eee054519228432e/src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsEditPage.tsx#L36
So we always reset to the currently opened workflow whenever we open a new workflow approval here
https://github.com/Expensify/App/blob/664e657f3c5d5f1ce4c55d09eee054519228432e/src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsEditPage.tsx#L95-L113
and then use it to display all necessary data while we are on the edit approval workflow page
https://github.com/Expensify/App/blob/664e657f3c5d5f1ce4c55d09eee054519228432e/src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsEditPage.tsx#L135-L143
So what is displayed in the current state of the workflow edit page solely depend on approvalWorkflow but we wrongly set the shouldShowNotFound to depend on currentApprovalWorkflow
https://github.com/Expensify/App/blob/664e657f3c5d5f1ce4c55d09eee054519228432e/src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsEditPage.tsx#L92-L93
but currentApprovalWorkflow will change the moment we hit the save button so because the workflow is update in onyx immediately if the firstApprover is changed it will not find an approval workflow based on the old firstApprover that it gets from the route
https://github.com/Expensify/App/blob/664e657f3c5d5f1ce4c55d09eee054519228432e/src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsEditPage.tsx#L87
As discussed in the RCA WorkspaceWorkflowsApprovalsEditPage loads and updates/tracks the current state of the approval workflow that is being edited in approvalWorkflow as a draft and what it displays also directly linked to approvalWorkflow so there is no reason shouldShowNotFoundView should depend on currentApprovalWorkflow it should only depend on approvalWorkflow, so change
https://github.com/Expensify/App/blob/664e657f3c5d5f1ce4c55d09eee054519228432e/src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsEditPage.tsx#L92-L93
const shouldShowNotFoundView = (isEmptyObject(policy) && !isLoadingReportData) || !PolicyUtils.isPolicyAdmin(policy) || PolicyUtils.isPendingDeletePolicy(policy) || !approvalWorkflow;
thesahindia
commented
4 weeks ago @JmillsExpensify I discovered this bug and have more context because I was also the main C+ on the approval workflow feature. I am happy to take this issue as C+ if it is external
This bug is handled on removing flow on #48089 and we also fix for this case where users update the first approver
@DylanDylann, you can take this one.
Just a note: C+ can't request to be assigned because they have reported the issue (which was discussed). This is for fairness.
DylanDylann
commented
4 weeks ago Thanks. reviewing now
DylanDylann
commented
4 weeks ago Thanks everyone.
I prefer to go with @daledah's proposal because it is simpler and their way already be used widely in other places
🎀 👀 🎀 C+ Reviewed
melvin-bot[bot]
commented
4 weeks ago Triggered auto assignment to @deetergp, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
melvin-bot[bot]
commented
4 weeks ago @deetergp @JmillsExpensify @thesahindia this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!
melvin-bot[bot]
commented
3 weeks ago 📣 @daledah 🎉 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 📖
melvin-bot[bot]
commented
1 week ago Reviewing label has been removed, please complete the "BugZero Checklist".
melvin-bot[bot]
commented
1 week ago The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.64-4 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:
If no regressions arise, payment will be issued on 2024-11-28. :confetti_ball:
For reference, here are some details about the assignees on this issue:
melvin-bot[bot]
commented
1 week ago @deetergp @JmillsExpensify @daledah The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]
DylanDylann
commented
6 days ago [x] [Contributor] 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/47249#discussion_r1861476758
[ ] [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source 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:
[x] [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.
[ ] [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.
Link to issue:
Do we agree 👍 or 👎
melvin-bot[bot]
commented
4 days ago ⚠️ Looks like this issue was linked to a Deploy Blocker here
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.
If a regression has occurred and you are the assigned CM follow the instructions here.
If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.
melvin-bot[bot]
commented
1 day ago @deetergp, @JmillsExpensify, @daledah Huh... This is 4 days overdue. Who can take care of this?
JmillsExpensify
commented
22 hours ago Payment summary:
JmillsExpensify
commented
22 hours ago Regression test created and one contributor payment made.
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: 9.0.52-1 Reproducible in staging?: y Reproducible in production?: y If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: 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: @DylanDylann Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1729573070656459
Action Performed:
Expected Result:
not found pageshould not displayActual Result:
not found pageis displayed brieflyWorkaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
https://github.com/user-attachments/assets/941bbae3-7682-4bf4-9321-2bd40abff472 https://github.com/user-attachments/assets/8e14903e-2983-4bef-a2e1-fc751a049816View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @JmillsExpensify