Page MenuHomePhabricator

MyLists.vue can call vuex before user is known
Closed, ResolvedPublicBUG REPORT

Description

The vuex method for getting the current user's lists ('lists/getMyLists') uses a hack of grabbing the username to pass to the backend API from context.rootState.user.user.username. When deep linking to /lists or hard reloading on that view it is very likely that the mounted() method will be called before the App.vue level 'user/getUserInfo' vuex call has completed resulting in passing a username of 'undefined' to the backend and an empty set of lists as the response.

Event Timeline

According to the Vuex docs https://vuex.vuejs.org/guide/actions.html#composing-actions, it is possible to compose actions, as long as the action that needs to complete first returns a Promise (which all actions that make API calls now do thanks to @Raymond_Ndibe).

Here is what composing actions could look like, although there are other options, such as async/await.

	getMyLists( context, page ) {
		return context.dispatch( 'user/getUserInfo', { vm: this }, { root: true } ).then( () => {
			const params = new URLSearchParams( [
				[ 'user', context.rootState.user.user.username ],
				[ 'page', page ]
			] );

			const request = {
				url: '/api/lists/?' + params.toString(),
				method: 'GET',
				headers: {
					'Content-Type': 'application/json'
				}
			};

			makeApiCall( context, request ).then(
				( success ) => {
					const data = success.body;
					context.commit( 'MY_LISTS', data );
				},
				( failure ) => {
					displayErrorNotification.call( this, failure );
				}
			);
		} );
	}

This seems to work, but so does the current code, as I haven't yet run into the problem of erroneously getting an empty set of lists as a response. How would we test for possible failure cases?

According to the Vuex docs https://vuex.vuejs.org/guide/actions.html#composing-actions, it is possible to compose actions, as long as the action that needs to complete first returns a Promise (which all actions that make API calls now do thanks to @Raymond_Ndibe).

Indeed. This ability was actually the reason we wanted to update things to return Promises.

This seems to work, but so does the current code, as I haven't yet run into the problem of erroneously getting an empty set of lists as a response. How would we test for possible failure cases?

I can reproduce the call to http://localhost:8000/api/lists/?user=undefined&page=1 and empty result every time with a forced reload of http://localhost:8000/lists.

The only reason it doesn't happen in the unit tests for getMyLists is that we have supplied a stub rootState there which includes a user.user.username value.

I would like to try and make the dispatch to 'user/getUserInfo' more boring. The vm argument there is used to update the $ability plugin with the collected rights of the user. I think we can hide that from the caller with a little work.

bd808 changed the task status from Open to In Progress.Feb 24 2022, 5:39 PM
bd808 claimed this task.
bd808 moved this task from Backlog to In Progress on the Toolhub board.

Change 765576 had a related patch set uploaded (by BryanDavis; author: Bryan Davis):

[wikimedia/toolhub@main] ui: ensure user is fetched before fetching user's lists

https://gerrit.wikimedia.org/r/765576

bd808 triaged this task as Medium priority.Feb 24 2022, 5:52 PM
bd808 moved this task from In Progress to Review on the Toolhub board.

Change 765576 merged by jenkins-bot:

[wikimedia/toolhub@main] ui: ensure user is fetched before fetching user's lists

https://gerrit.wikimedia.org/r/765576

Change 770638 had a related patch set uploaded (by BryanDavis; author: Bryan Davis):

[operations/deployment-charts@master] toolhub: Bump container version to 2022-03-15-002555-production

https://gerrit.wikimedia.org/r/770638

Change 770638 merged by jenkins-bot:

[operations/deployment-charts@master] toolhub: Bump container version to 2022-03-15-002555-production

https://gerrit.wikimedia.org/r/770638