Maintain grayed out chats with multiple offline splits #13602

neil-marcellini · 2022-12-14T23:37:47Z

Details

Note that when going back online the messages will briefly be out of order, which also happens on production currently. I did a small fix after testing on all platforms to make sure that the CREATED report action is always displayed first. The other actions will still be out of order briefly, but with that fix the bug is much less jarring.

Fixed Issues

$ #13586
PROPOSAL: N/A

Tests

Only offline is relevant.

Verify that no errors appear in the JS console

Offline tests

Go offline
From the global create menu +, select Split bill
Go through the prompt and select TWO or MORE participants that you have never split a bill with, and submit the request
Verify that the group chat is created with those participants and you are navigated to it (the whole chat should be greyed out)
Verify that the group chat has a message Split $X with and (this should be greyed out)
Verify that individual chats with each participant are created (these chats should be greyed out)
Verify that individual chats have the IOU message and preview (these messages should be greyed out)
From the group chat, tap + > Split bill and send another split bill request to the same users
Verify that the modal is dismissed
Verify that another split message shows up as the latest message in the group chat (this message should be greyed out)
Verify that each individual chat shows the new request and the total has been updated (these messages should be greyed out)
Turn on your internet connection
Verify that the chats and messages are still the same, but are no longer greyed out

QA Steps

Run the offline tests.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

web.mov

Mobile Web - Chrome

android-Chrome.mov

Mobile Web - Safari

iOS-Safari.mp4

Desktop

desktop.mov

iOS

iOS.mp4

Android

android.mov

melvin-bot · 2022-12-20T23:44:44Z

@techievivek 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]

techievivek

Looks good. Not sure why no C+ got added as a PR reviewer. If that's expected then I can test it and fill out the checklist.

techievivek · 2022-12-21T09:12:08Z

src/libs/ReportActionsUtils.js

