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 2023-12-08] [$500] Web - App marks task as unread even when no changes are done #27296

Closed
1 of 6 tasks
kbecciv opened this issue Sep 12, 2023 · 92 comments
Closed
1 of 6 tasks
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 Internal Requires API changes or must be handled by Expensify staff

Comments

@kbecciv
Copy link

kbecciv commented Sep 12, 2023

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. Open task report or raise new task report using any user and any title
  3. Open Description
  4. Without making any changes, click save and observe that task is marked unread

Expected Result:

Task should not be marked as unread if there is no change to any field

Actual Result:

Task is marked as unread even if there is no change to any field

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

task.unread.even.on.no.change.mp4
Recording.4426.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~014012f6817510a0d1
  • Upwork Job ID: 1701705833501474816
  • Last Price Increase: 2023-10-03
Issue OwnerCurrent Issue Owner: @dylanexpensify
@kbecciv kbecciv 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 Sep 12, 2023
@melvin-bot melvin-bot bot changed the title Web - App marks task as unread even when no changes are done [$500] Web - App marks task as unread even when no changes are done Sep 12, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 12, 2023

Job added to Upwork: https://www.upwork.com/jobs/~014012f6817510a0d1

@melvin-bot
Copy link

melvin-bot bot commented Sep 12, 2023

