[HOLD for payment 2023-09-21] [$1000] App allows task title update for completed task using deep link and throws error on update

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Open the app
2. Click on plus and click on 'Assign a task'
3. Write any title and assign task to anyone
4. In task report, click on task to open task title edit
5. Copy the URL and send it to any other report
6. Mark the task as done
7. Click on link sent in step 5, edit the title and click on save
8. Observe that task report in LHN now has red dot for error

Expected Result:

Once task is completed, app should not allow to edit task title by deep link

Actual Result:

App allows to edit task title using deep link even after task is marked as completed and if edited using the link, app displays red dot in LHN on task report

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

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.38-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

red.dot.for.task.update.already.completed.mov

Recording.1218.mp4

Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1688742180722969

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01df4c0b6543fe49a8
• Upwork Job ID: 1678539598526873600
• Last Price Increase: 2023-08-28
• Automatic offers:
• BhuvaneshPatil | Contributor | 26456675
• dhanashree-sawant | Reporter | 26456679

[melvin-bot]

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

[melvin-bot]

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

[hoangzinh]

Proposal

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

App allows task title update for completed task using deep link and throws error on update

What is the root cause of that problem?

In the TaskTitlePage component, we haven't have logic to check whether the task is complete/cancelled before showing the page.

This issue is also reproducible in edit Task description, edit Task assignee and edit Task share destination pages.

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

We should add logic into the TaskTitlePage component so that if the Task is not open (means is completed or cancelled), then we can either navigate back or show not show page. We can put those logic into an HOC/hook to reuse it in other 3 pages above. I imagine it almost same with this existing HOC withReportOrNotFound

[BhuvaneshPatil]

Proposal

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

App allows task title update for completed task using deep link and throws error on update

What is the root cause of that problem?

We are not having any check (whether task is open) when we are opening TaskTitlePage using link. That's causing this issue.
Same error is happening with TaskDesscriptionPage

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

We can wrap the TaskTitlePage with FullPageNotFoundView component.
For shouldShow prop, we can add the required checks -

• If the report/task is open or not

For this we are using this method -

App/src/components/ReportActionItem/TaskView.js

Line 50 in 6a5b1a4

const isOpen = ReportUtils.isOpenTaskReport(props.report);

This can be used in shouldShow prop for FullPageNotFoundView.

This has advantage over withReportOrNotFound HOC. We can add custom message to show in two scenarios described above.
We can pass custom subtitleKey ( that shows why page can't be viewed, example - Can't edit the Completed Task) to FullPageNotFoundView.
It will make the end user understand why description is not changeable

We can also use early return in case the Task is completed and render nothing at all, with this approach we are not being verbose to user that why we are not showing the page.

UPDATE🚨
Many proposals are here mentioning about access check for user while opening these pages - It was supposed to be handle in this issue - #21580. But that was closed in favour of this issue (the changes will be added to this issue) and proposal(that was selected) is - #21580 (comment)

What alternative solutions did you explore? (Optional)

If we don't want to wrap the FullPageNotFoundView, we can use withReportOrNotFound here we can add additional condition to check if report is open or not.

report.stateNum === CONST.REPORT.STATE_NUM.OPEN && report.statusNum === CONST.REPORT.STATUS.OPEN;

By using this solution we can't provide detailed info to user why he/she can't access the page.

[twisterdotcom]

Wow, good find.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01df4c0b6543fe49a8

[melvin-bot]

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

[melvin-bot]

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

[Nodebrute]

Proposal

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

Web - User able to add or edit assignee even if task was completed

What is the root cause of that problem?

We are not adding any check on who can edit assignee.

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

First we need to add check if isCompletedTaskReport but what is task is not completed yet?
User shouldn't be able to access that page if Task is completed we have few other cases here too.
We can use Task.canModifyTask to stop users who aren't allowed to edit the task
Additionally we can also check for isCancelledTaskreport.

Task is not completed yet and users who are not allowed to edit assignee can access that page and can edit Task.
Only 3 users are allowed to edit task.
Task Assignee
Task Assigner
Policy Admin

We need to add other checks here too

!Task.isTaskAssigneeOrTaskOwner ---To check if current user is task assignee or task owner if not then user will not be able to access that page.
!PolicyUtils.isPolicyAdmin----To check if current user is policy admin if not then user will not be able to access that page
ReportUtils.isCompletedTaskReport---To check if task is completed then no one should be able to access that page.

After adding these checks we can either dismiss modal, show hmmm it's not here page or return early from editTaskAndNavigate.

