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

feat: Build optimistic Edit Task actions #36768

Merged
merged 17 commits into from
Mar 7, 2024

Conversation

paultsimura
Copy link
Contributor

@paultsimura paultsimura commented Feb 17, 2024

Details

Fixed Issues

$ #36196
PROPOSAL: #36196 (comment)

Tests

Same as QA

Offline tests

Same as QA

QA Steps

  1. Create a task
  2. Go offline
  3. Add a description
  4. Verify the updated the description to {value} report action is added
  5. Modify the title
  6. Verify another updated the task title to {value} report action is added
  7. Remove the description
  8. Verify the removed the description report action is added
  9. Assign somebody
  10. Verify the assigned to @user report action is added
  11. Go online
  12. Refresh the page
  13. Verify all the report actions remain

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: mWeb Chrome
    • iOS: Native
    • iOS: mWeb 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 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(theme.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 the PR modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label so the design team can review the changes.
  • 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.

Screenshots/Videos

Android: Native
android-compressed.mp4
Android: mWeb Chrome
chrome-compressed.mp4
iOS: Native
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-02-29.at.18.24.41-compressed.mp4
iOS: mWeb Safari
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-02-29.at.18.28.26-compressed.mp4
MacOS: Chrome / Safari
Screen.Recording.2024-02-29.at.02.03.51-compressed.mp4
MacOS: Desktop
Screen.Recording.2024-02-29.at.17.48.23-compressed.mp4

@paultsimura
Copy link
Contributor Author

@thienlnam there are some points I'd like to discuss with you.

First of all, the EditTaskAssignee and EditTask API calls do not return the "modified" reportAction in the response – it comes later from the OpenReport, so we need to refresh (or revisit) the report page to replace the optimistic report action with the one from the server – this should be fixed on the BE side.

The second is related to the mentions when changing the assignee. Currently, they come from the BE in this format:

[
  {
    "type": "TEXT",
    "style": "normal",
    "text": "assigned to <mention-user>user_d (paultsimura+d@gmail.com)</mention-user>"
  }
]

This is incorrect in a couple of ways. First, the <mention-user> tag doesn't support this format – the easiest way would be to use the accountId param, e.g.:

`assigned to <mention-user accountID=${assigneeAccountID}></mention-user>`

The display name will be fetched automatically by the provided accountID by the HTML renderer.

And one last thing is the "type": "TEXT" doesn't support the HTML render – it will show the action as a plain text. We need to use the "type": "COMMENT" in order for mentions to be displayed correctly, or think of a way to allow parsing HTML in the TEXT actions.

@thienlnam
Copy link
Contributor

Thanks for the comments

First of all, the EditTaskAssignee and EditTask API calls do not return the "modified" reportAction in the response – it comes later from the OpenReport, so we need to refresh (or revisit) the report page to replace the optimistic report action with the one from the server – this should be fixed on the BE side.

Which reportAction are you referring to in this case? There should be a pusher update which updates the parent reportAction data so you don't need to fetch it from the API call. Though if you're talking about the edited reportAction, I agree we might need to update it so that it does

As for the other items, I'll get a fix up for those!

@paultsimura
Copy link
Contributor Author

Though if you're talking about the edited reportAction

Yes, I'm talking about them. The optimistic ones do not get replaced with the ones from BE until the page is refreshed, we just need to add one more merge to the BE response.

@paultsimura
Copy link
Contributor Author

As for the other items, I'll get a fix up for those!

Would be great if you could test this PR's optimistic flow to see if the BE should match it – so that we'll be on the same page.

As a side note: I saw you intended to use the Assigned to "you" mention format from the BE – I'm not sure it's something we currently support – the self-mention will render also with the displayName, but highlighted in green (all other mentions will be blue).

@thienlnam
Copy link
Contributor

As a side note: I saw you intended to use the Assigned to "you" mention format from the BE – I'm not sure it's something we currently support – the self-mention will render also with the displayName, but highlighted in green (all other mentions will be blue).

The "you" wasn't going to be a mention, but I think I'll just update it to be a mention and then it will show as green - that's okay as well

@paultsimura
Copy link
Contributor Author

The "you" wasn't going to be a mention

It's currently coming from BE like this, therefore I assumed the mention 🙂
image

@thienlnam
Copy link
Contributor

Screenshot 2024-02-20 at 6 06 24 PM

I tested the changes with your branch, looks good - seems like the only change that is needed is for the comments to show as system messages UI

@paultsimura
Copy link
Contributor Author

paultsimura commented Feb 21, 2024

We have at least 2 formats of system messages. Do you mean we would like the TASKEDITED messages to look like the "room renamed" actions (the second screenshot), not like the "Paid" ones (first screenshot)?

If so – we'll need to change the action format a little, so I'll ask you to hold on with the BE change for a while.

@thienlnam
Copy link
Contributor

No no, they should look like the first screenshot - I more meant that it the UI should be in the same text color from this screenshot #36768 (comment)

Though, I don't think I've seen a mention within a system message so this might be new

@paultsimura
Copy link
Contributor Author

Ah yes - that's a bit of an issue — the bold system-like formatting is supported by the actions of type TEXT, but these actions do not support mentions. And vice-versa, the COMMENT type supports mentions, but not the formatting. I can look what we can do with this formatting, but need to know if it's worth the time at this point

@thienlnam
Copy link
Contributor

Hahah of course there is something like that - I think for now it's okay to keep as is so no need to spend additional time investigating. Thanks for being willing though

@paultsimura
Copy link
Contributor Author

Great, so I'm waiting for your BE changes before moving this PR to review, right?

@thienlnam
Copy link
Contributor

Yes that's correct - the PRs are in review currently. I'll let you know when they're on production

@thienlnam
Copy link
Contributor

Changes should be on production now

@paultsimura
Copy link
Contributor Author

paultsimura commented Feb 28, 2024

Thanks @thienlnam. They are, so I'm unblocked now, but this one's still missing: #36768 (comment)

The EditTask API response does not update the task report's reportActions to include the new TASKEDITED action.
Here's the recording, the reportActions is updated with the server data only after we refresh the page and the OpenReport response comes (the task report ID is 5598782205931668):

Screen.Recording.2024-02-28.at.15.47.16-compressed.mp4

We must add them, otherwise, it's messing with the order of the action when coming online having a couple of actions pending (I suppose this is the root cause):

Untitled-compressed.mp4

@paultsimura
Copy link
Contributor Author

@thienlnam another BE bug: when editing the assignee, we need to add their display name to the reportAction.message.text.

This is what an optimistic action looks like (note the text in LHN as a result):
image

And here's what it becomes after coming online and being updated from the BE:
image

@paultsimura
Copy link
Contributor Author

paultsimura commented Feb 28, 2024

One more thing @thienlnam, sorry.

In money requests, we don't count the MODIFIEDEXPENSE system messages as visibleChildReportActions, therefore a money request with only such messages can be fully deleted (if there are other messages inside – the report won't be deleted). Also, these messages are not counted as "thread replies count".
Demo:

