Move task reportActions onto the task report #20014

thienlnam · 2023-06-01T23:40:03Z

Details

Fixes this deploy blocker #20285
Also a fix for patching the difference between reportActions showing up on the task report instead of the parent report. The back-end changes are already merged so this should get resolved as well

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/285951
PROPOSAL: GH_LINK_ISSUE(COMMENT)

Tests

Create a task
Complete the task
Verify that the 'complete task' reportAction now shows up within the task report
Reopen the task, verify it shows up within the task report
Cancel the task, verify it shows up within the task report

Verify that no errors appear in the JS console

Offline tests

Same as tests

QA Steps

Verify that no errors appear in the JS console

PR Author Checklist

I linked the correct issue in the ### Fixed Issues section above
I wrote clear testing steps that cover the changes made in this PR
- I added steps for local testing in the Tests section
- I added steps for the expected offline behavior in the Offline steps section
- I added steps for Staging and/or Production testing in the QA steps section
- I added steps to cover 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 tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
I included screenshots or videos for tests on all platforms
I ran the tests on all platforms & verified they passed on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
I followed proper code patterns (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
  - If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
- 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 followed the guidelines as stated in the Review Guidelines
I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
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
I verified that if a function's arguments changed that all usages have also been updated correctly
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 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 author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Screen.Recording.2023-06-01.at.4.57.02.PM.mov

Mobile Web - Chrome

Screen.Recording.2023-06-01.at.4.58.11.PM.mov

Mobile Web - Safari

Desktop

Screen.Recording.2023-06-01.at.5.00.26.PM.mov

iOS

Android

thienlnam · 2023-06-02T00:01:44Z

Holding on https://github.com/Expensify/Auth/pull/7999

melvin-bot · 2023-06-02T17:07:10Z

@mollfpr @hayata-suenaga One of you needs to 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]

thienlnam · 2023-06-02T17:07:34Z

Could I please get an initial code review here? These are holding on the backend changes https://github.com/Expensify/Auth/pull/7999 which are ready but I don't want to send those out until this App PR is also ready to send out

mollfpr · 2023-06-02T17:18:40Z

The code looks good to me, but I didn't get the time to test it. I'll try it and review it again in the morning.

thienlnam · 2023-06-02T17:21:51Z

Thanks - you might have to do offline tests for now since the back-end will still save the messages on the parent report. Once we confirm that generally works, I will merge the BE PR and you can do a full run through with online tests

hayata-suenaga · 2023-06-04T01:49:35Z

@thienlnam

we don't have to put hold on the title?

hayata-suenaga · 2023-06-04T01:56:22Z

@thienlnam

are we also moving the task actions already in the database from parent report to the task report?

hayata-suenaga

As I also mentioned in the previous comments, I don't know how PRs related to changes in the data schema are coordinated.

but the App code LGTM! I haven't tested this PR so let's wait for @mollfpr to complete the tests 🙇

mollfpr · 2023-06-05T15:05:02Z

@thienlnam @hayata-suenaga It tests well for offline.

thienlnam · 2023-06-05T15:08:51Z

Cool cool, merging the auth PR, will let you know when it's live

thienlnam · 2023-06-05T15:09:50Z

@hayata-suenaga Yup! I just merged the auth PR, and added hold to title until it's live

mollfpr

All yours @hayata-suenaga

melvin-bot · 2023-06-06T14:22:39Z

🎯 @mollfpr, thanks for reviewing and testing this PR! 🎉

An E/App issue has been created to issue payment here: #20270.

thienlnam · 2023-06-06T14:25:16Z

Just added one small line change to also resolve this bug

Note the issue is in the latest main. The checked state does not change while toggling complete from the thread and reopening the task.

mollfpr · 2023-06-06T14:43:12Z

Thanks @thienlnam for the fix! I didn't expect we could fix the issue in this PR 😅

Just need to resolve the failing lint, and I will approve it again.

thienlnam · 2023-06-06T14:44:55Z

heh yeah just a small change luckily - updated!

thienlnam · 2023-06-06T17:11:57Z

cc @hayata-suenaga Could you please approve/merge when you get the chance?

github-actions · 2023-06-06T17:28:28Z