What alternative solutions did you explore? (Optional)

We can do this for title and description pages too.

[dukenv0307]

Proposal

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

user able to add or edit assignee even if task was completed

What is the root cause of that problem?

We don't have any checks to dismiss task assignee modal or do nothing in editTaskAndNavigate if the task is completed.

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

After we change the permission, we can display not found page for TaskTitlePage, TaskDescriptionPage, and TaskAssigneeSelectorModal if we are go to the task pages in editing mode and the task is completed or the user cannot modify the task or the task is canceled

• We can create a function to check if the task can edited or not and then use this to display not found page

• For TaskAssigneeSelectorModal page, because the page is also used when we create a new task, so we should also check if we are open the page in a task report or not !_.isEmpty(report) && (ReportUtils.isCompletedTaskReport || ReportUtils.isCanceledTaskReport || !Task.canModifyTask)

• For TaskDescriptionPage and TaskTitlePage pages, we can simply add the check (ReportUtils.isCompletedTaskReport || ReportUtils.isCanceledTaskReport || !Task.canModifyTask) to display not found page

Because we display not found page for these page, we also should subscribe isLoadingReportData to display loading page if the data is loading when we open these page by deeplink before login to preventing not found page display briefly after logging or use withReportorNotFound HOC or create new HOC for task report to take care this.

What alternative solutions did you explore? (Optional)

