Skip to content

Conversation

@scrivna
Copy link
Contributor

@scrivna scrivna commented Sep 23, 2019

Closes #15399 - Saving audioNotificationValue from pushNotificationsFlexTab stores incorrect value

The incorrect value was being saved to the DB. The "value" from the UI dropdown has a concatenated string (it looks like "beep Beep") only the first portion of this string should be saved to the DB however the entire string was being saved, causing the notification to break. My fix splits the string and saves only the first portion.

Closes #15427 - Setting a channel's audio sound to default breaks audio notifications

Separate but related issue. Subscription "audioNotificationValue" field is set to "0" when it is switched back to "default" from another value (I don't know why this is set to 0 rather than removed or set to null, but it is). The audio playing code on the client UI doesn't recognise a value of "0" as being allowed, so it tries to play a sound named "audio#0" instead of using the path designed for the default sound, my fix recognises a "0" as the same as a "not set" value and uses the account default.

…d incorrectly. Closes RocketChat#15427 Setting a channel's audio sound to default breaks audio notifications
@scrivna scrivna changed the base branch from develop to release-candidate September 23, 2019 19:34
@scrivna scrivna changed the base branch from release-candidate to develop September 23, 2019 19:36
@scrivna
Copy link
Contributor Author

scrivna commented Sep 23, 2019

Both of these bugs and a further issue #13931 are related to this same set of changes made by this commit 82aceab

Someone changed this dropdown to have a value of "{id} {name}" vs the original "{id}" only - this has had a bunch of knock-on effects.
Can we revert this change and turn it back to only using the {id} - the reason I ask is because #13931 is difficult to fix as the form defaults are populated by the subscription which only has the {id} set.
If we keep that commit and my PR also, the string value of "0" for audioNotificationValue is going to be throughout the database and needs to be dealt with everywhere in the code, if we revert the original commit then my PR is not needed, but we would have to run a migration on the DB to remove the string values that commit has placed in the DB in error.

@ggazzo ggazzo added this to the 2.2.0 milestone Sep 24, 2019
@scrivna
Copy link
Contributor Author

scrivna commented Sep 24, 2019

@ggazzo thanks for adding this to the milestone, any thoughts on my comment above about the corrupted DB values?

@tassoevan tassoevan added feat: notification area: ui Touches the code on client side type: bug labels Oct 17, 2019
@tassoevan tassoevan changed the title [Fix] Closes #15399 & Closes #15427 - Issues saving audio notifications [FIX] Issues saving audio notifications Oct 17, 2019
@ggazzo ggazzo merged commit 4ca572c into RocketChat:develop Oct 18, 2019
@rodrigok rodrigok mentioned this pull request Oct 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: ui Touches the code on client side feat: notification type: bug

Projects

None yet

3 participants