diff --git a/docs/refactor-todo.md b/docs/refactor-todo.md index c0329bc2..04da4929 100644 --- a/docs/refactor-todo.md +++ b/docs/refactor-todo.md @@ -10,6 +10,7 @@ queryParams of the piece_list_store should all be reflected in the url and not a - Use classNames plugin instead of if-conditional-classes - Instead of using `currentUser && currentUser.email` in an validation that checks whether we user is logged in or now, in the `UserStore` on login we set a boolean property called `isLoggedIn` that can then be used instead of `email` - Refactor AclProxy to be a generic hide/show element component. Have it take data input and a validation function to assess whether it should show or hide child elements. Move current Acl checks to another place, eg. acl_utils.js. +- Convert all fetchers to [alt.js's sources](http://alt.js.org/docs/async/) # Refactor DONE - Refactor forms to generic-declarative form component ✓ diff --git a/js/actions/user_actions.js b/js/actions/user_actions.js index 3780a802..a661b8de 100644 --- a/js/actions/user_actions.js +++ b/js/actions/user_actions.js @@ -1,37 +1,18 @@ 'use strict'; import { altUser } from '../alt'; -import UserFetcher from '../fetchers/user_fetcher'; class UserActions { constructor() { this.generateActions( - 'updateCurrentUser', - 'deleteCurrentUser' + 'fetchCurrentUser', + 'successFetchCurrentUser', + 'logoutCurrentUser', + 'successLogoutCurrentUser', + 'errorCurrentUser' ); } - - fetchCurrentUser() { - UserFetcher.fetchOne() - .then((res) => { - this.actions.updateCurrentUser(res.users[0]); - }) - .catch((err) => { - console.logGlobal(err); - this.actions.updateCurrentUser({}); - }); - } - - logoutCurrentUser() { - UserFetcher.logout() - .then(() => { - this.actions.deleteCurrentUser(); - }) - .catch((err) => { - console.logGlobal(err); - }); - } } export default altUser.createActions(UserActions); diff --git a/js/actions/whitelabel_actions.js b/js/actions/whitelabel_actions.js index a1460fb8..3d0e9d41 100644 --- a/js/actions/whitelabel_actions.js +++ b/js/actions/whitelabel_actions.js @@ -1,29 +1,16 @@ 'use strict'; import { altWhitelabel } from '../alt'; -import WhitelabelFetcher from '../fetchers/whitelabel_fetcher'; class WhitelabelActions { constructor() { this.generateActions( - 'updateWhitelabel' + 'fetchWhitelabel', + 'successFetchWhitelabel', + 'errorWhitelabel' ); } - - fetchWhitelabel() { - WhitelabelFetcher.fetch() - .then((res) => { - if(res && res.whitelabel) { - this.actions.updateWhitelabel(res.whitelabel); - } else { - this.actions.updateWhitelabel({}); - } - }) - .catch((err) => { - console.logGlobal(err); - }); - } } export default altWhitelabel.createActions(WhitelabelActions); diff --git a/js/components/ascribe_forms/form_login.js b/js/components/ascribe_forms/form_login.js index 2f0265b6..8b7c1e23 100644 --- a/js/components/ascribe_forms/form_login.js +++ b/js/components/ascribe_forms/form_login.js @@ -60,7 +60,7 @@ let LoginForm = React.createClass({ GlobalNotificationActions.appendGlobalNotification(notification); if(success) { - UserActions.fetchCurrentUser(); + UserActions.fetchCurrentUser(true); } }, diff --git a/js/components/ascribe_forms/form_signup.js b/js/components/ascribe_forms/form_signup.js index 87b7f47a..22f3e120 100644 --- a/js/components/ascribe_forms/form_signup.js +++ b/js/components/ascribe_forms/form_signup.js @@ -61,7 +61,7 @@ let SignupForm = React.createClass({ // Refactor this to its own component this.props.handleSuccess(getLangText('We sent an email to your address') + ' ' + response.user.email + ', ' + getLangText('please confirm') + '.'); } else { - UserActions.fetchCurrentUser(); + UserActions.fetchCurrentUser(true); } }, diff --git a/js/components/ascribe_routes/proxy_routes/auth_proxy_handler.js b/js/components/ascribe_routes/proxy_routes/auth_proxy_handler.js index 4f0fe307..b2d552a7 100644 --- a/js/components/ascribe_routes/proxy_routes/auth_proxy_handler.js +++ b/js/components/ascribe_routes/proxy_routes/auth_proxy_handler.js @@ -49,7 +49,11 @@ export default function AuthProxyHandler({to, when}) { }, componentDidUpdate() { - this.redirectConditionally(); + // Only refresh this component, when UserSources are not loading + // data from the server + if(!UserStore.isLoading()) { + this.redirectConditionally(); + } }, componentWillUnmount() { diff --git a/js/components/ascribe_settings/account_settings.js b/js/components/ascribe_settings/account_settings.js index f337a17b..c650358c 100644 --- a/js/components/ascribe_settings/account_settings.js +++ b/js/components/ascribe_settings/account_settings.js @@ -27,7 +27,7 @@ let AccountSettings = React.createClass({ }, handleSuccess(){ - this.props.loadUser(); + this.props.loadUser(true); let notification = new GlobalNotificationModel(getLangText('Settings succesfully updated'), 'success', 5000); GlobalNotificationActions.appendGlobalNotification(notification); }, diff --git a/js/components/ascribe_settings/settings_container.js b/js/components/ascribe_settings/settings_container.js index 923759fd..5b05e708 100644 --- a/js/components/ascribe_settings/settings_container.js +++ b/js/components/ascribe_settings/settings_container.js @@ -46,8 +46,8 @@ let SettingsContainer = React.createClass({ UserStore.unlisten(this.onChange); }, - loadUser(){ - UserActions.fetchCurrentUser(); + loadUser(invalidateCache){ + UserActions.fetchCurrentUser(invalidateCache); }, onChange(state) { diff --git a/js/fetchers/README.md b/js/fetchers/README.md new file mode 100644 index 00000000..4ab4d45b --- /dev/null +++ b/js/fetchers/README.md @@ -0,0 +1,9 @@ +# README + +Recently, we've been discovering [alt.js's sources](http://alt.js.org/docs/async/). As it allows us to implement caching as well as avoid a lot of unnecessary boilerplate code, we do not want to write any more NEW `fetcher`s. + +So if you need to query an endpoint, or if you make active changes to one of the `fetcher`s, please consider refactoring it to a `source`. + +Also, please read the `NAMING_CONVENTIONS.md` file in there first. + +Thanks \ No newline at end of file diff --git a/js/fetchers/user_fetcher.js b/js/fetchers/user_fetcher.js deleted file mode 100644 index eca7494d..00000000 --- a/js/fetchers/user_fetcher.js +++ /dev/null @@ -1,20 +0,0 @@ -'use strict'; - -import requests from '../utils/requests'; -import ApiUrls from '../constants/api_urls'; - -let UserFetcher = { - /** - * Fetch one user from the API. - * If no arg is supplied, load the current user - */ - fetchOne() { - return requests.get('user'); - }, - - logout() { - return requests.get(ApiUrls.users_logout); - } -}; - -export default UserFetcher; diff --git a/js/fetchers/whitelabel_fetcher.js b/js/fetchers/whitelabel_fetcher.js deleted file mode 100644 index 7a39f676..00000000 --- a/js/fetchers/whitelabel_fetcher.js +++ /dev/null @@ -1,17 +0,0 @@ -'use strict'; - -import requests from '../utils/requests'; - -import { getSubdomain } from '../utils/general_utils'; - - -let WhitelabelFetcher = { - /** - * Fetch the custom whitelabel data from the API. - */ - fetch() { - return requests.get('whitelabel_settings', {'subdomain': getSubdomain()}); - } -}; - -export default WhitelabelFetcher; diff --git a/js/sources/NAMING_CONVENTIONS.md b/js/sources/NAMING_CONVENTIONS.md new file mode 100644 index 00000000..81158793 --- /dev/null +++ b/js/sources/NAMING_CONVENTIONS.md @@ -0,0 +1,70 @@ +# Naming conventions for sources + +## Introduction + +When using [alt.js's sources](http://alt.js.org/docs/async/), we don't want the source's method to clash with the store/action's method names. + +While actions will still be named by the following schema: + +``` + +``` + +e.g. + +``` +fetchCurrentUser +logoutCurrentUser +fetchApplication +refreshApplicationToken +``` + +we cannot repeat this for a sources' methods as patterns like this would emerge in the stores: + +```javascript +onFetchCurrentUser(invalidateCache) { + this.invalidateCache = invalidateCache; + + if(!this.getInstance().isLoading()) { + this.getInstance().fetchCurrentUser(); // does not call a flux "action" but a method in user_source.js - which is confusing + } +} +``` + +Therefore we're introducing the following naming convention: + +## Rules + +1. All source methods that perform a data lookup of any kind (be it cached or not), are called `lookup`. As a mnemonic aid, "You *lookup* something in a *source*". +2. Promise-based callback methods - provided by alt.js - prepend their action, followed by `ObjectToManipulateInTheStore` (e.g. `error`) +2. For all methods that do not fit 1.), we prepend `perform`. + +## Examples + +### Examples for Rule 1.) +*HTTP GET'ing the current User* + +```javascript +UserActions.fetchCurrentUser +UserStore.onFetchCurrentUser +UserSource.lookupCurrentUser +``` + +### Examples for Rule 2.) +This talks about the naming in an actual `*_source.js` file: + +```javascript +lookupCurrentUser: { + success: UserActions.successFetchCurrentUser, // 'success' + error: UserActions.errorCurrentUser, // 'error' +}, +``` + +### Examples for Rule 3.) +*HTTP GET'ing a certain user endpoint, that logs the user out :sad_face:(, as this should not be a GET request anyway)* + +```javascript +UserActions.logoutCurrentUser +UserStore.onLogoutCurrentUser +UserSource.performLogoutCurrentUser +``` \ No newline at end of file diff --git a/js/sources/user_source.js b/js/sources/user_source.js new file mode 100644 index 00000000..872b060b --- /dev/null +++ b/js/sources/user_source.js @@ -0,0 +1,34 @@ +'use strict'; + +import requests from '../utils/requests'; +import ApiUrls from '../constants/api_urls'; + +import UserActions from '../actions/user_actions'; + + +const UserSource = { + lookupCurrentUser: { + remote() { + return requests.get('user'); + }, + + local(state) { + return state.currentUser && state.currentUser.email ? state : {}; + }, + success: UserActions.successFetchCurrentUser, + error: UserActions.errorCurrentUser, + shouldFetch(state) { + return state.userMeta.invalidateCache || state.currentUser && !state.currentUser.email; + } + }, + + performLogoutCurrentUser: { + remote() { + return requests.get(ApiUrls.users_logout); + }, + success: UserActions.successLogoutCurrentUser, + error: UserActions.errorCurrentUser + } +}; + +export default UserSource; \ No newline at end of file diff --git a/js/sources/whitelabel_source.js b/js/sources/whitelabel_source.js new file mode 100644 index 00000000..07bea746 --- /dev/null +++ b/js/sources/whitelabel_source.js @@ -0,0 +1,25 @@ +'use strict'; + +import requests from '../utils/requests'; +import WhitelabelActions from '../actions/whitelabel_actions'; + +import { getSubdomain } from '../utils/general_utils'; + + +const WhitelabelSource = { + lookupWhitelabel: { + remote() { + return requests.get('whitelabel_settings', {'subdomain': getSubdomain()}); + }, + local(state) { + return Object.keys(state.whitelabel).length > 0 ? state : {}; + }, + success: WhitelabelActions.successFetchWhitelabel, + error: WhitelabelActions.errorWhitelabel, + shouldFetch(state) { + return state.whitelabelMeta.invalidateCache || Object.keys(state.whitelabel).length === 0; + } + } +}; + +export default WhitelabelSource; \ No newline at end of file diff --git a/js/stores/user_store.js b/js/stores/user_store.js index 8ea18eea..e869f2df 100644 --- a/js/stores/user_store.js +++ b/js/stores/user_store.js @@ -3,19 +3,46 @@ import { altUser } from '../alt'; import UserActions from '../actions/user_actions'; +import UserSource from '../sources/user_source'; + class UserStore { constructor() { this.currentUser = {}; + this.userMeta = { + invalidateCache: false, + err: null + }; + this.bindActions(UserActions); + this.registerAsync(UserSource); } - onUpdateCurrentUser(user) { + onFetchCurrentUser(invalidateCache) { + this.userMeta.invalidateCache = invalidateCache; + + if(!this.getInstance().isLoading()) { + this.getInstance().lookupCurrentUser(); + } + } + + onSuccessFetchCurrentUser({users: [user]}) { + this.userMeta.invalidateCache = false; this.currentUser = user; } - onDeleteCurrentUser() { + + onLogoutCurrentUser() { + this.getInstance().performLogoutCurrentUser(); + } + + onSuccessLogoutCurrentUser() { this.currentUser = {}; } + + onErrorCurrentUser(err) { + console.logGlobal(err); + this.userMeta.err = err; + } } export default altUser.createStore(UserStore, 'UserStore'); diff --git a/js/stores/whitelabel_store.js b/js/stores/whitelabel_store.js index 017fb98e..378c5dbc 100644 --- a/js/stores/whitelabel_store.js +++ b/js/stores/whitelabel_store.js @@ -2,17 +2,38 @@ import { altWhitelabel } from '../alt'; import WhitelabelActions from '../actions/whitelabel_actions'; +import WhitelabelSource from '../sources/whitelabel_source'; class WhitelabelStore { constructor() { this.whitelabel = {}; + this.whitelabelMeta = { + invalidateCache: false, + err: null + }; + this.bindActions(WhitelabelActions); + this.registerAsync(WhitelabelSource); } - onUpdateWhitelabel(whitelabel) { + onFetchWhitelabel(invalidateCache) { + this.whitelabelMeta.invalidateCache = invalidateCache; + + if(!this.getInstance().isLoading()) { + this.getInstance().lookupWhitelabel(); + } + } + + onSuccessFetchWhitelabel({ whitelabel }) { + this.whitelabelMeta.invalidateCache = false; this.whitelabel = whitelabel; } + + onErrorCurrentUser(err) { + console.logGlobal(err); + this.whitelabelMeta.err = err; + } } export default altWhitelabel.createStore(WhitelabelStore, 'WhitelabelStore'); diff --git a/js/utils/feature_detection_utils.js b/js/utils/feature_detection_utils.js index d086c68c..6cbd533d 100644 --- a/js/utils/feature_detection_utils.js +++ b/js/utils/feature_detection_utils.js @@ -23,4 +23,4 @@ * @type {bool} Is drag and drop available on this browser */ export const dragAndDropAvailable = 'draggable' in document.createElement('div') && - !/Mobile|Android|Slick\/|Kindle|BlackBerry|Opera Mini|Opera Mobi/i.test(navigator.userAgent); \ No newline at end of file + !/Mobile|Android|Slick\/|Kindle|BlackBerry|Opera Mini|Opera Mobi/i.test(navigator.userAgent);