Only check for user preferences if signed in

[trichoplax]

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 develop branch. Behaviour differs in the final bullet point, as intended:

• Create a post in Q&A (preferences are only required if there are posts)
• Sign in from a private browsing window
• Click on "Posts"
• The network tab of the developer window shows 2 or 3 requests when filtered by "pref" as the preferences are not yet stored in localStorage
• Click on "Posts" again
• The network tab of the developer window shows no requests when filtered by "pref" as the preferences are now stored in localStorage
• Sign out
• Click on "Posts"
• The network tab of the developer window shows no requests when filtered by "pref" (on the develop branch this is because the preferences are still stored in localStorage and the develop branch 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)
• Close the private browsing window to forget the localStorage
• Open in a new private browsing window without signing in
• Click on "Posts"
  • On the develop branch the network tab of the developer window shows 2 or 3 requests when filtered by "pref", even though there is no user signed in
  • On this branch the network tab of the developer window shows no requests when filtered by "pref", because there is no user signed in

JavaScript 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 preferences function and instead explicitly checked for null and key presence (to avoid undefined). This is my personal preference for two reasons:

1. This explicit form makes the intention clear to future readers.
2. Double exclamation mark rejects any arbitrary falsy values, not just null and undefined, giving the possibility of subtle bugs.

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 preferences function to avoid some of the duplication in the if expressions. 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 the preferences function to use QPixel. instead. This now matches the other functions in this file. The fact that this. does not refer to the same place as QPixel. could otherwise be confusing for future readers (as it was for me...). It meant that the _preferences property defined just before the preferences function was never accessed or overwritten, and was entirely redundant. _preferences was instead being created on the global Window object. I've also changed the references to this._preferences in the functions preference and setPreference so that they don't stop working now that _preferences is being read from QPixel rather than Window.

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 preferences function 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 a signed_in data attribute somewhere in the HTML (maybe on body) 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.org uses the same id per user as the other communities on codidact.com. Is there anything else to consider that may make user id unsuitable for this purpose?

[Taeir]

Suggestion:

Suggested change

	if (typeof(value) !== 'undefined') {
	if (value !== undefined) {

[trichoplax]

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.

[Taeir]

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?

[trichoplax]

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 as if (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 ===).

[ArtOfCode-]

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).

[Taeir]

Perhaps !== is better here? Or can this value also be undefined?

[trichoplax]

This comment shows me that you already knew what I just explained in my response to your previous comment. I apologise.

[trichoplax]

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.

[Taeir]

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?

[trichoplax]

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.

[trichoplax]

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.

[ArtOfCode-]

@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.

[Taeir]

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.

[trichoplax]

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.

[cellio]

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!

[trichoplax]

@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...

[ArtOfCode-]

consider:

Suggested change

	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.

[trichoplax]

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 if user does not contain a key called 'error'
• if(!!user.error) is false if either user does not contain a key called 'error', or user does 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.

[ArtOfCode-]

@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.