Screen.Recording.2024-02-29.at.00.17.48-compressed.mp4

Do I understand correctly that we want to implement a similar approach for tasks?

@thienlnam
Copy link
Contributor

No worries, thanks for pointing these out - the second one has been an issue and I'll address those issues likely next week but it shouldn't block this PR from going out

Do I understand correctly that we want to implement a similar approach for tasks?

Yes, we want the task to delete entirely if there are no other visible reportActions - however, instead of changing it so that it doesn't count as a visible reportAction (because it is actually visible). Though I'd need to double check if this is a pattern we're taking with all system messages or not. Ideally I think we would just check that there are no visible reportActions or only system messages

Also, these messages are not counted as "thread replies count".

I'll add this to the list of BE fixes needed

@paultsimura
Copy link
Contributor Author

paultsimura commented Feb 29, 2024

Also, these messages are not counted as "thread replies count".

I'll add this to the list of BE fixes needed

This is similar to how it's done with the Money Request system messages as well as the "task completed/reopened", so I would double-check that - it seems expected to me.

This PR is ready for review code-wise, so I would recommend just checking how the branch works, and letting me know if something is misaligned with your vision.
Btw, I managed to make these messages look like the system ones.

@paultsimura
Copy link
Contributor Author

Bug:

  1. Create a task
  2. Go offline
  3. Add a description
  4. Add an assignee
  5. Mark as complete
  6. Mark as incomplete
  7. At this point all of the report actions are in the correct order
  8. Go online
  9. Notice "marked as complete" and "marked as incomplete" items changing position while the app refreshes

That's known: #36768 (comment)

@thienlnam is tracking this on the BE side.

@paultsimura
Copy link
Contributor Author

paultsimura commented Mar 4, 2024

I think we should localize the actions, they aren't translated into Spanish

For this, we'll need another BE change.
@thienlnam could you please add to your tracking issue the following?

The TASKEDITED report actions should reflect the MODIFIEDEXPENSE:

  1. The API endpoint must consume one field at a time (title / description);
  2. The originalMessage must contain only the changed field (old + new value), as it is for expenses:

Updated Description:

{
  "oldComment": "",
  "newComment": "Desc",
  "lastModified": "2024-03-04 23:22:47.451"
}

Updated Merchant:

{
  "oldMerchant": "Old Merchant",
  "merchant": "New Merchant",
  "lastModified": "2024-03-04 23:28:22.705"
}

