-
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
Write platform specific mute settings to backend #50493
Conversation
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]; |
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, 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.
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.
Done!
|
||
const optimisticData: OnyxUpdate[] = [ | ||
{ | ||
onyxMethod: Onyx.METHOD.SET, |
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.
Is it better to use MERGE
here without passing mutedPlatforms
to this function togglePlatformMute
?
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.
For NVPs, the server uses SET, so it's probably best to use SET here as well. That will keep things consistent.
Hi! The API has been deployed to staging, so you should be able to finish this up now. |
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 |
Hm, no... They shouldn't be. I can look into that. Let's not let that block this PR though. |
🤔 But, without fixing that we cannot reliably test this PR.
Screen.Recording.2024-10-23.at.8.48.01.PM.mov |
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. |
Thanks. Going ahead! |
@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. |
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 |
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 |
Ah, phooey, I was afraid of that. OK, how about naming it |
Sounds good! |
Great, I've submitted a PR to the server for that so it should be live and on staging by tomorrow I expect. |
@c3024 Were you able to retest with the staging server? |
Backend PR has not been deployed to Staging yet. mWebNotYetOnStaging.mp4 |
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. |
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.
Tests nicely using the staging server!
Cool, thanks! I see the backend PR was deployed about 17 hours ago. |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Performance Comparison Report 📊Significant Changes To Duration
Show details
Meaningless Changes To DurationShow entries
Show details
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
🚀 Deployed to staging by https://github.com/tgolen in version: 9.0.56-0 🚀
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.0.56-9 🚀
|
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 Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.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
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