@@ -52,6 +52,11 @@ function getSortedReportActions(reportActions, shouldSortInDescendingOrder = fal
    }
    const invertedMultiplier = shouldSortInDescendingOrder ? -1 : 1;
    reportActions.sort((first, second) => {
+        // If only one action is a report created action, return the created action first.


This is already being discussed here #13310 is not a priority for now. So maybe we can just focus on greying out the LHN.

I added a reportActionID to created report actions to fix a console error where the report action item in the list was missing a key (the reportActionID). As a result of that change the created messages would appear after the IOU message because they have the same created timestamp. Adding this condition fixes that issue and helps with the ordering so I would like to keep the change.

NAB from my end. But I guess we will anyhow need to clean the sort method because reportActions would be random now, right?

techievivek · 2022-12-21T09:18:04Z

src/libs/actions/IOU.js

-    };
+
+    // If we have an existing groupChatReport use it's pending fields, otherwise indicate that we are adding a chat
+    if (!existingGroupChatReport) {


Good catch.

neil-marcellini · 2022-12-21T17:40:32Z

Not sure why no C+ got added as a PR reviewer. If that's expected then I can test it and fill out the checklist.

Yeah that is odd. I have reported it in #expensify-bugs here.

mananjadhav · 2022-12-21T19:52:42Z

@neil-marcellini I can see the order changing when moving from offline to online. I can see that in your video too. I think that's an issue right? Because it takes one message at a time and syncs online.

techievivek · 2022-12-21T20:33:38Z

@mananjadhav Yep, that's expected for now. We already have an issue for this but not prioritizing it for now.

mananjadhav · 2022-12-21T20:43:25Z

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 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 correct English and 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 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.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

web-grayed-out-split-bill.mov

Mobile Web - Chrome

mweb-chrome-grayed-out-split-bill.mov

Mobile Web - Safari

mweb-safari-grayed-out-split-bill.mov

Desktop

desktop-grayed-out-split-bill.mov

iOS

ios-grayed-out-split-bill.mov

Android

android-grayed-out-split-bill.mov

mananjadhav · 2022-12-21T20:44:49Z

Thanks for clarifying @techievivek. I've tested the flows and it works fine. Completed the checklist in the previous comment along with the screencast.

All yours.

techievivek

Looks good, thanks for the explanation.

OSBotify · 2022-12-21T21:07:49Z

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

github-actions · 2022-12-21T21:25:36Z

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries

Name	Duration
TTI	815.455 ms → 838.158 ms (+22.704 ms, +2.8%)
runJsBundle	196.656 ms → 200.406 ms (+3.750 ms, +1.9%)
nativeLaunch	9.800 ms → 11.031 ms (+1.231 ms, +12.6%)
regularAppStart	0.014 ms → 0.015 ms (+0.000 ms, +2.1%)

Show details

Name	Duration
TTI	Baseline Mean: 815.455 ms Stdev: 37.634 ms (4.6%) Runs: 745.8070189999416 755.1771290004253 765.550401000306 771.9216750003397 774.5715260002762 776.5358210001141 779.0264839995652 779.8079530000687 781.947598000057 785.7526839999482 798.5864460002631 800.7082319995388 803.0108479997143 805.3376709995791 809.5746419997886 811.8871139995754 820.1153269996867 821.6496270000935 824.6187199996784 825.4735070001334 827.5829490004107 842.8458669995889 844.8203720003366 858.1138970004395 862.8346130000427 864.3946970002726 865.5558000002056 866.3846340002492 868.7857079999521 870.1674480000511 870.5465649999678 Current Mean: 838.158 ms Stdev: 33.676 ms (4.0%) Runs: 791.2366009997204 793.6173660000786 795.9627120001242 801.8037550002337 802.6742470003664 808.3602520003915 814.2931070001796 815.2544360002503 815.3458799999207 818.4566449997947 822.901115000248 826.4224009998143 829.1022439999506 830.244370999746 832.7288889996707 834.7022439995781 835.4366509998217 835.4594520004466 837.587170000188 855.6437250003219 859.1852810001001 860.4776659999043 860.7325259996578 861.6758810002357 864.2645650003105 865.102253000252 895.3467150004581 915.3153170002624 927.2555839996785
runJsBundle	Baseline Mean: 196.656 ms Stdev: 25.730 ms (13.1%) Runs: 149 155 159 162 170 171 172 172 180 182 183 184 186 186 186 190 191 198 206 208 212 218 219 221 221 224 226 227 228 230 238 239 Current Mean: 200.406 ms Stdev: 20.373 ms (10.2%) Runs: 159 169 174 176 177 180 185 185 188 189 190 192 192 196 196 199 201 201 201 202 206 210 210 212 217 217 220 226 227 231 234 251
nativeLaunch	Baseline Mean: 9.800 ms Stdev: 1.470 ms (15.0%) Runs: 8 8 8 8 8 8 9 9 9 9 9 9 9 9 9 9 10 10 10 10 11 11 11 11 11 11 12 12 13 13 Current Mean: 11.031 ms Stdev: 3.147 ms (28.5%) Runs: 8 8 8 8 8 8 8 9 9 9 9 9 9 9 10 10 10 10 10 11 11 11 12 12 12 14 15 17 17 17 17 18
regularAppStart	Baseline Mean: 0.014 ms Stdev: 0.001 ms (7.8%) Runs: 0.012450999580323696 0.012899000197649002 0.012980000115931034 0.013102000579237938 0.013184000737965107 0.013224000111222267 0.013387000188231468 0.0134680001065135 0.013468999415636063 0.013508999720215797 0.013590000569820404 0.013712000101804733 0.013875000178813934 0.013996999710798264 0.014038000255823135 0.014119000174105167 0.014159999787807465 0.014201000332832336 0.014322999864816666 0.014403999783098698 0.01444500032812357 0.014485000632703304 0.014812000095844269 0.0148930000141263 0.014973999932408333 0.015258999541401863 0.015258999541401863 0.015461999922990799 0.01574699953198433 0.016600999981164932 0.0167239997535944 0.017008000053465366 Current Mean: 0.015 ms Stdev: 0.001 ms (5.2%) Runs: 0.013387000188231468 0.013509999960660934 0.01355000026524067 0.013671999797224998 0.013793999329209328 0.0138349998742342 0.013875000178813934 0.013997000642120838 0.01407800056040287 0.014159999787807465 0.014241999946534634 0.014362999238073826 0.014526000246405602 0.014566999860107899 0.014607999473810196 0.014689000323414803 0.014811000786721706 0.014851999469101429 0.014891999773681164 0.0148930000141263 0.014933000318706036 0.015096000395715237 0.015298999845981598 0.015298999845981598 0.015339999459683895 0.015461999922990799 0.01550300046801567 0.015625 0.01680499967187643

OSBotify · 2022-12-21T23:39:20Z

🚀 Deployed to staging by @techievivek in version: 1.2.43-0 🚀

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

neil-marcellini · 2022-12-22T01:48:56Z

Thanks for the reviews guys! I'm going to close out the related issue #13586 in favor of #13310 because they are the same problem.

kavimuru · 2022-12-22T16:43:43Z

@neil-marcellini Now there is a slightly different issue than the original issue #13586. Do we need to create separate issue?
Or is it a dupe of #13310?
https://user-images.githubusercontent.com/43996225/209183035-7d1f2a5e-1c07-40c4-a942-3c06adcf0699.mp4

neil-marcellini · 2022-12-22T17:20:36Z

@kavimuru That's an issue that existed before this PR, and yes it will be solved by #13310.

OSBotify · 2022-12-27T18:25:42Z

🚀 Deployed to production by @chiragsalian in version: 1.2.43-1 🚀

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

mananjadhav · 2023-01-03T10:41:32Z

@neil-marcellini @techievivek @johncschuster This is ready for payout today. But the title isn't updated on the issue. Also the issue is locked, hence I couldn't comment there.

neil-marcellini · 2023-01-03T16:03:51Z

I updated the title and @johncschuster will handle the payment soon.

mananjadhav · 2023-01-06T04:05:25Z

@johncschuster can you help me with the payout?

johncschuster · 2023-01-06T16:23:03Z

@mananjadhav yep! I'm on it right now!

mananjadhav · 2023-01-06T16:27:11Z

@johncschuster i can’t apply to the job as it says Access Denied.

Couldn’t comment on the issue hence responding here.

johncschuster · 2023-01-06T21:12:23Z

Weeeeird. Let me take a look! I'll post back here in these comments.

johncschuster · 2023-01-06T21:14:11Z

@mananjadhav Ok, I just made the job public in case that was the issue. Can you try applying again?

https://www.upwork.com/jobs/~01d7aa95ed8ddc61fb

mananjadhav · 2023-01-06T21:47:21Z

This worked. Applied @johncschuster

johncschuster · 2023-01-06T22:05:22Z

Offer sent! Let me know when you accept and I'll get the payment issued. Thanks!

mananjadhav · 2023-01-07T03:30:02Z

Accepted @johncschuster

johncschuster · 2023-01-09T16:05:05Z

Paid! 🎉 Thanks, @mananjadhav!

Use existing pending chat field vs clearing it

9cf2a5a

neil-marcellini self-assigned this Dec 14, 2022

neil-marcellini mentioned this pull request Dec 20, 2022

BUG: Out of order messages when creating 2 bill splits while offline #13586

Closed

neil-marcellini added 3 commits December 20, 2022 15:04

Give optimistic created report actions a list key

c9faf0f

Merge branch 'main' into neil-fix-offline-split

c9647b6

Always sort created report actions first

ea2ce75

neil-marcellini changed the title ~~Use existing pending chat field instead of clearing it~~ Maintain grayed out chats with multiple offline splits Dec 20, 2022

neil-marcellini marked this pull request as ready for review December 20, 2022 23:44

neil-marcellini requested a review from a team as a code owner December 20, 2022 23:44

melvin-bot bot requested review from techievivek and removed request for a team December 20, 2022 23:44

techievivek reviewed Dec 21, 2022

View reviewed changes

neil-marcellini requested a review from mananjadhav December 21, 2022 18:27

mananjadhav approved these changes Dec 21, 2022

View reviewed changes

techievivek approved these changes Dec 21, 2022

View reviewed changes

techievivek merged commit 3ecec61 into main Dec 21, 2022

techievivek deleted the neil-fix-offline-split branch December 21, 2022 21:02

OSBotify mentioned this pull request Dec 21, 2022

Deploy Checklist: New Expensify 2022-12-21 #13777

Closed

20 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Maintain grayed out chats with multiple offline splits #13602

Maintain grayed out chats with multiple offline splits #13602

neil-marcellini commented Dec 14, 2022 •

edited

Loading

melvin-bot bot commented Dec 20, 2022

techievivek left a comment

techievivek Dec 21, 2022

neil-marcellini Dec 21, 2022

techievivek Dec 21, 2022

techievivek Dec 21, 2022

neil-marcellini commented Dec 21, 2022

mananjadhav commented Dec 21, 2022 •

edited

Loading

techievivek commented Dec 21, 2022

mananjadhav commented Dec 21, 2022

mananjadhav commented Dec 21, 2022

techievivek left a comment

OSBotify commented Dec 21, 2022

github-actions bot commented Dec 21, 2022

OSBotify commented Dec 21, 2022

neil-marcellini commented Dec 22, 2022

kavimuru commented Dec 22, 2022

neil-marcellini commented Dec 22, 2022

OSBotify commented Dec 27, 2022

mananjadhav commented Jan 3, 2023

neil-marcellini commented Jan 3, 2023

mananjadhav commented Jan 6, 2023

johncschuster commented Jan 6, 2023

mananjadhav commented Jan 6, 2023

johncschuster commented Jan 6, 2023

johncschuster commented Jan 6, 2023

mananjadhav commented Jan 6, 2023

johncschuster commented Jan 6, 2023

mananjadhav commented Jan 7, 2023

johncschuster commented Jan 9, 2023

Maintain grayed out chats with multiple offline splits #13602

Maintain grayed out chats with multiple offline splits #13602

Conversation

neil-marcellini commented Dec 14, 2022 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Dec 20, 2022

techievivek left a comment

Choose a reason for hiding this comment

techievivek Dec 21, 2022

Choose a reason for hiding this comment

neil-marcellini Dec 21, 2022

Choose a reason for hiding this comment

techievivek Dec 21, 2022

Choose a reason for hiding this comment

techievivek Dec 21, 2022

Choose a reason for hiding this comment

neil-marcellini commented Dec 21, 2022

mananjadhav commented Dec 21, 2022 • edited Loading

techievivek commented Dec 21, 2022

mananjadhav commented Dec 21, 2022

Reviewer Checklist

Screenshots/Videos

mananjadhav commented Dec 21, 2022

techievivek left a comment

Choose a reason for hiding this comment

OSBotify commented Dec 21, 2022

github-actions bot commented Dec 21, 2022

Performance Comparison Report 📊

Significant Changes To Duration

Meaningless Changes To Duration

OSBotify commented Dec 21, 2022

neil-marcellini commented Dec 22, 2022

kavimuru commented Dec 22, 2022

neil-marcellini commented Dec 22, 2022

OSBotify commented Dec 27, 2022

mananjadhav commented Jan 3, 2023

neil-marcellini commented Jan 3, 2023

mananjadhav commented Jan 6, 2023

johncschuster commented Jan 6, 2023

mananjadhav commented Jan 6, 2023

johncschuster commented Jan 6, 2023

johncschuster commented Jan 6, 2023

mananjadhav commented Jan 6, 2023

johncschuster commented Jan 6, 2023

mananjadhav commented Jan 7, 2023

johncschuster commented Jan 9, 2023

neil-marcellini commented Dec 14, 2022 •

edited

Loading

mananjadhav commented Dec 21, 2022 •

edited

Loading