We need to make sure the originalMessage will cover the title, description, and assigneeAccountID change.
Based on this, we'll be able to build a localizable message for each TASKEDITED action, similar to MODIFIEDEXPENSE here:

function getForReportAction(reportID: string | undefined, reportAction: OnyxEntry<ReportAction>): string {
if (reportAction?.actionName !== CONST.REPORT.ACTIONS.TYPE.MODIFIEDEXPENSE) {
return '';
}
const reportActionOriginalMessage = reportAction?.originalMessage as ExpenseOriginalMessage | undefined;
const policyID = ReportUtils.getReportPolicyID(reportID) ?? '';
const removalFragments: string[] = [];
const setFragments: string[] = [];
const changeFragments: string[] = [];
const hasModifiedAmount =
reportActionOriginalMessage &&
'oldAmount' in reportActionOriginalMessage &&
'oldCurrency' in reportActionOriginalMessage &&
'amount' in reportActionOriginalMessage &&
'currency' in reportActionOriginalMessage;
const hasModifiedMerchant = reportActionOriginalMessage && 'oldMerchant' in reportActionOriginalMessage && 'merchant' in reportActionOriginalMessage;
if (hasModifiedAmount) {
const oldCurrency = reportActionOriginalMessage?.oldCurrency ?? '';
const oldAmountValue = reportActionOriginalMessage?.oldAmount ?? 0;
const oldAmount = oldAmountValue > 0 ? CurrencyUtils.convertToDisplayString(reportActionOriginalMessage?.oldAmount ?? 0, oldCurrency) : '';
const currency = reportActionOriginalMessage?.currency ?? '';
const amount = CurrencyUtils.convertToDisplayString(reportActionOriginalMessage?.amount ?? 0, currency);
// Only Distance edits should modify amount and merchant (which stores distance) in a single transaction.
// We check the merchant is in distance format (includes @) as a sanity check
if (hasModifiedMerchant && (reportActionOriginalMessage?.merchant ?? '').includes('@')) {
return getForDistanceRequest(reportActionOriginalMessage?.merchant ?? '', reportActionOriginalMessage?.oldMerchant ?? '', amount, oldAmount);
}
buildMessageFragmentForValue(amount, oldAmount, Localize.translateLocal('iou.amount'), false, setFragments, removalFragments, changeFragments);
}
const hasModifiedComment = reportActionOriginalMessage && 'oldComment' in reportActionOriginalMessage && 'newComment' in reportActionOriginalMessage;
if (hasModifiedComment) {
buildMessageFragmentForValue(
reportActionOriginalMessage?.newComment ?? '',
reportActionOriginalMessage?.oldComment ?? '',
Localize.translateLocal('common.description'),
true,
setFragments,
removalFragments,
changeFragments,
);
}
if (reportActionOriginalMessage?.oldCreated && reportActionOriginalMessage?.created) {
const formattedOldCreated = DateUtils.formatWithUTCTimeZone(reportActionOriginalMessage.oldCreated, CONST.DATE.FNS_FORMAT_STRING);
buildMessageFragmentForValue(
reportActionOriginalMessage.created,
formattedOldCreated,
Localize.translateLocal('common.date'),
false,
setFragments,
removalFragments,
changeFragments,
);
}
if (hasModifiedMerchant) {
buildMessageFragmentForValue(
reportActionOriginalMessage?.merchant ?? '',
reportActionOriginalMessage?.oldMerchant ?? '',
Localize.translateLocal('common.merchant'),
true,
setFragments,
removalFragments,
changeFragments,
);
}
const hasModifiedCategory = reportActionOriginalMessage && 'oldCategory' in reportActionOriginalMessage && 'category' in reportActionOriginalMessage;
if (hasModifiedCategory) {
buildMessageFragmentForValue(
reportActionOriginalMessage?.category ?? '',
reportActionOriginalMessage?.oldCategory ?? '',
Localize.translateLocal('common.category'),
true,
setFragments,
removalFragments,
changeFragments,
);
}
const hasModifiedTag = reportActionOriginalMessage && 'oldTag' in reportActionOriginalMessage && 'tag' in reportActionOriginalMessage;
if (hasModifiedTag) {
const policyTags = allPolicyTags?.[`${ONYXKEYS.COLLECTION.POLICY_TAGS}${policyID}`] ?? {};
const transactionTag = reportActionOriginalMessage?.tag ?? '';
const oldTransactionTag = reportActionOriginalMessage?.oldTag ?? '';
const splittedTag = TransactionUtils.getTagArrayFromName(transactionTag);
const splittedOldTag = TransactionUtils.getTagArrayFromName(oldTransactionTag);
const localizedTagListName = Localize.translateLocal('common.tag');
Object.keys(policyTags).forEach((policyTagKey, index) => {
const policyTagListName = PolicyUtils.getTagListName(policyTags, index) || localizedTagListName;
const newTag = splittedTag[index] ?? '';
const oldTag = splittedOldTag[index] ?? '';
if (newTag !== oldTag) {
buildMessageFragmentForValue(
PolicyUtils.getCleanedTagName(newTag),
PolicyUtils.getCleanedTagName(oldTag),
policyTagListName,
true,
setFragments,
removalFragments,
changeFragments,
policyTagListName === localizedTagListName,
);
}
});
}
const hasModifiedBillable = reportActionOriginalMessage && 'oldBillable' in reportActionOriginalMessage && 'billable' in reportActionOriginalMessage;
if (hasModifiedBillable) {
buildMessageFragmentForValue(
reportActionOriginalMessage?.billable ?? '',
reportActionOriginalMessage?.oldBillable ?? '',
Localize.translateLocal('iou.request'),
true,
setFragments,
removalFragments,
changeFragments,
);
}
const message =
getMessageLine(`\n${Localize.translateLocal('iou.changed')}`, changeFragments) +
getMessageLine(`\n${Localize.translateLocal('iou.set')}`, setFragments) +
getMessageLine(`\n${Localize.translateLocal('iou.removed')}`, removalFragments);
if (message === '') {
return Localize.translateLocal('iou.changedTheRequest');
}
return `${message.substring(1, message.length)}`;
}