⚠️ ⚠️ Heads up! This pull request has the CP Staging label ⚠️ ⚠️
If you applied the CP Staging label before the PR was merged, the PR will be be immediately deployed to staging even if the open StagingDeployCash deploy checklist is locked.
However if you applied the CP Staging after the PR was merged it's possible it won't be CP'ed automatically. If you need it to be CP'ed to staging, tag a member of @Expensify/mobile-deployers to CP it manually, otherwise you can wait for it to go out with the next deploy.

neil-marcellini

Looks good and tests well thanks!

neil-marcellini · 2023-06-06T17:37:27Z

src/libs/actions/Task.js

-function completeTask(taskReportID, parentReportID, taskTitle) {
-    const message = `Completed task: ${taskTitle}`;
+function completeTask(taskReportID, taskTitle) {
+    const message = `completed task: ${taskTitle}`;


NAB: I preferred sentence case but I guess we had a good reason to change it?

Yup, keeping it consistent with manual requests

Move task reportActions onto the task report (cherry picked from commit a61b74a)

…-20014 🍒 Cherry pick PR #20014 to staging 🍒

OSBotify · 2023-06-15T01:25:15Z

🚀 Cherry-picked to staging by https://github.com/AndrewGable in version: 1.3.28-2 🚀

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

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

OSBotify · 2023-06-16T18:43:04Z

🚀 Deployed to production by https://github.com/AndrewGable in version: 1.3.28-5 🚀

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

narefyev91 · 2023-07-13T06:36:11Z

src/components/ReportActionItem/TaskPreview.js

-    const isTaskCompleted =
-        (props.taskReport.stateNum === CONST.REPORT.STATE_NUM.SUBMITTED && props.taskReport.statusNum === CONST.REPORT.STATUS.APPROVED) ||
-        (props.action.childStateNum === CONST.REPORT.STATE_NUM.SUBMITTED && props.action.childStatusNum === CONST.REPORT.STATUS.APPROVED);
+    const isTaskCompleted = props.taskReport


#21924 (comment) issue happened using props.taskReport

s77rt · 2023-08-13T00:42:22Z

src/libs/actions/Task.js

-            key: `${ONYXKEYS.COLLECTION.REPORT}${parentReportID}`,
-            value: {
-                lastVisibleActionCreated: completedTaskReportAction.created,
-                lastMessageText: message,
-                lastActorEmail: completedTaskReportAction.actorEmail,
-            },
-        },
-        {
-            onyxMethod: Onyx.METHOD.MERGE,
-            key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${parentReportID}`,


The task report is a child, whenever it's updated we should also update the parent.

thienlnam added 7 commits June 1, 2023 15:33

move completeTask

7985bcd

update the task action so that it does not redirect to existing link

9cb8051

update copy so it is uncapitalized

3474701

update reopenTask

9df0edb

cancel task

859c002

fix taskReportID

22a4177

Update Task.js

1f26a5c

thienlnam self-assigned this Jun 1, 2023

remove lint

9626322

thienlnam marked this pull request as ready for review June 2, 2023 17:06

thienlnam requested a review from a team as a code owner June 2, 2023 17:06

melvin-bot bot requested review from hayata-suenaga and mollfpr and removed request for a team June 2, 2023 17:07

thienlnam added 2 commits June 2, 2023 10:08

Merge branch 'main' into jack-moveReportActions

1dec3d0

lint

95978db

hayata-suenaga previously approved these changes Jun 4, 2023

View reviewed changes

thienlnam changed the title ~~Move task reportActions onto the task report~~ [HOLD 7999] Move task reportActions onto the task report Jun 5, 2023

thienlnam changed the title ~~[HOLD 7999] Move task reportActions onto the task report~~ Move task reportActions onto the task report Jun 5, 2023

Merge branch 'main' into jack-moveReportActions

8ac7e15

mollfpr previously approved these changes Jun 6, 2023

View reviewed changes

melvin-bot bot mentioned this pull request Jun 6, 2023

23 June: Payment issue for E/App PR#20014 #20270

Closed

fix preview not updating

7589989

thienlnam dismissed mollfpr’s stale review via 7589989 June 6, 2023 14:24

linter

8933e76

mollfpr approved these changes Jun 6, 2023

View reviewed changes

thienlnam mentioned this pull request Jun 6, 2023

Task - Task is not unchecked when marked as incomplete #20285

Closed

6 tasks

neil-marcellini self-requested a review June 6, 2023 17:28

thienlnam added the CP Staging label Jun 6, 2023

neil-marcellini approved these changes Jun 6, 2023

View reviewed changes

neil-marcellini merged commit a61b74a into main Jun 6, 2023

neil-marcellini deleted the jack-moveReportActions branch June 6, 2023 17:43

OSBotify pushed a commit that referenced this pull request Jun 6, 2023

Merge pull request #20014 from Expensify/jack-moveReportActions

ffe3bfe

Move task reportActions onto the task report (cherry picked from commit a61b74a)

This was referenced Jun 6, 2023

🍒 Cherry pick PR #20014 to staging 🍒 #20295

Merged

Deploy Checklist: New Expensify 2023-06-05 #20195

Closed

neil-marcellini added a commit that referenced this pull request Jun 6, 2023

Merge pull request #20295 from Expensify/OSBotify-cherry-pick-staging…

6a906b8

…-20014 🍒 Cherry pick PR #20014 to staging 🍒

OSBotify mentioned this pull request Jun 15, 2023

Deploy Checklist: New Expensify 2023-06-14 #20782

Closed

jjcoffee mentioned this pull request Jun 26, 2023

[HELD ON #19298] \[$1000] Unchecking done assignment is not reflected on check-mark #19153

Closed

6 tasks

narefyev91 reviewed Jul 13, 2023

View reviewed changes

narefyev91 mentioned this pull request Jul 13, 2023

[HOLD for payment 2023-07-14] [$1000] Task checkbox state incorrect for unassigned user in chat #21924

Closed

6 tasks

s77rt reviewed Aug 13, 2023

View reviewed changes

s77rt mentioned this pull request Aug 13, 2023

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

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move task reportActions onto the task report #20014

Move task reportActions onto the task report #20014

thienlnam commented Jun 1, 2023 •

edited

Loading

thienlnam commented Jun 2, 2023

melvin-bot bot commented Jun 2, 2023

thienlnam commented Jun 2, 2023

mollfpr commented Jun 2, 2023

thienlnam commented Jun 2, 2023

hayata-suenaga commented Jun 4, 2023

hayata-suenaga commented Jun 4, 2023

hayata-suenaga left a comment

mollfpr commented Jun 5, 2023

thienlnam commented Jun 5, 2023

thienlnam commented Jun 5, 2023

mollfpr left a comment

melvin-bot bot commented Jun 6, 2023

thienlnam commented Jun 6, 2023

mollfpr commented Jun 6, 2023

thienlnam commented Jun 6, 2023

thienlnam commented Jun 6, 2023

github-actions bot commented Jun 6, 2023

neil-marcellini left a comment

neil-marcellini Jun 6, 2023

thienlnam Jun 6, 2023

OSBotify commented Jun 15, 2023

OSBotify commented Jun 16, 2023

narefyev91 Jul 13, 2023

s77rt Aug 13, 2023

Move task reportActions onto the task report #20014

Move task reportActions onto the task report #20014

Conversation

thienlnam commented Jun 1, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

thienlnam commented Jun 2, 2023

melvin-bot bot commented Jun 2, 2023

thienlnam commented Jun 2, 2023

mollfpr commented Jun 2, 2023

thienlnam commented Jun 2, 2023

hayata-suenaga commented Jun 4, 2023

hayata-suenaga commented Jun 4, 2023

hayata-suenaga left a comment

Choose a reason for hiding this comment

mollfpr commented Jun 5, 2023

thienlnam commented Jun 5, 2023

thienlnam commented Jun 5, 2023

mollfpr left a comment

Choose a reason for hiding this comment

melvin-bot bot commented Jun 6, 2023

thienlnam commented Jun 6, 2023

mollfpr commented Jun 6, 2023

thienlnam commented Jun 6, 2023

thienlnam commented Jun 6, 2023

github-actions bot commented Jun 6, 2023

neil-marcellini left a comment

Choose a reason for hiding this comment

neil-marcellini Jun 6, 2023

Choose a reason for hiding this comment

thienlnam Jun 6, 2023

Choose a reason for hiding this comment

OSBotify commented Jun 15, 2023

OSBotify commented Jun 16, 2023

narefyev91 Jul 13, 2023

Choose a reason for hiding this comment

s77rt Aug 13, 2023

Choose a reason for hiding this comment

thienlnam commented Jun 1, 2023 •

edited

Loading