Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Only check for user preferences if signed in #951

Merged
merged 16 commits into from Jan 17, 2023
Merged

Only check for user preferences if signed in #951

Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
b3bc0d4
Convert 'else if' to 'if' because it depends on data from the previou…
trichoplax Jan 9, 2023
8be131c
Remove check for old preferences schema from over 2 years ago
trichoplax Jan 9, 2023
4ba7d6a
Add early return for most frequent case to reduce processing
trichoplax Jan 9, 2023
aab2313
Remove null check that is now redundant due to early return
trichoplax Jan 9, 2023
c0b42f4
Improve comment to state local variable contains preferences
trichoplax Jan 10, 2023
0b5b4b5
Reject only null & undefined, rather than all falsy local preferences
trichoplax Jan 10, 2023
4949c7f
Remove 'if' block made redundant by early returns
trichoplax Jan 10, 2023
a1bb304
Store preferences on QPixel rather than window ('this')
trichoplax Jan 10, 2023
94ba47e
Suppress preferences retrieval when user is not signed in
trichoplax Jan 10, 2023
33abac3
Extract duplicated localStorage preferences code into a function
trichoplax Jan 11, 2023
306a554
Use unique localStorage key per user to avoid corruption
trichoplax Jan 11, 2023
1bba8a3
Make 'if' explicit and include mandatory braces for code standards
trichoplax Jan 11, 2023
81caadb
Add the commas I forgot between new functions in QPixel object
trichoplax Jan 11, 2023
6a4b147
Change preference function to use early return
trichoplax Jan 11, 2023
eba672d
Extract common code into separate functions
trichoplax Jan 12, 2023
4582d43
Add JavaScript documentation comments for new functions
trichoplax Jan 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
109 changes: 67 additions & 42 deletions app/assets/javascripts/qpixel_api.js
View file
Open in desktop
Expand Up @@ -158,7 +158,9 @@ window.QPixel = {
* @returns {Promise<Object>} a JSON object containing user details
*/
user: async () => {
if (QPixel._user) return QPixel._user;
if (QPixel._user != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return QPixel._user;
}
const resp = await fetch('/users/me', {
credentials: 'include',
headers: {
Expand All @@ -177,29 +179,28 @@ window.QPixel = {
* @returns {Promise<Object>} a JSON object containing user preferences
*/
preferences: async () => {
if (this._preferences == null && !!localStorage['qpixel.user_preferences']) {
this._preferences = JSON.parse(localStorage['qpixel.user_preferences']);

// If we don't have the global key, we're probably using an old preferences schema.
if (!this._preferences.global) {
delete localStorage['qpixel.user_preferences'];
this._preferences = null;
}
// Do not attempt to access preferences if user is not signed in
const user = await QPixel.user();
if ('error' in user) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return null;
}
else if (this._preferences == null) {
// If they're still null (or undefined) after loading from localStorage, we're probably on a site we haven't
// loaded them for yet. Load from Redis via AJAX.
const resp = await fetch('/users/me/preferences', {
credentials: 'include',
headers: {
'Accept': 'application/json'
}
});
const data = await resp.json();
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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return QPixel._preferences;
}
return this._preferences;
// Early return the preferences from localStorage unless null or undefined
const key = await QPixel._preferencesLocalStorageKey();
const localStoragePreferences = (key in localStorage)
? JSON.parse(localStorage[key])
: null;
if (localStoragePreferences != null) {
QPixel._preferences = localStoragePreferences;
return QPixel._preferences;
}
// Preferences are still null (or undefined) after loading from localStorage, so we're probably on a site we
// haven't loaded them for yet. Load from Redis via AJAX.
await QPixel._fetchPreferences();
return QPixel._preferences;
},

/**
Expand All @@ -212,26 +213,16 @@ window.QPixel = {
let prefs = await QPixel.preferences();
let value = community ? prefs.community[name] : prefs.global[name];

// Deliberate === here: null is a valid value for a preference, but undefined means we haven't fetched it.
// If we haven't fetched a preference, that probably means it's new - run a full re-fetch.
if (value === undefined) {
const resp = await fetch('/users/me/preferences', {
credentials: 'include',
headers: {
'Accept': 'application/json'
}
});
const data = await resp.json();
localStorage['qpixel.user_preferences'] = JSON.stringify(data);
this._preferences = data;

prefs = await QPixel.preferences();
value = community ? prefs.community[name] : prefs.global[name];
return value;
}
else {
// Note that null is a valid value for a preference, but undefined means we haven't fetched it.
if (typeof(value) !== 'undefined') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion:

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return value;
}
// If we haven't fetched a preference, that probably means it's new - run a full re-fetch.
await QPixel._fetchPreferences();

prefs = await QPixel.preferences();
value = community ? prefs.community[name] : prefs.global[name];
return value;
},

/**
Expand All @@ -258,11 +249,45 @@ window.QPixel = {
console.error(resp);
}
else {
this._preferences = data.preferences;
localStorage['qpixel.user_preferences'] = JSON.stringify(this._preferences);
await QPixel._updatePreferencesLocally(data.preferences);
}
},

/**
* Get the key to use for storing user preferences in localStorage, to avoid conflating users
* @returns {Promise<string>} the localStorage key
*/
_preferencesLocalStorageKey: async () => {
const user = await QPixel.user();
return `qpixel.user_${user.id}_preferences`;
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
* Update local variable _preferences and localStorage with an AJAX call for the user preferences
* @returns {Promise<void>}
*/
_fetchPreferences: async () => {
const resp = await fetch('/users/me/preferences', {
credentials: 'include',
headers: {
'Accept': 'application/json'
}
});
const data = await resp.json();
await QPixel._updatePreferencesLocally(data);
},

/**
* Set local variable _preferences and localStorage to new preferences data
* @param data an object, containing the new preferences data
* @returns {Promise<void>}
*/
_updatePreferencesLocally: async data => {
QPixel._preferences = data;
const key = await QPixel._preferencesLocalStorageKey();
localStorage[key] = JSON.stringify(QPixel._preferences);
},

/**
* Get the word in a string that the given position is in, and the position within that word.
* @param splat an array, containing the string already split by however you define a "word"
Expand Down