-
-
Notifications
You must be signed in to change notification settings - Fork 580
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
check whether notif level is undefined, because 0
is falsey
#651
Conversation
…failed Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
src/models/room-state.js
Outdated
@@ -455,7 +455,7 @@ RoomState.prototype.mayTriggerNotifOfType = function(notifLevelKey, userId) { | |||
powerLevelsEvent && | |||
powerLevelsEvent.getContent() && | |||
powerLevelsEvent.getContent().notifications && | |||
powerLevelsEvent.getContent().notifications[notifLevelKey] | |||
powerLevelsEvent.getContent().notifications[notifLevelKey] !== undefined |
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.
and if it's null
, false
, NaN
, or []
? Might be best to change the check to be something like this:
!isNaN(powerLevelsEvent.getContent().notifications[notifLevelKey])
&& typeof(powerLevelsEvent.getContent().notifications[notifLevelKey]) === 'number'
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.
I got impatient and switched it and the others in this file over to a utils method isNumber
I added which does pretty much the above
I kept it consistent with the other checks but did a not NaN previously, will make another pr to change them all after this one. I guess here we're trusting the server to sanitize the values |
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
src/utils.js
Outdated
* Returns whether the given value is a finite number without type-coercion | ||
* | ||
* @param {*} value the value to test | ||
* @return {boolean} whether ot not value is a finite number without type-coercion |
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.
"ot"
I don't think we are doing that here.
I can't see evidence of us doing that before. |
as in a local change before this commit I made, then went for |
so if you set
@room
notif=0, then js-sdk would think its actually 50 :(Signed-off-by: Michael Telatynski 7t3chguy@gmail.com