Write platform specific mute settings to backend#50493
Merged
tgolen merged 16 commits intoExpensify:mainfrom Oct 29, 2024
Merged
Write platform specific mute settings to backend#50493tgolen merged 16 commits intoExpensify:mainfrom
tgolen merged 16 commits intoExpensify:mainfrom
Conversation
tgolen
requested changes
Oct 10, 2024
src/libs/actions/User.ts
Outdated
| Onyx.merge(ONYXKEYS.USER, {isMutedAllSounds}); | ||
| function togglePlatformMute(platform: Platform, mutedPlatforms: Platform[]) { | ||
| const isPlatformMuted = mutedPlatforms?.includes(platform); | ||
| const newMutedPlatforms = isPlatformMuted ? mutedPlatforms.filter((mutedPlatform) => mutedPlatform !== platform) : [...mutedPlatforms, platform]; |
Contributor
There was a problem hiding this comment.
Ah, sorry. I guess this was something I didn't explicitly say. It's actually an object like this:
{
'web': true,
'desktop': true,
}
and if a platform is muted, it is removed from the object entirely.
c3024
commented
Oct 10, 2024
|
|
||
| const optimisticData: OnyxUpdate[] = [ | ||
| { | ||
| onyxMethod: Onyx.METHOD.SET, |
Contributor
Author
There was a problem hiding this comment.
Is it better to use MERGE here without passing mutedPlatforms to this function togglePlatformMute?
Contributor
There was a problem hiding this comment.
For NVPs, the server uses SET, so it's probably best to use SET here as well. That will keep things consistent.
Contributor
|
Hi! The API has been deployed to staging, so you should be able to finish this up now. |
Contributor
Author
|
When we mute some platforms and then unmute all of them, backend seems to send an empty array in Pusher to other devices (here it's and iOS emulator) and Onyx complains.
|
Contributor
Author
|
|
Contributor
Ah, this is because of PHP's JSON encoding which converts the empty object to an empty array. Very troublesome. I think it might be better to set the value to
It looks like it's the same for all the NVPs, so I think that's OK for now. I agree it's not the most consistent, but I don't really want to change the behavior of all the NVPs in |
Contributor
|
OK, I merged a fix for the empty array. It should deploy to staging tomorrow. Here, you can see me making the request in PostMan and then NewDot receiving the Onyx update. I hope that will work for you!
|
Contributor
Author
|
Are
|
Contributor
|
Hm, no... They shouldn't be. I can look into that. Let's not let that block this PR though. |
Contributor
Author
🤔 But, without fixing that we cannot reliably test this PR.
Screen.Recording.2024-10-23.at.8.48.01.PM.mov |
Contributor
|
Yeah, I understand it would be a bug on mobile web, but mobile web was also the least important (someone even proposed not supporting mobile web at all). It's up to you though. I can probably get a fix written for it today or tomorrow, and that would be merged and deployed by Monday if you want to wait. |
Contributor
Author
|
Thanks. Going ahead! |
Contributor
|
@c3024 I looked into that bug and found some interesting things. I wasn't able to reproduce it using PostMan. These are the four requests I made:
and here were the OnyxUpdates sent to the client via pusher (which look correct)
When I looked at the logs for your request ID
ConclusionIt looks like this is a frontend issue, so you might want to check the actual network requests to see what parameter is sent to the server. |
Contributor
Author
|
Apparently, we enhance the request parameters and add platform. So, App/src/libs/Network/enhanceParameters.ts Line 44 in 866ce4b We need to rename the |
Contributor
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid-app-2024-10-24_10.43.04.mp4Android: mWeb Chromeandroid-chrome-2024-10-24_10.47.28.mp4iOS: Nativeios-app-2024-10-24_11.07.16.mp4iOS: mWeb Safariios-safari-2024-10-24_11.09.48.mp4MacOS: Chrome / Safaridesktop-chrome-2024-10-24_10.32.13.mp4MacOS: Desktopdesktop-app-2024-10-24_10.38.21.mp4 |
Contributor
Ah, phooey, I was afraid of that. OK, how about naming it |
Contributor
Author
|
Sounds good! |
Contributor
|
Great, I've submitted a PR to the server for that so it should be live and on staging by tomorrow I expect. |
Contributor
|
@c3024 Were you able to retest with the staging server? |
Contributor
Author
|
Backend PR has not been deployed to Staging yet. mWebNotYetOnStaging.mp4 |
Contributor
|
Yeah, it still hasn't been deployed yet. I'm hopeful that will happen today. I'll try to ping here as soon as I see it deployed. |
jjcoffee
approved these changes
Oct 29, 2024
Contributor
jjcoffee
left a comment
jjcoffee
left a comment
There was a problem hiding this comment.
Tests nicely using the staging server!
Contributor
|
Cool, thanks! I see the backend PR was deployed about 17 hours ago. |
tgolen
approved these changes
Oct 29, 2024
Contributor
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Contributor
Performance Comparison Report 📊Significant Changes To Duration
Show details
Meaningless Changes To DurationShow entries
Show details
|
github-actions
bot
added
the
DeployBlockerCash
Contributor
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
Contributor
|
🚀 Deployed to staging by https://github.com/tgolen in version: 9.0.56-0 🚀
|
Contributor
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.0.56-9 🚀
|
This was referenced Nov 6, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Details
Mute settings are currently saved only locally, and sometimes they get reset without any user action. This PR addresses the front-end implementation for saving platform-specific mute settings to the backend.
Fixed Issues
$ #49087
PROPOSAL: #49087 (comment)
Tests
Offline tests
NA
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Screen.Recording.2024-10-23.at.10.21.50.PM.mov
Android: mWeb Chrome
Not tested because there are still backend changes required.
iOS: Native
muteiOS.mov
iOS: mWeb Safari
Not tested because there are still backend changes required.
MacOS: Chrome / Safari
muteChrome.mov
MacOS: Desktop
muteDesktop.mov