-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Handle starting a chat with secondary login when chat with primary exists #22147
Conversation
@mananjadhav 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] |
@mananjadhav FYI the back-end changes for this are only on staging right now, so you'll need to be on staging to test this. I'm also having problems with my android emulator, so haven't uploaded those videos yet, but I will as soon as I get it working. |
@mananjadhav FYI the the back-end PR is on production, so please review when you have a chance. |
src/libs/actions/Report.js
Outdated
// We should clear out the optimistically created report and re-route the user to the preexisting report. | ||
if (report && report.reportID && report.preexistingReportID) { | ||
Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`, null); | ||
const currentRoute = navigationRef.current && navigationRef.current.getCurrentRoute(); |
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.
Could we have used Navigation.getActiveRoute
?
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.
Ah, you're right we could.
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.
Though Navigation.getActiveRoute
returns a string like /r/1195783708973664
which we'd have to copmare like this:
if (Navigation.getActiveRoute === `/r/${report.reportID}`) {
...
}
Which doesn't actually feel better to me. So I think I prefer it the way it is. What do you think @mananjadhav?
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.
Alternatively, I could create a Navigation.getActiveRouteObject
function.
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.
Yeah that sounds good, although I am on the fence, as I can see other places where we're importing navigationRef
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.
Yeah, I think I prefer to just leave it as-is.
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.
@Beamanator do you have an opinion?
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.
Hmm if we use getActiveRoute
we could do:
if (Navigation.getActiveRoute().endsWith(`/${report.reportID}`) {
Which doesn't really seem that bad to me, I do feel like that looks better than the current implementation, especially since we're reusing a useful looking function :D
Also I see navigationRef.current.getCurrentRoute().path is undefined in the first navigation.
in this comment:
App/src/libs/Navigation/Navigation.js
Lines 205 to 206 in ecf0ec2
* Building path with getPathFromState since navigationRef.current.getCurrentRoute().path | |
* is undefined in the first navigation. |
Which kinda scares me :D
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.
Ahhh fair points. Ok, I'm down with that approach, will update.
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.
Updated!
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.
Change looks good, I'll have it tested today.
@cristipaval 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] |
Reviewer Checklist
Screenshots/VideosWebMobile Web - ChromeMobile Web - SafariDesktopiOSAndroid |
@mananjadhav can you please test on all platforms? 🙏 |
Yes @Beamanator. I'll be doing that today. Had some issues with system. |
@puneetlath I tried but this doesn't seem to be redirecting me to the preexistingReportID. The API does return the correct preexistingReportID, but the App doesn't navigate. web-pre-existing-chat.mov |
Interesting. @mananjadhav so it's re-directing you to some other random report?
|
No it doesn't redirect to any report. It stays at the current active route. I logged and saw the rest of the logic of
|
@mananjadhav I'm confused. In the video you uploaded you open this chat: |
@puneetlath I manually opened the chat with the reportID |
@puneetlath did you get a chance to look at the previous comment? |
Hi! |
Hi @mananjadhav I was able to reproduce the same behavior. |
Thanks @pasyukevich. |
Closing this in favor of #24320 |
Details
This PR handles the case where the client attempts to create a new DM/group-DM with users they already have a DM/group-DM with. This can happen if:
Fixed Issues
$ #15511
Tests
Offline tests
This cannot be done offline since it requires an API response. However if you create the report while offline, once you come back online, step 4 from above should still happen.
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
2023-07-11_11-38-52.mp4
Mobile Web - Chrome
64af43495ba5d7.32365149.pdf
64af434543b660.65681710.pdf
64af43474c2769.78280582.pdf
Mobile Web - Safari
2023-07-12_19-27-08.mp4
Desktop
2023-07-12_19-15-23.mp4
iOS
2023-07-12_19-28-39.mp4
Android
2023-07-12_20-06-36.mp4