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 2022-12-20] [$250] BUG: Out of order LHN after split bill between web (staging.new.expensify) and IOS app reported by @chauchausoup #11590

Closed
kavimuru opened this issue Oct 4, 2022 · 52 comments
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 Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review

Comments

@kavimuru
Copy link

kavimuru commented Oct 4, 2022

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. Split bill with 7+ unique users on iOS/Android or web
  2. Navigate back to the LHN
  3. Observe that there is a different order of contacts between mobile app and Web
  4. Observe that the group message has a different subtitle message between mobile app and Web

Expected Result:

  • Order of the contacts should be same in web and IOS of user profiles
  • Both web and IOS should have correct subtitle messages.

Actual Result:

Order of contacts is different in web and iOS

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android

Version Number: 1.2.11-2
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

image

Expensify/Expensify Issue URL:
Issue reported by: @chauchausoup
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1664646300696639

View all open jobs on GitHub

@kavimuru kavimuru added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels Oct 4, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 4, 2022

Triggered auto assignment to @sophiepintoraetz (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@melvin-bot melvin-bot bot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Oct 4, 2022
@sophiepintoraetz
Copy link
Contributor

Hey @kavimuru - can you update the screenshots to clearly mark which chats you are referring to? It's hard to follow. Please reassign me once you've done so - or tag me in the comments.

@kavimuru
Copy link
Author

kavimuru commented Oct 5, 2022

@sophiepintoraetz I have attached the screenshot
splitbill order of contacts

@kavimuru kavimuru assigned sophiepintoraetz and unassigned kavimuru Oct 5, 2022
@sophiepintoraetz sophiepintoraetz added Weekly KSv2 Engineering Improvement Item broken or needs improvement. and removed Daily KSv2 labels Oct 5, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 5, 2022

Triggered auto assignment to @grgia (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@grgia
Copy link
Contributor

grgia commented Oct 5, 2022

I'm not able to replicate this on staging web/mobile

@sophiepintoraetz

This comment was marked as off-topic.

@chauchausoup
Copy link

@sophiepintoraetz I think you didn't got the issue correctly. Issue relates to post Split Bill action. Please check upper screenshots in more detail or slack convo too.

@sophiepintoraetz
Copy link
Contributor

Ah, split bill - I'm not able to test this out yet. Unfortuantely, unless @grgia is able to reproduce, this issue is not likely to go anywhere. @grgia - do we need to loop in someone else to confirm? Or @chauchausoup - can you reproduce?

@chauchausoup
Copy link

Yes I can reproduce it again at my end @sophiepintoraetz as per the video and image references attached above.

@sophiepintoraetz
Copy link
Contributor

@chauchausoup - we need this to be reproducible, given you are the person who reported it, we need an independent reproduction.

@chauchausoup
Copy link

@sophiepintoraetz I think the maximum number of profiles that can take part in split bill is 8. So if you try your test with similar number of profiles then I think you might be able to reproduce.

@grgia
Copy link
Contributor

grgia commented Oct 14, 2022

@chauchausoup which version are you testing on?

@chauchausoup
Copy link

@grgia I am testing it with https://staging.new.expensify.com/ web app and IOS app from app store.

@sophiepintoraetz
Copy link
Contributor

@grgia - this is in the OP Version Number: 1.2.11-2 as well

@grgia
Copy link
Contributor

grgia commented Oct 17, 2022

Reopening, this is after requesting a split of $10 with 8 people. Different order LHN
Screen Shot 2022-10-17 at 10 39 52 AM
Screen Shot 2022-10-17 at 10 38 46 AM
IMG_4365

@grgia grgia reopened this Oct 17, 2022
@chiragsalian
Copy link
Contributor

I've got fixes for the LHN ready. I've tested it and I think it works better.
I'd like to add some jest unit tests for it and then it should be ready for review. I'll add it on Monday since I'm OOO the rest of the week.

@chiragsalian
Copy link
Contributor

Also another issue was mentioned here i.e.,

Observe that the group message has a different subtitle message between mobile app and Web

Mind if we split that into another issue? You can assign it to me. I'd just like to tackle stuff in smaller parts 🙂

@flodnv flodnv removed their assignment Nov 24, 2022
@trjExpensify trjExpensify added the Internal Requires API changes or must be handled by Expensify staff label Nov 24, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 24, 2022

A Contributor Manager will be assigned to issue payment via Upwork if we deploy an associated Pull Request to production. Per Contributing.md.

@melvin-bot
Copy link

melvin-bot bot commented Nov 28, 2022

@eVoloshchak, @chiragsalian, @sophiepintoraetz Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot melvin-bot bot added the Overdue label Nov 28, 2022
@eVoloshchak
Copy link
Contributor

Not overdue, progress is being made in #12978

@melvin-bot melvin-bot bot removed the Overdue label Nov 28, 2022
@sophiepintoraetz
Copy link
Contributor

@chiragsalian - did you have an idea on when you'll be able to get the PR out?

@chiragsalian
Copy link
Contributor

So some higher priority deprecate sequence number tasks came up so I pushed this back a little bit. Still aiming for this week.

Also @sophiepintoraetz can you split this part into another issue,

Observe that the group message has a different subtitle message between mobile app and Web

@chiragsalian chiragsalian added the Reviewing Has a PR in review label Dec 3, 2022
@chiragsalian
Copy link
Contributor

Updated the PR with tests and its now in review.

@chiragsalian
Copy link
Contributor

Question,

  • so the problem right now is when two IOU amounts are the same the ordering of items on different devices can show up differently.
  • my solution for this is adding a logic where if IOU amounts are the same then sort by their names so that the ordering is improved on different devices.

So my question is, what if the report display names are also the same,

  1. Should I add logic to handle this scenario, i.e., having a 3rd fallback sort like on lastActionCreated if display names are the same? (which feels a bit like a rabbit hole cause what if lastActionCreated is also the same) Or,
  2. Do nothing for now and add that if it's a problem in the future?

@chiragsalian
Copy link
Contributor

Friendly bump @SofiedeVreese or @tjferriss. Any thoughts on my question above?

@tjferriss
Copy link
Contributor

Do nothing for now and add that if it's a problem in the future?

I agree with doing nothing here as this seems like a rare possibility.

@SofiedeVreese
Copy link
Contributor

cc @sophiepintoraetz , not me 🤣

@chiragsalian
Copy link
Contributor

oh whoops 😅

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Dec 13, 2022
@melvin-bot melvin-bot bot changed the title [$250] BUG: Out of order LHN after split bill between web (staging.new.expensify) and IOS app reported by @chauchausoup [HOLD for payment 2022-12-20] [$250] BUG: Out of order LHN after split bill between web (staging.new.expensify) and IOS app reported by @chauchausoup Dec 13, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 13, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.38-6 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2022-12-20. 🎊

After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@melvin-bot
Copy link

melvin-bot bot commented Dec 13, 2022

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:

  • [@eVoloshchak / @chiragsalian] The PR that introduced the bug has been identified. Link to the PR:
  • [@eVoloshchak / @chiragsalian] 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:
  • [@eVoloshchak / @chiragsalian] 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:
  • [@sophiepintoraetz] A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here:

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Dec 20, 2022
@aimane-chnaif
Copy link
Contributor

@sophiepintoraetz C+ compensation for internal PR review is pending here

@sophiepintoraetz
Copy link
Contributor

Got it! invited you to the job.

@aimane-chnaif
Copy link
Contributor

Got it! invited you to the job.

@sophiepintoraetz accepted. thanks

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 Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests