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.

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

[Hold for payment 2024-03-06] [$500] Workspace-The archived task report is no longer archived after reopening the report. #29049

Closed
4 of 6 tasks
izarutskaya opened this issue Oct 7, 2023 · 62 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review

Comments

@izarutskaya
Copy link

izarutskaya commented Oct 7, 2023

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


Action Performed:

  1. Launch app
  2. Tap announce room
  3. Tap plus icon
  4. Select assign task
  5. Enter title "Ree task" and tap next
  6. Tap confirm task
  7. Navigate back to LHN
  8. Tap profile
  9. Tap Workspaces
  10. Select the Workspace(in whose announce room task is created)
  11. Tap on 3dots on top and delete the workspace
  12. Navigate to LHN and note the created task( " Ree Task") is archived
  13. Tap on archived Workspace and confirm Workspace is archived
  14. Open the archived announce room
  15. Tap on the task and note the task report header
  16. Tap back and tick the task
  17. Tap on the task again and note the task report header

Expected Result:

In archived WS-announce room, user must not be able to tick the task and interact.
The archived task report must remain archived after reopening the report also.

Actual Result:

In archived WS-announce room, user able to tick the task and interact.
The archived task report is no longer archived after reopening the report.

Workaround:

Unknown

Platforms:

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

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.79-3

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

Notes/Photos/Videos: Any additional supporting documentation

Bug6228480_1696662427683.ree.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause-Internal Team

Slack conversation: @

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~019a5d1f4f0cd37991
  • Upwork Job ID: 1710593181054087168
  • Last Price Increase: 2023-11-20
  • Automatic offers:
    • mollfpr | Reviewer | 27773918
    • s-alves10 | Contributor | 27773920
Issue OwnerCurrent Issue Owner: @muttmuure
@izarutskaya izarutskaya added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Oct 7, 2023
@melvin-bot melvin-bot bot changed the title Android-Workspace-The archived task report is no longer archived after reopening the report. [$500] Android-Workspace-The archived task report is no longer archived after reopening the report. Oct 7, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 7, 2023

Job added to Upwork: https://www.upwork.com/jobs/~019a5d1f4f0cd37991

@melvin-bot
Copy link

melvin-bot bot commented Oct 7, 2023

