fix: update parent action for assignee report as well #24110

s-alves10 · 2023-08-03T13:39:38Z

Details

Optimistically update parent action in assignee report as well

Fixed Issues

$ #23920
PROPOSAL: #23920 (comment)

Tests

Create a group with user A and user B
Create a task for user A or user B shared in the group created at step 1
Go to the group and update the task using checkbox
Verify that replies count updates
Go to the assignee report and update the task using checkbox
Verify that replies count updates

Verify that no errors appear in the JS console

Offline tests

Create a group with user A and user B
Create a task for user A or user B shared in the group created at step 1
Go to the group and update the task using checkbox
Verify that replies count updates
Go to the assignee report and update the task using checkbox
Verify that replies count updates

QA Steps

Create a group with user A and user B
Create a task for user A or user B shared in the group created at step 1
Go to the group and update the task using checkbox
Verify that replies count updates
Go to the assignee report and update the task using checkbox
Verify that replies count updates

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

23920_mac_chrome.mp4

Mobile Web - Chrome

23920_android_chrome.mp4

Mobile Web - Safari

23920_ios_safari.mp4

Desktop

23920_mac_desktop.mp4

iOS

23920_ios_native.mp4

Android

23920_android_native.mp4

…eport action

melvin-bot · 2023-08-03T13:50:00Z

@s77rt Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

s-alves10 · 2023-08-03T19:52:12Z

@s77rt
Please take a look

src/components/ReportActionItem/TaskPreview.js

src/libs/ReportActionsUtils.js

src/pages/home/HeaderView.js

src/libs/actions/Task.js

src/components/ParentNavigationSubtitle.js

src/libs/actions/Task.js

src/libs/ReportActionsUtils.js

src/libs/actions/Task.js

…shareDestination reports

s-alves10 · 2023-08-04T13:24:45Z

@s77rt

Please take a look again. Thanks

src/components/ParentNavigationSubtitle.js

src/libs/ReportActionsUtils.js

src/libs/actions/Task.js

s77rt · 2023-08-04T23:53:50Z

@s-alves10 Some of the previous comments are not resolved but marked as resolved. Please double check.

s-alves10 · 2023-08-05T02:10:10Z

@s77rt

I fixed all you mentioned.

Here is my concern. Clearly parentReportID and childReportID are number and reportID is string. It may cause an issue

s77rt · 2023-08-05T05:56:44Z

@s-alves10 Let's write the code as if they are both strings. Parent report ids being returned as a number is a bug https://expensify.slack.com/archives/C01GTK53T8Q/p1691167905181059?thread_ts=1691167905.181059&cid=C01GTK53T8Q

s-alves10 · 2023-08-05T10:12:22Z

src/libs/ReportActionsUtils.js

