-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[No QA] Created IOU preview component within chat #2112
[No QA] Created IOU preview component within chat #2112
Conversation
report: PropTypes.shape({ | ||
|
||
// IOU report ID associated with current report | ||
iouReportID: PropTypes.number, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to just pass the reportID at this stage, and subscribe to the full report Onyx further down the Component chain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I can pass just iouReportID here. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this was addressed and I think @Julesssss was suggesting that we pass the iouReportID
directly instead of subscribing to the entire report object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good so far.
Just to let you know, until now group chat IOU actions have been of type ADDCOMMENT
, but we've changed this to be an IOU
action, which should make it easier to render the Quote component in group chats.
[This change is currently on staging.expensify.com]
Do we use the same API method for creating transactions in group chat? Should I worry about different transaction types, request money and split bill? |
Good question, sorry I didn't share this to begin with:
|
Hey @Julesssss When I called Is there anywhere else that I can find settled IOUs, so I can display the paid amount? |
Hey @tugbadogan, we won't show the lower preview section (component B) if the balance is 0. Sorry, I forgot to mention this in the original description. This should resolve the problem of displaying the Paid amount. However, I realise now that this same issue will apply to the view details link -- let me investigate and get back to you. |
Actually ignore that last part, the view details link isn't part of this PR 🙃 |
I updated the PR. It's ready for review. |
Nice work! Here are some things I've noticed:
Also, we should just show a users first name for the requested quote message (but this is a separate backend issue) |
report: { | ||
key: ({reportID}) => `${ONYXKEYS.COLLECTION.REPORT}${reportID}`, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we already have reportID
, isn't this unnecessary -- now that we simply pass reportID
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is chatReportID
but we need iouReportID
and we need to fetch chatReport first to use report.iouReportID
in child component's withOnyx. I'm now passing report to use report.hasOutstandingIOU
anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we pass the iouReportID
or anything else we need directly?
👍
These action messages are generated on server side. Should I modify the message on the client side?
I fixed these issues 👍
Agreed, this is similar to 'for' issue. It is better to fix this on backend. |
No, but after the for there should be the comment of the request, I think that we are not removing the
Will handle this too. |
I ran through the full flow on web, Android, and iOS. I can see the last issues I mentioned have been fixed, but noticed one remaining issue: When I pay/decline/create an IOU transaction I see the storage being updated locally. But, we're not updating the current page. I think this is because we're updating the wrong data store. I see the IOUReport being persisted as a regular report under |
Just wanted to leave the test flow that I ran though here, in case anyone else tests this over the UK easter bank holiday:
IOU API requestsCreateIOUReport
CreateIOUSplit
CreateChatReport
Pay IOU
Decline IOU
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately there seems to have been a slight regression with the latest changes. As the user who creates a group chat, sends a message, and then an IOU split for that group, we should see the following Onyx data created for each individual 1:1
report_940
reportActions_940
reportIOUs_941 --> This is being created as reportIOUs_940
So we're creating the chat reports, but not creating or linking to the IOUReport. Other than this and the issue where IOUReports are not retrievable via the push data, everything else is testing well.
To round up the feedback:
Once that's done I'll ping the remaining reviewers and run the test flows once more. |
I tested split bill flow for a group chat. The problem is caused by push notification being sent with
In this example |
@Julesssss And we are getting push notifications for these IOU report IDs. When we call |
Great spot! Thanks for pointing this out... I've just fixed this and will report back once this change has gone live on prod. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. Not sure what happened with the last commit, but thanks for reverting.
I've tested the flows against this internal Web-E PR which fixes the push notification problem we introduced. It also requires pulling the latest Auth changes and rebuilding.
I'll update the test steps to let QA know what is testable at this stage. I think for now this should be tested internally.
@NikkiWines @marcaaron @AndrewGable -- not much has changed since your previous reviews, but I'll leave the PR open for a couple of hours in case you have any last thoughts. |
Seems you've got conflicts @tugbadogan, can you resolve them please? |
Resolved the conflicts. |
🚀 Deployed to production in version: 1.0.39-5🚀
|
@iwiznia @Julesssss
Details
Fixed Issues
Fixes #2027
Tests
QA Steps
For now, this is only testable internally. The cancel/decline cases will be tested as part of the IOU Details PR, which will introduce that functionality. For now, I have been testing this with API calls.
amount
andY owes X
message.amount
,Y owes X
message andPay
button.Tested On
Screenshots
Web
Owner / Manager Views
Group Chat View
Mobile Web
Owner / Manager Views
Desktop
Owner / Manager Views
iOS
Owner / Manager Views
Android
Owner / Manager Views