We also can add the check ReportUtils.isCompletedTaskReport || !Task.canModifyTaskeditTaskAndNavigate` here to do nothing if the task is completed

if (ReportUtils.isCompletedTaskReport || !Task.canModifyTask) {
    return;
}

App/src/libs/actions/Task.js

Line 308 in 38890e2

function editTaskAndNavigate(report, ownerEmail, ownerAccountID, {title, description, assignee = '', assigneeAccountID = 0}) {

[twisterdotcom]

@eVoloshchak - any of these proposals look good?

[DanutGavrus]

Proposal

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

App allows task title update for completed task using deep link and throws error on update

What is the root cause of that problem?

We allow the Sub-Report deep link routes to be handled by their respective component pages, which means that users will be able to access pages which they shouldn't:

App/src/libs/ReportUtils.js

Lines 2292 to 2300 in 2d05d2e

	function getReportIDFromLink(url) {
	const route = getRouteFromLink(url);
	const {reportID, isSubReportPageRoute} = ROUTES.parseReportRouteParams(route);
	if (isSubReportPageRoute) {
	// We allow the Sub-Report deep link routes (settings, details, etc.) to be handled by their respective component pages
	return '';
	}
	return reportID;
	}

While coming from a deeplink, or manually entering the link in the search bar and press enter, the following happens: Report.openReportFromDeepLink(url, isAuthenticated) triggers -> const reportID = ReportUtils.getReportIDFromLink(url); triggers -> return ''; triggers which lets the openReportFromDeepLink continue with no reportID -> thus giving users access to those sub-pages.

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

I think we should prevent users from navigating to pages which they shouldn't be allowed to(such as navigating to /login while already logged in - a general example). To do that, we should:

1. Modify getReportIDFromLink as to return {reportID, isSubReportPageRoute} without setting it to '' before;
2. Update openReportFromDeepLink as follows:

let route = ReportUtils.getRouteFromLink(url);
let { reportID, isSubReportPageRoute } = ReportUtils.getReportIDFromLink(url);
if (isSubReportPageRoute) {
    // Prevent Sub-Report deep link to routes with no access
    const report = ReportUtils.getReport(reportID);
    if (ReportUtils.isCompletedTaskReport(report) && route.endsWith('/assignee')) {
        route = route.slice(0, -9);
    } else {
        // We allow the Sub-Report deep link to routes with access to be handled by their respective component pages
        reportID = ''
    }
}

Previous code is a draft, we should add more conditions in our if for access verification, add more sub-routes, replace the hard-coded sub-route string with consts, -9 with those consts' lengths, etc.
This is the result:

restrict_access.mp4

[twisterdotcom]

Bump @eVoloshchak. I'll reassign to another C+ tomorrow if you're a bit busy, don't worry.

[melvin-bot]

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

[eVoloshchak]

Thank you for the proposals, everyone!
The first four proposals are quite similar (they can't be that different, since all we need to do is conditionally display an error)

I think we should proceed with @Nodebrute's proposal, it is adding an important check if user has permission to edit/view the task title/description in addition to checking if task is completed.

We can use FullPageNotFoundView, there seem to be two cases:

1. The task is completed -> show FullPageNotFoundView with something like 'Can't edit task when it's completed'
2. User isn't Task Assignee, Task Assigner or Policy Admin (can't see or edit the task) -> show FullPageNotFoundView with 'You can't access this task'

🎀👀🎀 C+ reviewed!

UPD: there's already an issue User able to add or edit assignee even if task was completed, we would be resolving that too with this proposal

[melvin-bot]

📣 @dhanashree-sawant 🎉 An offer has been automatically sent to your Upwork account for the Reporter role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[BhuvaneshPatil]

Thank you @thienlnam .
Scope of this issue will be -

• Disable updating title and other fields in case of completed task using deep link
• Add extra checks as discussed in [HOLD 296932][$1000] Web - Member from Share Somewhere in assign task (creating from group) can choose assignee, results in error #21580

Please confirm.

[thienlnam]

Yup sounds good, thanks for sticking with these issues

[dukenv0307]

@thienlnam I think we are missing some case here.

1. With a valid reportID but it isn't the task report
2. Invalid reportID or cannot access report
3. Not found page should come with loading page.

Solution: use withReportOrNotFound HOC that can verify the report is vaild and control the loading as well. And add the check in task page to check the report is task report or not.

Can you check my comment #22451 (comment) and #22451 (comment)

And my proposal

[dhanashree-sawant]

Hi @thienlnam, melvin bot has provided me with 50$ offer even though it is raised in july 😅. Can you check once you are available?

[sophiepintoraetz]

@dhanashree-sawant - I've paid you for $250.

[melvin-bot]

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

• when @BhuvaneshPatil got assigned: 2023-09-01 18:15:30 Z
• when the PR got merged: 2023-09-13 01:55:01 UTC
• days elapsed: 7

On to the next one 🚀

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.69-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix: adds FullPageNotFoundView view for completed task #26634

If no regressions arise, payment will be issued on 2023-09-21. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @eVoloshchak does not require payment (Eligible for Manual Requests)
• @BhuvaneshPatil requires payment offer (Contributor)
• @dhanashree-sawant requires payment offer (Reporter)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

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:

• [@eVoloshchak] The PR that introduced the bug has been identified. Link to the PR:
• [@eVoloshchak] 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:
• [@eVoloshchak] 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:
• [@eVoloshchak] Determine if we should create a regression test for this bug.
• [@eVoloshchak] 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.
• [@twisterdotcom / @sophiepintoraetz] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[BhuvaneshPatil]

^^ @sophiepintoraetz @eVoloshchak @thienlnam

Can we proceed further on this.
Also the PR was approved before 3 day time period, but was put on freeze.

[sophiepintoraetz]

Cool so:

Payouts due:

Issue Reporter: $250 done
Contributor: $1500 @BhuvaneshPatil
Contributor+: $1500 @eVoloshchak (request a newdot payment once the BZ regression test checklist is done)

Eligible for 50% #urgency bonus? Y

[sophiepintoraetz]

@BhuvaneshPatil - for future reference, please wait until 7 days has passed before bumping - this has a weekly priority so while we endeavour to pay out on time, it's not always the case.

[BhuvaneshPatil]

thanks @sophiepintoraetz for mentioning.
I will keep that in mind from now.

[eVoloshchak]

• The PR that introduced the bug has been identified. Link to the PR: There isn't a PR that caused this, we've just added the logic that was missing. The whole task creation flow was implemented in Create Task FrontEnd Changes #17992, but at that point, this wasn't needed
• 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: N/A
• 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: I don't think an additional discussion is needed, we already have a checklist item to catch similar bugs: If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account

[eVoloshchak]

Regression Test Proposal

1. Create a task
2. copy URL of task for description https://staging.new.expensify.com/r//description
3. Complete that task(mark as done)
4. Paste the URL into the browser
5. Verify that the user is presented with full-page not found view
6. Do the same steps (4-5) for title page - https://staging.new.expensify.com/r//title
7. Do the same steps (4-5) for assignee page - https://staging.new.expensify.com/r//assignee

Do we agree 👍 or 👎

[eVoloshchak]

@sophiepintoraetz, please don't close an issue if the BZ checklist hasn't been completed yet
If the issue is closed, it isn't displayed in Assigned Issues section for C+, so there is no way to access it unless you specifically remember it exists (I stumbled upon this one randomly😅)
Thank you😊

[eVoloshchak]

Requested the payment via NewDot

[JmillsExpensify]

$1,500 payment approved for @eVoloshchak based on BZ summary.

[melvin-bot]

⚠️ 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.