+ * @returns {Object}
+ */
+function getParentReportActionForTask(taskReportID, reportID) {
+    return _.find(allReportActions[reportID], (reportAction) => reportAction && `${reportAction.childReportID}` === `${taskReportID}`);


@s-alves10 Let's write the code as if they are both strings. Parent report ids being returned as a number is a bug https://expensify.slack.com/archives/C01GTK53T8Q/p1691167905181059?thread_ts=1691167905.181059&cid=C01GTK53T8Q

@s77rt

Got it. But if we don't convert them to string, comparison would return false and can't find the parent action in assignee report. childReportID is a number as well.

I think we need to convert them to string unless the backend is fixed. What do you think?

Indeed. We may need to wait for an outcome from that slack thread. But for now let's write the right code.

s77rt · 2023-08-06T14:36:40Z

@s-alves10 Can you please avoid resolving any PR comment until you see a thumbs up or a confirmation that it can be resolved.

s77rt

Overall the logic looks good. I think we may just need to make the code more readable. (will add another comment)

src/components/ReportActionItem/TaskPreview.js

s77rt · 2023-08-06T14:59:21Z

src/libs/ReportActionsUtils.js

+ * @param {String} reportID
+ * @returns {Object}
+ */
+function getParentReportActionForTask(taskReportID, reportID) {


This function is not exclusive to tasks. Can we rename it to something else?

Renamed to getParentReportActionInReport

s77rt · 2023-08-06T16:30:05Z

@s-alves10 Can we re-write the getOptimisticDataForParentReportAction function as follow:

const getOptimisticDataForParentReportAction = (reportID, lastVisibleActionCreated, type, parentReportID = 0, parentReportActionID = 0) => {
    const report = getReport(reportID);
    const parentReportAction = ReportActionsUtils.getParentReportAction(report);

    if (_.isEmpty(parentReportAction)) {
        return {};
    }

    const optimisticParentReportAction = updateOptimisticParentReportAction(parentReportAction, lastVisibleActionCreated, type);
    
    if (!parentReportID) {
        parentReportID = report.parentReportID;
    }

    if (!parentReportActionID) {
        parentReportActionID = report.parentReportActionID;
    }

    return {
        onyxMethod: Onyx.METHOD.MERGE,
        key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${parentReportID}`,
        value: {
            [parentReportActionID]: optimisticParentReportAction,
        },
    };
};

Then in Task.js we would just call this function twice, first time as we did previously

const optimisticDataForParentReportAction = ReportUtils.getOptimisticDataForParentReportAction(taskReportID, completedTaskReportAction.created, CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD);

and the second time with the collected assignee report id (using getChatByParticipants) and the collected report action id (using getParentReportActionForTask)

const optimisticDataForClonedParentReportAction = ReportUtils.getOptimisticDataForParentReportAction(taskReportID, completedTaskReportAction.created, CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD, assigneeChatReportID, clonedParentReportActionID);

s-alves10 · 2023-08-07T02:29:40Z

@s77rt

I've updated the code according to your change requests with one exception. I cannot remove string construction because that makes the solution not working

s77rt · 2023-08-07T12:15:32Z

@s-alves10 This comment is still unresolved #24110 (comment) Please check

s-alves10 · 2023-08-07T12:51:30Z

@s-alves10 This comment is still unresolved #24110 (comment) Please check

Done

s77rt

just few non-blocking comments. will proceed with tests asap

src/libs/ReportUtils.js

src/libs/actions/Task.js

src/libs/ReportUtils.js

s-alves10 · 2023-08-07T13:24:13Z

@s77rt

Updated according to your suggestions

s77rt · 2023-08-07T14:21:37Z

@s-alves10 I don't see any update, did you forget to push the changes maybe?

s-alves10 · 2023-08-07T14:28:43Z

@s77rt
I'm sorry. Can you see the updates now?

s77rt · 2023-08-07T15:51:45Z

src/libs/ReportUtils.js

+ * @returns {Object}
+ */
+function getTaskParentReportActionIDInAssigneeReport(taskReport) {
+    const assigneeChatReportID = lodashGet(getChatByParticipants(taskReport.participantAccountIDs), 'reportID');


Suggested change

const assigneeChatReportID = lodashGet(getChatByParticipants(taskReport.participantAccountIDs), 'reportID');

const assigneeChatReportID = lodashGet(getChatByParticipants(isTaskAssignee(taskReport) ? [taskReport.ownerAccountID] : [taskReport.managerID]), 'reportID');

I think we have a case that we are not covering, if you are assigned a task by someone else and click the checkbox in the assignee report, nothing will happen. This is because getChatByParticipants will look for a chat that have the assignee as a participant but since you are the assignee there won't be a chat with your self, instead there will be a chat the owner.

Kooha-2023-08-07-16-43-34.mp4

@s77rt
I think we don't need to find assignee report in this case because there is no assignee report to update for the assignee himself. In our solution, getChatByParticipants returns null in this case and would work as expected.
Please tell me if I missed something

because there is no assignee report to update for the assignee himself

There is an assignee report, it's shown in the above video

@s77rt

Great catch. Code updated.
Thank you

src/libs/ReportUtils.js

s77rt · 2023-08-07T17:54:24Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
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.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

web.mp4

Mobile Web - Chrome

mweb-chrome.mp4

Mobile Web - Safari

mweb-safari.mp4

Desktop

desktop.mp4

iOS

ios.mp4

Android

android.mp4

s77rt

LGTM! 🚀

s-alves10 · 2023-08-08T06:46:42Z

@techievivek
Can you take a look at this PR?

techievivek · 2023-08-08T07:09:43Z

@s-alves10 Yes, I am taking a look, I have a few questions, can you please answer them in realtime? Thanks

techievivek

Not very familiar with the data structure behind a thread/task so I have a few quick question to understand the changes.

src/libs/actions/Task.js

s-alves10 · 2023-08-08T07:10:40Z

@techievivek

yes

src/components/ReportActionItem/TaskPreview.js

techievivek

Great work 🚀

OSBotify · 2023-08-08T07:47:15Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2023-08-09T09:11:38Z

🚀 Deployed to staging by https://github.com/techievivek in version: 1.3.52-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	failure ❌
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2023-08-09T09:50:21Z

🚀 Deployed to staging by https://github.com/techievivek in version: 1.3.52-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2023-08-10T20:08:47Z

🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.52-5 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

eVoloshchak · 2023-10-16T08:38:05Z

src/libs/actions/Task.js

-    const optimisticParentReportData = ReportUtils.getOptimisticDataForParentReportAction(taskReportID, reopenedTaskReportAction.created, CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD);
-    if (!_.isEmpty(optimisticParentReportData)) {
-        optimisticData.push(optimisticParentReportData);
+    const optimisticDataForParentReportAction = ReportUtils.getOptimisticDataForParentReportAction(taskReportID, reopenedTaskReportAction.created, CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD);


Coming from #25548
This PR didn't account for the case where a reportAction has more than one parent (i.e. if you change the asignee of the task), so only the first one was updated
More details in #25548 (comment)

@eVoloshchak I didn't read the whole issue context but isn't the approved PR reintroducing the fixed bug here #23920?

@s77rt, we aren't counting completing/opening a task as a reply anymore (recent BE update), so this logic isn't needed
I should have included that actually, so thank you

sangar-1028 added 3 commits August 3, 2023 06:31

fix: update parent action for assignee report as well

313d2e5

fix: optimistic data 'participantAccountIDs' in task report, not in r…

9728aa2

…eport action

fix: console warning

574c144

s-alves10 marked this pull request as ready for review August 3, 2023 13:49

s-alves10 requested a review from a team as a code owner August 3, 2023 13:49

melvin-bot bot requested review from s77rt and removed request for a team August 3, 2023 13:49

s-alves10 mentioned this pull request Aug 3, 2023

[HOLD for payment 2023-08-17] [$1000] Replies count doesn't update on a task shared in group. #23920

Closed

6 tasks

fix: conflicts

24a3870

s77rt suggested changes Aug 3, 2023

View reviewed changes

sangar-1028 added 2 commits August 3, 2023 18:39

fix: add comments, remove failureData for assignee report action

1e380ae

fix: prettier

abff057

s77rt suggested changes Aug 4, 2023

View reviewed changes

fix: use of updateOptimisticParentReportAction for both assignee and …

a5eac40

…shareDestination reports

s-alves10 requested a review from s77rt August 4, 2023 04:21

fix: prettier

688217f

s77rt suggested changes Aug 4, 2023

View reviewed changes

sangar-1028 added 2 commits August 4, 2023 20:59

fix: rnaming, comments

d4dd964

fix: comments, string constructor

fbed766

fix: prettier

6a977fd

s-alves10 commented Aug 5, 2023

View reviewed changes

s77rt suggested changes Aug 6, 2023

View reviewed changes

fix: reopen/complete task in HeaderView

a5f4dfb

s77rt suggested changes Aug 7, 2023

View reviewed changes

src/libs/ReportUtils.js Outdated Show resolved Hide resolved

src/libs/ReportUtils.js Outdated Show resolved Hide resolved

src/libs/ReportUtils.js Outdated Show resolved Hide resolved

src/libs/actions/Task.js Show resolved Hide resolved

src/libs/ReportUtils.js Show resolved Hide resolved

fix: comments and function name

2e3f28e

s77rt suggested changes Aug 7, 2023

View reviewed changes

fix: uncovered case for report with task owner when log in with assignee

5152ef3

s77rt approved these changes Aug 7, 2023

View reviewed changes

melvin-bot bot requested a review from techievivek August 7, 2023 17:55

techievivek reviewed Aug 8, 2023

View reviewed changes

src/libs/actions/Task.js Show resolved Hide resolved

src/libs/actions/Task.js Show resolved Hide resolved

techievivek reviewed Aug 8, 2023

View reviewed changes

src/components/ReportActionItem/TaskPreview.js Show resolved Hide resolved

techievivek approved these changes Aug 8, 2023

View reviewed changes

techievivek merged commit fdcae9f into Expensify:main Aug 8, 2023

OSBotify mentioned this pull request Aug 9, 2023

Deploy Checklist: New Expensify 2023-08-09 #24307

Closed

87 tasks

This was referenced Aug 20, 2023

[C+ checklist needs completion] [$1000] Upon checking the task checkbox, the reply is only displayed in the first task report #25548

Closed

WOWO esh-g/plantonic#8

Open

eVoloshchak reviewed Oct 16, 2023

View reviewed changes

s-alves10 deleted the fix/issue-23920 branch November 22, 2023 16:44

	const assigneeChatReportID = lodashGet(getChatByParticipants(taskReport.participantAccountIDs), 'reportID');
	const assigneeChatReportID = lodashGet(getChatByParticipants(isTaskAssignee(taskReport) ? [taskReport.ownerAccountID] : [taskReport.managerID]), 'reportID');

fix: update parent action for assignee report as well #24110

fix: update parent action for assignee report as well #24110

Conversation

s-alves10 commented Aug 3, 2023

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Aug 3, 2023

s-alves10 commented Aug 3, 2023

s-alves10 commented Aug 4, 2023

s77rt commented Aug 4, 2023

s-alves10 commented Aug 5, 2023 • edited Loading

s77rt commented Aug 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s77rt commented Aug 6, 2023

s77rt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s77rt commented Aug 6, 2023

s-alves10 commented Aug 7, 2023

s77rt commented Aug 7, 2023

s-alves10 commented Aug 7, 2023

s77rt left a comment

Choose a reason for hiding this comment

s-alves10 commented Aug 7, 2023

s77rt commented Aug 7, 2023

s-alves10 commented Aug 7, 2023

s77rt Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

s-alves10 Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s77rt commented Aug 7, 2023

Reviewer Checklist

Screenshots/Videos

s77rt left a comment

Choose a reason for hiding this comment

s-alves10 commented Aug 8, 2023

techievivek commented Aug 8, 2023

techievivek left a comment

Choose a reason for hiding this comment

s-alves10 commented Aug 8, 2023

techievivek left a comment

Choose a reason for hiding this comment

OSBotify commented Aug 8, 2023

OSBotify commented Aug 9, 2023

OSBotify commented Aug 9, 2023

OSBotify commented Aug 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s-alves10 commented Aug 5, 2023 •

edited

Loading

s77rt Aug 7, 2023 •

edited

Loading

s-alves10 Aug 7, 2023 •

edited

Loading