Only check for user preferences if signed in #951
Conversation
| } | ||
| else { | ||
| // Note that null is a valid value for a preference, but undefined means we haven't fetched it. | ||
| if (typeof(value) !== 'undefined') { |
There was a problem hiding this comment.
Suggestion:
| if (typeof(value) !== 'undefined') { | |
| if (value !== undefined) { |
There was a problem hiding this comment.
In general I prefer to use typeof because it is more robust. Without it, direct comparison with undefined will result in a ReferenceError if value does not exist. For example:
Defining a variable as undefined causes no problem. The following outputs 'undefined' as expected:
const value = undefined;
if (value !== undefined) {
console.log('defined');
} else {
console.log('undefined');
}Declaring a variable but not defining it also causes no problem. The following also outputs 'undefined' as expected:
let value;
if (value !== undefined) {
console.log('defined');
} else {
console.log('undefined');
}Not declaring a variable causes a problem as it cannot be accessed in order to process the conditional expression of the if block. The following might be expected to output 'undefined', but it actually results in a ReferenceError:
if (value !== undefined) {
console.log('defined');
} else {
console.log('undefined');
}Using typeof avoids this problem, as it can cope with undeclared variables. The following outputs 'undefined':
if (typeof(value) !== 'undefined') {
console.log('defined');
} else {
console.log('undefined');
}The line you commented on follows a declaration of value, so in this case the robustness is not needed, but I wanted to make it robust to any future changes too.
| */ | ||
| user: async () => { | ||
| if (QPixel._user) return QPixel._user; | ||
| if (QPixel._user != null) { |
There was a problem hiding this comment.
Inquiry: The old code was doing something equivalent (perhaps even slightly better, since it also handles undefined?). Was there a reason for changing this part?
There was a problem hiding this comment.
My main reason for changing this was to comply with the code standards that forbid omitting braces from if statements:
Braces must not be omitted for single-line statements following a control flow expression (e.g.
if/else,for,while).
Adding in the != null is not essential, I just prefer explicit comparisons for readability. The file currently has a mix of both styles and I'm happy to go with whichever is preferred.
Even the addition of the braces is not essential. I was just making the code a little more code standards compliant while the file was already being changed.
Incidentally (not related to this change), != null still handles undefined (but not undeclared variables). This is mentioned in the code standards:
Equality checks must use strict equality checking (
===). The only exception is when checking for null/undefined values, which may be written asif (value == null).
Note that if (value == null) will evaluate to true for both null and undefined. I find this counterintuitive, so if they were my code standards I would forbid this usage, and make all comparisons strict (!== or ===).
There was a problem hiding this comment.
Perfectly happy with this as is - just to mention that if you have something like this check that you want to make explicit, you can force it to a boolean: if (QPixel._user) becomes if (!!QPixel._user).
| localStorage['qpixel.user_preferences'] = JSON.stringify(data); | ||
| this._preferences = data; | ||
| // Early return for the most frequent case (local variable already contains the preferences) | ||
| if (QPixel._preferences != null) { |
There was a problem hiding this comment.
Perhaps !== is better here? Or can this value also be undefined?
There was a problem hiding this comment.
This comment shows me that you already knew what I just explained in my response to your previous comment. I apologise.
There was a problem hiding this comment.
This value should never be undefined in theory, but in practice it was often undefined before the change from this._preferences to QPixel._preferences (elsewhere in this pull request). If any future change introduces the possibility of it being undefined, I wouldn't want to return that undefined, but instead proceed to trying to restore the preferences from localStorage.
| _preferencesLocalStorageKey: async () => { | ||
| const user = await QPixel.user(); | ||
| return `qpixel.user_${user.id}_preferences`; | ||
| }, |
There was a problem hiding this comment.
Since you change the local storage key or a user to a different value with this change, I presume old values will be ignored and no longer used. What will the effect of that be? Will those values correctly be loaded from the servers, or do the settings set disappear?
There was a problem hiding this comment.
If I have understood correctly, the localStorage version of _preferences is intended to always match the server.
The setPreference function is the only way to change a preference, and it always updates the server first, and then localStorage. So fetching the preferences from the server should always give the most up to date preferences.
In cases where two or more users share a browser, they may historically have overridden each other's localStorage preferences as this only existed once per browser. In these cases their preferences will now revert to their correct preferences from the server. If a user has become accustomed to incorrect preferences, this will appear to them as a change. Is this something we need to avoid / warn about? I don't know how many users (if any) share a browser with another user.
There was a problem hiding this comment.
Since this is my first code change to QPixel, if there's any doubt about the consequences it might be worth a second (third?) opinion. My understanding may be incorrect.
There was a problem hiding this comment.
@trichoplax your understanding sounds correct to me. The only other effect that I can think this will have is to force a re-fetch of every user's preferences the first time they visit after this is deployed. That's not a major load.
|
Besides the few minor questions/comments, I think your style of using early returns and avoiding some of the javaScript "strange features that can have unintended side effects" is very good. |
|
Thanks very much for the review comments @Taeir. I've added my reasons but let me know whatever parts you still want me to change. |
|
Thanks for the detailed explanation of approach up front. I'm not qualified to review these changes, but I found that helpful and learned stuff, and I assume it helps reviewers in general. Understanding the "why" of a change is great! |
|
@cellio thank you! I'm glad it helped. It also means that when I get something wrong it hopefully helps the reviewer see why I got it wrong, which makes explanations easier... |
| } | ||
| // Do not attempt to access preferences if user is not signed in | ||
| const user = await QPixel.user(); | ||
| if ('error' in user) { |
There was a problem hiding this comment.
consider:
| if ('error' in user) { | |
| if (!!user.error) { |
This is entirely opinion, but I find the 'property' in object form confusing - on the face of it, it looks like a string operation of some sort, when it's not. The above is an alternative.
There was a problem hiding this comment.
I know the syntax of 'error' in user is a bit odd (especially coming from Python, where in can be used for checking for substrings, and where I can write 'error' in user.keys() to make it clearer what's being checked).
I still prefer 'error' in user over the cast to Boolean for the avoidance of ambiguity:
if('error' in user)is only false ifuserdoes not contain a key called'error'if(!!user.error)is false if eitheruserdoes not contain a key called'error', oruserdoes contain a key called'error'whose corresponding value is anything falsy
In this particular case it's unlikely user.error would ever contain something falsy, but in general that's my reason for preferring the explicit key check.
Just checked out of curiosity and it is possible to be even more explicit with JavaScript, similarly to Python but a little more verbose:
if (Object.keys(user).includes('error'))
I don't feel strongly enough about this particular case to argue against any of the mentioned forms though.
| _preferencesLocalStorageKey: async () => { | ||
| const user = await QPixel.user(); | ||
| return `qpixel.user_${user.id}_preferences`; | ||
| }, |
There was a problem hiding this comment.
@trichoplax your understanding sounds correct to me. The only other effect that I can think this will have is to force a re-fetch of every user's preferences the first time they visit after this is deployed. That's not a major load.
Addresses #947 and #948 plus some other things I noticed in the surrounding code.
Please let me know if there are any changes needed.
Testing
I'm using Firefox so I refer to "private browsing". In Chromium based browsers this may instead be referred to as "incognito".
I have tested with a local installation of QPixel as follows, on both this branch and the
developbranch. Behaviour differs in the final bullet point, as intended:developbranch this is because the preferences are still stored in localStorage and thedevelopbranch looks in localStorage regardless of whether a user is signed in; on this branch this is because we no longer make the AJAX request for preferences when a user is not signed in)developbranch the network tab of the developer window shows 2 or 3 requests when filtered by "pref", even though there is no user signed inJavaScript documentation comments
I've endeavoured to keep to the same style used for the comments on existing functions. Please let me know if anything should be different.
Double exclamation marks
I've removed the use of double exclamation mark from the
preferencesfunction and instead explicitly checked fornulland key presence (to avoidundefined). This is my personal preference for two reasons:I prefer to avoid double exclamation mark outside of code golf, but I understand that it is a widely used idiom, so let me know if it is to be preferred for QPixel code. Either way, would it be worth mentioning in the code standards whether double exclamation mark is encouraged, discouraged, or optional?
Early returns
I've switched to using early returns in the
preferencesfunction to avoid some of the duplication in theifexpressions. The previous function,user, already uses an early return, so I'm guessing this is acceptable, but I know that early returns can divide opinions. Would it be worth mentioning in the code standards whether early returns are encouraged, discouraged, or optional? I personally find early returns easier to reason about as they keep the reasoning more local, and I expect them to be slightly less bug prone.No longer use
this.I have changed all uses of
this.in thepreferencesfunction to useQPixel.instead. This now matches the other functions in this file. The fact thatthis.does not refer to the same place asQPixel.could otherwise be confusing for future readers (as it was for me...). It meant that the_preferencesproperty defined just before thepreferencesfunction was never accessed or overwritten, and was entirely redundant._preferenceswas instead being created on the globalWindowobject. I've also changed the references tothis._preferencesin the functionspreferenceandsetPreferenceso that they don't stop working now that_preferencesis being read fromQPixelrather thanWindow.Check if user is signed in
I've added in a check for whether the user is signed in so that we no longer make dozens of AJAX requests when loading a page. I've put the check at the beginning of the
preferencesfunction instead of only before the AJAX request, because currently the preferences are being fetched from localStorage even after the user signs out. I'm assuming that we do not want a user's preferences to continue to apply after they sign out.Are there other places that should also check whether there is a logged in user before making a request? What about the request for
QPixel._user? This only makes a single redundant AJAX request rather than dozens, but we could still avoid it. Would it be worth adding asigned_indata attribute somewhere in the HTML (maybe onbody) so that the user data AJAX request is only made when signed in? Would any such changes be best raised as separate issues?Remove old schema check
Two years ago a check was added in case the localStorage preferences were using an old preferences schema. I've removed this as it seems redundant now. Let me know if you would prefer to keep it around just in case there is a user who hasn't visited the site in the last two years but still has a browser containing the old preferences schema in its localStorage. There will be no need to keep it around if the following change to the localStorage key is accepted.
Unique localStorage key per user
I suspect the current code will still load the previous user's preferences from localStorage even if they sign out and a different user signs in, giving them the wrong preferences. To prevent this I have included the user id in the localStorage key name so multiple users don't clash and they always get their own preferences. Should we also check for an old style localStorage preferences key (without the user id) and delete it if found, to tidy up the user's machine? Possibly as temporary code that can be removed after enough time to have deleted all the old keys?
I have assumed here that the user id will be unique and consistent across all places the preferences apply to. I've checked that Collab on
codidact.orguses the same id per user as the other communities oncodidact.com. Is there anything else to consider that may make user id unsuitable for this purpose?