Triggered auto assignment to @kevinksullivan (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 Sep 12, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 12, 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 Sep 12, 2023

Triggered auto assignment to @dylanexpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@melvin-bot
Copy link

melvin-bot bot commented Sep 12, 2023

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

@iamuddeshya
Copy link

iamuddeshya commented Sep 12, 2023

Proposal

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

Task on the sidebar is marked as unread even if the task is saved with no changes made.

What is the root cause of that problem?

The editTaskAndNavigate function is calling the API even when there is no change in the current data and the updated data of the task.

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

Before calling the EditTask API.write function, add a if condition that checks if there has been any change made in the previous state vs updated state of the Task. If not then avoid calling the API to update the task.

API.write(

What alternative solutions did you explore? (Optional)

Most apps disable the Save button until any change has been made. We can write a logic to keep the Save button disabled, and only enable it when there is a change made in the values of the task.

@melvin-bot
Copy link

melvin-bot bot commented Sep 12, 2023

📣 @iamuddeshya! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@iamuddeshya
Copy link

iamuddeshya commented Sep 12, 2023

Contributor details
Your Expensify account email: ua@hyathi.com
Upwork Profile Link: https://www.upwork.com/freelancers/~011b2c513d4b7b3536

@hoangzinh
Copy link
Contributor

hoangzinh commented Sep 12, 2023

Proposal

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

Web - App marks task as unread even when no changes are done

What is the root cause of that problem?

In the submit callback of the Edit description page, we always call the util to edit the Task without check the changes

const submit = useCallback(
(values) => {
// Set the description of the report in the store and then call Task.editTaskReport
// to update the description of the report on the server
Task.editTaskAndNavigate(props.report, props.session.accountID, {description: values.description});
},
[props],
);

After the API command EditTask done, it updates the lastVisibleActionCreated of the report, which will make the report has unread messages
Screenshot 2023-09-13 at 06 41 01

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

  • In this callback, we should check if description value is changed before call util to call API command EditTask

    const submit = useCallback(
    (values) => {
    // Set the description of the report in the store and then call Task.editTaskReport
    // to update the description of the report on the server
    Task.editTaskAndNavigate(props.report, props.session.accountID, {description: values.description});
    },
    [props],
    );

    Otherwise, we should only call Navigation.dismissModal to close the RHN.

  • We can apply this logic for the Edit title page as well.

@iamuddeshya
Copy link

Contributor details
Your Expensify account email: ua@hyathi.com
Upwork Profile Link: https://www.upwork.com/freelancers/~011b2c513d4b7b3536

@melvin-bot
Copy link

melvin-bot bot commented Sep 13, 2023

⚠️ Missing/invalid email or upwork profile link. Please make sure you add both your Expensify email and Upwork profile link in the format specified.

@rayane-djouah
Copy link
Contributor

rayane-djouah commented Sep 13, 2023

Proposal

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

The task report is marked as unread even when no changes are made to the title, description, or assignee fields in the TaskTitlePage, TaskDescriptionPage, and TaskAssigneeSelectorModal components of the application. This behavior is unexpected as tasks should only be marked as unread and the API should only be called when there are actual changes.

What is the root cause of that problem?

In the submit callback of the TaskTitlePage and TaskDescriptionPage components, we consistently invoke the utility function Task.editTaskAndNavigate to modify the Task without verifying whether any changes have been made.
For TaskTitlePage:

const submit = useCallback(
(values) => {
// Set the title of the report in the store and then call Task.editTaskReport
// to update the title of the report on the server
Task.editTaskAndNavigate(props.report, props.session.accountID, {title: values.title});
},
[props],
);

For TaskDescriptionPage:
const submit = useCallback(
(values) => {
// Set the description of the report in the store and then call Task.editTaskReport
// to update the description of the report on the server
Task.editTaskAndNavigate(props.report, props.session.accountID, {description: values.description});
},
[props],
);

In the selectReport callback of the TaskAssigneeSelectorModal component, we consistently invoke the utility function Task.editTaskAssigneeAndNavigate to modify the Task without verifying whether any changes have been made.
For TaskAssigneeSelectorModal:
const selectReport = (option) => {
if (!option) {
return;
}
// Check to see if we're creating a new task
// If there's no route params, we're creating a new task
if (!props.route.params && option.accountID) {
// Clear out the state value, set the assignee and navigate back to the NewTaskPage
setSearchValue('');
Task.setAssigneeValue(option.login, option.accountID, props.task.shareDestination, OptionsListUtils.isCurrentUser(option));
return Navigation.goBack(ROUTES.NEW_TASK);
}
// Check to see if we're editing a task and if so, update the assignee
if (report) {
const assigneeChatReport = Task.setAssigneeValue(option.login, option.accountID, props.route.params.reportID, OptionsListUtils.isCurrentUser(option));
// Pass through the selected assignee
Task.editTaskAssigneeAndNavigate(report, props.session.accountID, option.login, option.accountID, assigneeChatReport);
}
};

Following the completion of the EditTask API command in the Task.editTaskAndNavigate util function and EditTaskAssignee API command in the Task.editTaskAssigneeAndNavigate util function, it modifies the task report action created property and the lastVisibleActionCreated property of the report. This alteration results in the report being flagged as unread.

API.write(
'EditTask',
{

API.write(
'EditTaskAssignee',
{

created: DateUtils.getDBTime(),

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

We should check if the field value is changed before calling the utility function.

For TaskTitlePage:

if (values.title !== props.report.reportName) {
    Task.editTaskAndNavigate(props.report, props.session.accountID, {title: values.title});
} else {
    Navigation.dismissModal(props.report.reportID);
}

For TaskDescriptionPage:

if (values.description !== props.report.description) {
    Task.editTaskAndNavigate(props.report, props.session.accountID, {description: values.description});
} else {
    Navigation.dismissModal(props.report.reportID);
}

For TaskAssigneeSelectorModal:

if (option.accountID !== props.task.assigneeAccountID) {
    Task.editTaskAssigneeAndNavigate(report, props.session.accountID, option.login, option.accountID, assigneeChatReport);
} else {
    Navigation.dismissModal(report.reportID);
}

Result:

2023-09-13.01-48-55.mp4

What alternative solutions did you explore? (Optional)

check if there are any changes before calling the EditTask API command in the Task.editTaskAndNavigate util function and EditTaskAssignee API command in the Task.editTaskAssigneeAndNavigate util function.

@tienifr
Copy link
Contributor

tienifr commented Sep 13, 2023

Proposal

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

I think the issue Task is marked as unread even if there is no change to any field is not entirely correct.

The Task edited actions are non-visible actions (as clearly indicated in the code here), so the edit action should not affect the chat read/unread status at all (even if there's change to a field). We can also see that even as the chat is marked as unread as in the video, it still shows No activity yet.

In order for an action to make the report as unread, it should at least:

  • Are visible action, show in the chat
  • Affect the "last message" in LHN
  • Aside from other conditions of unread report

So the correct issue here should be Task report is marked as unread if the user edits the task, which is invisible action

What is the root cause of that problem?

  1. The problem is in the EditTask API, the back-end is returning updated lastVisibleActionCreated which is not correct since the EditTask action is not "visible action".

  2. There's another issue where even though the field is not updated (has the same value), we still call EditTask API and mark the pendingAction. This causes redundant API call and cause the task to show pending UX while offline.

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

  1. We should fix in the BE not to update the lastVisibleActionCreated in the EditTask API
  2. We should not call editTaskAndNavigate when changing task title, description, assignee, if the value does not change. That can be done for example in here (for the description, similar for other fields), if the description did not change then we simply call Navigation.dismissModal.
if (props.report.description === values.description) {
    Navigation.dismissModal();
    return;
}

Another way for this is to check if anything changes in editTaskAndNavigate, if not then just navigate.

What alternative solutions did you explore? (Optional)

For 1, if instead we want to make the Task edit report actions to become visible actions, then we need to:

  • Display the "changes from ... to ...` properly in the report when something changes in the Task
  • Make sure the TASKEDITED report action shows as last message in LHN, if it's the last report action
  • Remove this condition
  • There might be other changes as well since this direction is more like a feature request

@dylanexpensify
Copy link
Contributor

Agree this seems like a bug, but confirming first internally

@melvin-bot melvin-bot bot added the Overdue label Sep 18, 2023
@sobitneupane
Copy link
Contributor

@dylanexpensify I'm uncertain about the rationale behind marking it as 'unread' initially for the user initiating the change. In my opinion, it would be more logical to designate the task report as 'unread' solely for users other than the one initiating the change.

@melvin-bot melvin-bot bot removed the Overdue label Sep 18, 2023
@tienifr
Copy link
Contributor

tienifr commented Sep 18, 2023

@sobitneupane can you take a look at the Problem part of my proposal, IMO there should be no "unread" at all since the Task edited actions are non-visible actions

cc @dylanexpensify

@rayane-djouah
Copy link
Contributor

@sobitneupane in transactions threads we are marking the report as 'unread' for the user initiating the change, so I think it's an expected behaviour.

2023-09-18.17-22-39.mp4

and when no changes done we don't mark the report as unread

2023-09-18.17-23-52.mp4

@rayane-djouah
Copy link
Contributor

rayane-djouah commented Sep 18, 2023

@sobitneupane can you take a look at the Problem part of my proposal, IMO there should be no "unread" at all since the Task edited actions are non-visible actions

cc @dylanexpensify

In my opinion, from a user experience perspective, we should mark the report as 'unread' to notify users about the updates (in the case where there are actual changes) even if the EditTask action is not a visible action.

@tienifr
Copy link
Contributor

tienifr commented Sep 18, 2023

@rayane-djouah that will only confuses the users since the LHN is saying No activity yet and there's unread UI. Then when clicking on the report the user doesn't see any message. "Invisible" means it should not impact the report in a way a "visible" action does 😄

@melvin-bot
Copy link

melvin-bot bot commented Sep 19, 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 Daily KSv2 and removed Weekly KSv2 labels Dec 8, 2023
@sobitneupane sobitneupane mentioned this issue Dec 8, 2023
63 tasks
@sobitneupane
Copy link
Contributor

sobitneupane commented Dec 8, 2023

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:

  • The PR that introduced the bug has been identified. Link to the PR:

#18672

  • 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:

#18672 (comment)

  • 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:
  • Determine if we should create a regression test for this bug.

Yes

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

#27296 (comment)

@sobitneupane
Copy link
Contributor

Regression Test Proposal:

  1. Open an existing task report or create a new task report
  2. Press on Task description
  3. Save the description without making any change
  4. Verify that the Task report is not marked as unread in LHN.
  5. Repeat Step 2, 3 and 4 for Task title and Task assignee.

Do we agree 👍 or 👎

@melvin-bot melvin-bot bot added the Overdue label Dec 11, 2023
@dylanexpensify
Copy link
Contributor

Payment summary:

Please apply!

@dylanexpensify
Copy link
Contributor

New Upwork Job!

@dylanexpensify
Copy link
Contributor

Please apply! ^

@dylanexpensify
Copy link
Contributor

melvin! 🙄

@melvin-bot melvin-bot bot removed the Overdue label Dec 11, 2023
@hoangzinh
Copy link
Contributor

Applied. Thanks @dylanexpensify

@dylanexpensify
Copy link
Contributor

Bump @dhanashree-sawant!

@dhanashree-sawant
Copy link

Sorry for the delay, I have applied to the job

@sobitneupane
Copy link
Contributor

#27296 (comment)

Requested payment on newDot.

@melvin-bot melvin-bot bot added the Overdue label Dec 15, 2023
@JmillsExpensify
Copy link

$500 payment approved for @sobitneupane based on this comment.

Copy link

melvin-bot bot commented Dec 18, 2023

@hoangzinh, @sobitneupane, @stitesExpensify, @dylanexpensify Eep! 4 days overdue now. Issues have feelings too...

@dylanexpensify
Copy link
Contributor

nice! Will pay out reporter then close!

@melvin-bot melvin-bot bot removed the Overdue label Dec 20, 2023
@dylanexpensify
Copy link
Contributor

@dhanashree-sawant offer sent!

@melvin-bot melvin-bot bot added the Overdue label Dec 25, 2023
Copy link

melvin-bot bot commented Dec 26, 2023

@hoangzinh, @sobitneupane, @stitesExpensify, @dylanexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@dhanashree-sawant
Copy link

Thanks @dylanexpensify, I have accepted the offer.

Copy link

melvin-bot bot commented Dec 28, 2023

@hoangzinh, @sobitneupane, @stitesExpensify, @dylanexpensify Huh... This is 4 days overdue. Who can take care of this?

Copy link

melvin-bot bot commented Jan 1, 2024

@hoangzinh, @sobitneupane, @stitesExpensify, @dylanexpensify 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

Copy link

melvin-bot bot commented Jan 2, 2024

@hoangzinh, @sobitneupane, @stitesExpensify, @dylanexpensify 10 days overdue. I'm getting more depressed than Marvin.

@dylanexpensify
Copy link
Contributor

done!

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 Internal Requires API changes or must be handled by Expensify staff
Projects
None yet
Development

No branches or pull requests