Triggered auto assignment to @muttmuure (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 7, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 7, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@melvin-bot
Copy link

melvin-bot bot commented Oct 7, 2023

Triggered auto assignment to Contributor-plus team member for initial proposal review - @mollfpr (External)

@graylewis
Copy link
Contributor

graylewis commented Oct 7, 2023

Proposal

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

Tasks that belong to an archived task report are still able to be completed/reopened

What is the root cause of that problem?

This actually seems to be a backend issue. This is evident due to the data coming from the API from the openReport call containing the property: "isDeletedParentAction": false. The reason the title on the task is briefly correct [shows {TaskName} (Archived)] is due to optimistic updates. The task report is never actually archived.

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

Backend code needs to be checked for bugs regarding the calculation of the isDeletedParentAction prop in the context of an archived workspace.

If a patch for the frontend is needed in the meantime, a line could be added to isDeletedParentAction which utilizes the persisted onyx store to look for the actionType "CLOSED" at the key for the report being opened.

@s-alves10
Copy link
Contributor

Proposal

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

The task report action in archived announce room is interactable and the task report is no longer archived after reopen the report

What is the root cause of that problem?

When we delete a workspace, we set the archived state of related reports here

..._.map(reports, ({reportID}) => ({
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
value: {
stateNum: CONST.REPORT.STATE_NUM.SUBMITTED,
statusNum: CONST.REPORT.STATUS.CLOSED,
hasDraft: false,
oldPolicyName: allPolicies[`${ONYXKEYS.COLLECTION.POLICY}${policyID}`].name,
},
})),

Task report is marked as archived as well. But when we reopen the task report, the backend returns state: 0, status: 0 again and so the task report returns to the normal state. This is one reason

After the workspace is deleted, we should disable the interaction of tasks. We disable the task using the below function

function canModifyTask(taskReport, sessionAccountID) {
if (ReportUtils.isCanceledTaskReport(taskReport)) {
return false;
}
if (sessionAccountID === getTaskOwnerAccountID(taskReport) || sessionAccountID === getTaskAssigneeAccountID(taskReport)) {
return true;
}
// If you don't have access to the task report (maybe haven't opened it yet), check if you can access the parent report
// - If the parent report is an #admins only room
// - If you are a policy admin
const parentReport = ReportUtils.getParentReport(taskReport);
return ReportUtils.isAllowedToComment(parentReport);
}

As you can see, we don't check if the parent report is an archived room. This is another reason

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

This issue requires both the backend and frontend fixes

  1. Backend
    When a workspace is deleted, the backend should mark all the reports related to the workspace archived

  2. Frontend
    We need to update the canModifyTask function above to check the parent report
    Update the above function as follows

function canModifyTask(taskReport, sessionAccountID) {
    if (ReportUtils.isCanceledTaskReport(taskReport)) {
        return false;
    }

    const parentReport = ReportUtils.getParentReport(taskReport);
    if (ReportUtils.isArchivedRoom(parentReport)) {
        return false;
    }

    if (sessionAccountID === getTaskOwnerAccountID(taskReport) || sessionAccountID === getTaskAssigneeAccountID(taskReport)) {
        return true;
    }

    // If you don't have access to the task report (maybe haven't opened it yet), check if you can access the parent report
    // - If the parent report is an #admins only room
    // - If you are a policy admin
    return ReportUtils.isAllowedToComment(parentReport);
}

This works as expected

What alternative solutions did you explore? (Optional)

@mollfpr
Copy link
Contributor

mollfpr commented Oct 9, 2023

Asking the internal team for the expected behavior of the task report when the parent report is archived.

@melvin-bot melvin-bot bot added the Overdue label Oct 11, 2023
@muttmuure
Copy link
Contributor

Did you find anything out @mollfpr ?

@melvin-bot melvin-bot bot removed the Overdue label Oct 11, 2023
@izarutskaya izarutskaya changed the title [$500] Android-Workspace-The archived task report is no longer archived after reopening the report. [$500] Workspace-The archived task report is no longer archived after reopening the report. Oct 13, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 14, 2023

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@melvin-bot melvin-bot bot added the Overdue label Oct 14, 2023
@mollfpr
Copy link
Contributor

mollfpr commented Oct 15, 2023

Not overdue, still no update on the thread, I'll bump it once again.

@melvin-bot melvin-bot bot removed the Overdue label Oct 15, 2023
@mollfpr
Copy link
Contributor

mollfpr commented Oct 17, 2023

Bumping the Slack again to get some help.

@s-alves10 For the canModifyTask logic, I think it's better to check the status instead of the parent report archived.

@s-alves10
Copy link
Contributor

@mollfpr

I think we need to confirm the expected behavior.

For the canModifyTask logic, I think it's better to check the status instead of the parent report archived.

Yes, you're right. But the status is changed after opening the report because backend returns state: 0, status: 0. Also the expected behavior is In archived WS-announce room, user must not be able to tick the task and interact.

@mollfpr
Copy link
Contributor

mollfpr commented Oct 18, 2023

I've got feedback, and the task report should behave as the nested thread works. So, even if the parent report is archived, the task report is still OPEN.

Yes, you're right. But the status is changed after opening the report because backend returns state: 0, status: 0.

The thread report status should not be changed on deleting the workspace.


In archived WS-announce room, user must not be able to tick the task and interact.

@muttmuure Could you confirm if the above expectation is still needed? For me, it does not make sense, because the task report is still OPEN and the user can't interact with it.

The archived task report must remain archived after reopening the report also.

We can remove this expected result.

The only thing we can do in this issue is, fix the LHN showing that the task report is archived after deleting the workspace.

@muttmuure Let me know what you think from the above. Thank you!

@melvin-bot melvin-bot bot added the Overdue label Oct 20, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 21, 2023

@mollfpr @muttmuure 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
Copy link

melvin-bot bot commented Oct 21, 2023

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@melvin-bot
Copy link

melvin-bot bot commented Oct 23, 2023

@mollfpr, @muttmuure Huh... This is 4 days overdue. Who can take care of this?

@melvin-bot
Copy link

melvin-bot bot commented Oct 25, 2023

@mollfpr, @muttmuure Still overdue 6 days?! Let's take care of this!

@melvin-bot melvin-bot bot added the Overdue label Jan 16, 2024
@neil-marcellini neil-marcellini added the Reviewing Has a PR in review label Jan 16, 2024
@melvin-bot melvin-bot bot removed the Overdue label Jan 16, 2024
@neil-marcellini
Copy link
Contributor

The PR went through the first round of reviews. I'm going to do a bit more manually testing and make sure the proper updates are pushed to users, then put it up for review again soon.

@neil-marcellini neil-marcellini changed the title [HOLD][$500] Workspace-The archived task report is no longer archived after reopening the report. [HOLD Expensify 308026][$500] Workspace-The archived task report is no longer archived after reopening the report. Jan 31, 2024
@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Feb 9, 2024
Copy link

melvin-bot bot commented Feb 9, 2024

This issue has not been updated in over 15 days. @pecanoro, @mollfpr, @neil-marcellini, @muttmuure, @s-alves10 eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@neil-marcellini neil-marcellini changed the title [HOLD Expensify 308026][$500] Workspace-The archived task report is no longer archived after reopening the report. [$500] Workspace-The archived task report is no longer archived after reopening the report. Feb 13, 2024
@neil-marcellini
Copy link
Contributor

This should be fixed now. @muttmuure would you please verify if it's fixed and let us know if anything else is needed?

@neil-marcellini neil-marcellini added Weekly KSv2 and removed Reviewing Has a PR in review Monthly KSv2 labels Feb 13, 2024
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Weekly KSv2 labels Feb 14, 2024
@s-alves10
Copy link
Contributor

@mollfpr @muttmuure
PR is ready for review: #31674

@s-alves10
Copy link
Contributor

@pecanoro

Automation looks not working. Pay day would be 6th, Mar here

@pecanoro pecanoro changed the title [$500] Workspace-The archived task report is no longer archived after reopening the report. [Hold for payment 2024-03-06] [$500] Workspace-The archived task report is no longer archived after reopening the report. Feb 29, 2024
@pecanoro pecanoro added the Awaiting Payment Auto-added when associated PR is deployed to production label Feb 29, 2024
@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Mar 6, 2024
@muttmuure
Copy link
Contributor

Seems like this is fixed, issuing payment now

@muttmuure
Copy link
Contributor

Sent new offer to @mollfpr since the original offer expired

@muttmuure
Copy link
Contributor

@s-alves10 has been paid

@mollfpr
Copy link
Contributor

mollfpr commented Mar 9, 2024

[@mollfpr] The PR that introduced the bug has been identified. Link to the PR:
[@mollfpr] 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:

No offending PR was found. This is an improvement and need a backend change to fix the issue both ways.

[@mollfpr] 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:

The regression step should be good.

[@mollfpr] Determine if we should create a regression test for this bug.
[@mollfpr] 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.

  1. Open ND and log in with any account
  2. Go to announce room
  3. Create a task
  4. Delete the workspace in whose announce room the task is just created
  5. Note that the task report was archived in LHN
  6. Tap task in the archived announce room
  7. Verify that the task report remains archived and a user isn't able to tick the task to interact
  8. 👍 or 👎

@muttmuure I'll request in NewDot. Could you create the payment summary? Thank you!

@muttmuure
Copy link
Contributor

$500 - @mollfpr C+

@JmillsExpensify
Copy link

$500 approved for @mollfpr based on summary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests