-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Fix: Attachment modal is not closed when click notification #30968
Conversation
@tienifr Please ping me once the PR is ready for review. |
@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 Can we continue? |
I didn't review because I thought this was still blocked. @tienifr Are you still facing issues with the notifications? I think it makes sense to add videos from all the platforms? |
There's no way we can test notification on native. |
@mananjadhav Or we can request build to test on physical devices. |
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.
The change looks fine, and it's best that we test with the adhoc build.
@joelbettner Can you help trigger the adhoc build to test this one? |
@joelbettner Can you help trigger adhoc build? Thanks! |
We might need to include this test case: #31625 to cover the case when app was quit and notification was received in the background. |
@joelbettner did you get a chance to look at the previous comments for adhoc build? |
This comment has been minimized.
This comment has been minimized.
Sorry @mananjadhav and @tienifr. I was OOO last week (and one unexpected day of being OOO because I was sick). Catching up on things now. Thanks for triggering that build @tgolen. |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
@tienifr Are you able to run this the adhoc builds? The build are not loading for me. Screen.Recording.2023-12-01.at.1.28.33.AM.mov |
@mananjadhav It's working fine one my side. |
@mananjadhav This is what I faced when receiving notifications on mWeb Chrome, both production and Seems like notification is broken on mWeb. Also I could not test on physical iOS as it required Apple developer's account to run adhoc build. Can you suggest any process for that? Or we can ask QA team for help. But it worked fine on Android. I've updated screenshot. |
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid-attachment-notification.mp4Android: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / Safariweb-notification-modal.movMacOS: Desktopdesktop-notification-modal.mov |
@tienifr Can you please merge the latest main and resolve conflicts? @joelbettner I've tried multiple times, and different ways to enable notifications on my emulators but it's in vain. Can you please help with the remaining platforms? or if you can trigger another adhoc build to see it works on the latest changes? |
I've replied earlier here that:
|
Yes agreed. Hence I've asked @joelbettner to see if they can help. I can still see merge conflicts @tienifr. |
@mananjadhav GH server was having issues several minutes ago. That's why you can't see the latest changes. They've all been resolved. |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
@mananjadhav @tienifr I've just posted the adhoc builds. |
@joelbettner Do adhoc builds support push notifications? |
I asked in Slack, and no, push notifications do not work in ad-hoc builds. |
src/libs/Notification/PushNotification/subscribeToReportCommentPushNotifications.ts
Show resolved
Hide resolved
@mananjadhav you should be able to test push notifications in dev on Android. Correct @arosiclair? |
Tested again today and worked fine: video_2024-01-12_13-06-27.mp4@mananjadhav Did you allow notifications for the Adhoc app? |
Okay I finally managed to test this on my friend's android device. Uploaded the video. |
@joelbettner Can we please move this forward? @tienifr Can you sync the latest main? |
All done. |
Looks good. Just need the last items on the checklist checked off, @mananjadhav. |
@mananjadhav Can you check off the rest platforms? We have problems testing those so I think that's fine. We can delegate the work to QA once it's on staging. |
@joelbettner @tienifr Done. |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to production by https://github.com/thienlnam in version: 1.4.31-7 🚀
|
Details
Attachment modal is not closed when click notification, and later we can't reopen that attachment. This PR closes any modal when navigate from notification.
Fixed Issues
$ #30446
PROPOSAL: #30446 (comment)
Tests
Offline tests
NA
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
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
Android: Native
Untitled.mp4
MacOS: Chrome / Safari
Screen.Recording.2023-11-08.at.15.25.45-compressed.mov
MacOS: Desktop
Screen.Recording.2023-11-08.at.15.28.30-compressed.mov