@paultsimura
Copy link
Contributor Author

@thienlnam I have implemented the localizable optimistic actions. So if you haven't updated the API on the BE, I'll share the object type I'm currently expecting optimistically, please share what you think about it:

type OriginalMessageTaskEdited = {
    actionName: typeof CONST.REPORT.ACTIONS.TYPE.TASKEDITED;
    originalMessage: {
        oldTitle?: string;
        title?: string;
        oldDescription?: string;
        description?: string;
        oldAssigneeAccountID?: number;
        assigneeAccountID?: number;
    };
};

This way, the originalMessage will be as close to the MODIFIEDEXPENSE as possible: it will hold only oldValue and value of the field that was modified.
Besides that, storing the oldValue will allow displaying messages in a format like: updated the description to "new" (previously "old") if we want to.

@paultsimura paultsimura marked this pull request as draft March 6, 2024 19:04
@thienlnam
Copy link
Contributor

I should have clarified but let's move forward without localization in this PR - localization is not a current priority so while I've added it to the list - it may be a while until we decide to reprioritize those changes

@paultsimura paultsimura marked this pull request as ready for review March 6, 2024 19:39
@paultsimura
Copy link
Contributor Author

Got you. Then we're good to test again @eVoloshchak.

Copy link
Contributor

@eVoloshchak eVoloshchak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested this by cherry-picking the PR into the latest main, works well, approving
@paultsimura, could you resolve the conflicts please?

@melvin-bot melvin-bot bot requested a review from thienlnam March 7, 2024 18:29
# Conflicts:
#	src/pages/home/report/ReportActionItem.tsx
@thienlnam
Copy link
Contributor

Getting some type errors cc @paultsimura

Error: src/libs/actions/Task.ts(483,93): error TS2345: Argument of type 'number | null' is not assignable to parameter of type 'number'.
  Type 'null' is not assignable to type 'number'.

@paultsimura
Copy link
Contributor Author

Getting some type errors

Should be resolved now, let's see what the CI will say...

@thienlnam
Copy link
Contributor

All set - thanks for the quick work on this!

@thienlnam thienlnam merged commit 6b7b972 into Expensify:main Mar 7, 2024
15 checks passed
@OSBotify
Copy link
Contributor

OSBotify commented Mar 7, 2024

✋ 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
Copy link
Contributor

OSBotify commented Mar 8, 2024

🚀 Deployed to staging by https://github.com/thienlnam in version: 1.4.49-0 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/luacmartins in version: 1.4.50-5 🚀

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

) {
lastMessageTextFromReport = lastReportAction?.message?.[0].text ?? '';
} else if (ReportActionUtils.isTaskAction(lastReportAction)) {
lastMessageTextFromReport = TaskUtils.getTaskReportActionMessage(lastReportAction).text;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line will introduce the regression #37976, causing the last message text to become multiple lines in the LHN. So I think in the future whenever we develop a feature that relate to text, we should also test the text overflow case.

text: ' edited this task',
type: CONST.REPORT.MESSAGE.TYPE.COMMENT,
text: `assigned to ${getDisplayNameForParticipant(assigneeAccountID)}`,
html: `assigned to <mention-user accountID=${assigneeAccountID}></mention-user>`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line created a bug where LHN won't show the user. See more details on #45167

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants