Improve router handlers architecture in Coach

[MisRob]

Blocks

#11422

Observed behavior

For majority of Coach pages, in addition to fetching data in their individual route handlers (e.g. showExamsPage handler), we fetch some of their data in the global beforeRoute handler, which is run before individual route handlers:

kolibri/kolibri/plugins/coach/assets/src/app.js

Lines 38 to 57 in 0f7b06c

	if (
	to.name &&
	!['CoachClassListPage', 'StatusTestPage', 'CoachPrompts', 'AllFacilitiesPage'].includes(
	to.name
	)
	) {
	promises.push(this.store.dispatch('initClassInfo', to.params.classId));
	} else {
	this.store.dispatch('coachNotifications/stopPolling');
	}
	if (this.store.getters.isSuperuser && this.store.state.core.facilities.length === 0) {
	promises.push(this.store.dispatch('getFacilities').catch(() => {}));
	}
	if (promises.length > 0) {
	Promise.all(promises).then(next, error => {
	this.store.dispatch('handleApiError', { error });
	});
	} else {
	next();
	}

This approach has the following problems:

1. Slows down navigation (user-facing)
Particularly observable on slower networks when after clicking a coach page, until all global beforeRoute promises are finished, user is not even redirected to a new page.

2. Limits individual pages ability to control when and how to display loading states (user-facing)
Many pages have their individual handlers that are sometimes connected to local loading states, however there is no way to factor in waiting time for this global beforeRoute which often seem to take even more time than promises in individual handlers, causing inaccuracy and discrepancy of loading states.

3. It's difficult to build a mental picture of what requests are performed when a page is visited (developer-facing)
Promises in the global beforeRoute are run for majority of pages except of few pages listed here

kolibri/kolibri/plugins/coach/assets/src/app.js

Line 40 in 0f7b06c

!['CoachClassListPage', 'StatusTestPage', 'CoachPrompts', 'AllFacilitiesPage'].includes(

Then, many pages have their separate handlers that have no information or control over the common logic, and are placed in different files.

Expected behavior

• Users are redirected immediately after navigating to another page and appropriate loaders are displayed on the page
• Individual pages have information on what data is being fetched, when it starts, ends and can control loading states in a flexible and simple manner

Steps to reproduce

Navigate Coach main pages and tabs on slower network

Guidance

To be able to achieve the expected behavior, we need to move promises from the global beforeRoute handler

kolibri/kolibri/plugins/coach/assets/src/app.js

Line 26 in 0f7b06c

router.beforeEach((to, from, next) => {

to individual page handlers.

For example, for showExamsPage page handler the steps could be:

1. Search the codebase to locate the name of a page that uses showExamsPage: PageNames.EXAMS

kolibri/kolibri/plugins/coach/assets/src/routes/planExamRoutes.js

Lines 27 to 37 in b88a804

	name: PageNames.EXAMS,
	path: '/:classId/plan/quizzes',
	component: CoachExamsPage,
	handler(toRoute) {
	showExamsPage(store, toRoute.params.classId);
	},
	meta: {
	titleParts: ['quizzesLabel', 'CLASS_NAME'],
	},
	},
	{

2. Add a condition to the global beforeRoute handler instructing it to not run code containing promises fetching data if user is navigating to a page with name PageNames.EXAMS

3. And instead, copy all those promises to showExamsPage

• This needs to be done for all individual page handlers in Coach. In this manner, it can be done gradually. I would recommend opening one pull request for one handler so we can review and QA loading states easily.

• After all individual handlers are updated to have the promises, we can finally remove the promises completely from the global beforeRoute handlers.

Acceptance criteria

• The global beforeRoute handler doesn't contain any promises fetching data
• The promises are instead in individual page handlers
• No regressions in regards to fetching data for Coach pages
• No regressions in the existing loading states

Tasks

Beta Give feedback

1. Refactor routes in kolibri/plugins/coach/assets/src/routes/planLessonsRoutes.js (DONE in Improved router handlers architecture in Coach - planLessonRoutes #11675)
2. Refactor routes in kolibri/plugins/coach/assets/src/routes/planRoutes.js (DONE in Improved route handlers architecture in Coach - planRoutes #11900)
3. Refactor routes in kolibri/plugins/coach/assets/src/routes/index.js (WIP, first related PR merged Refactored HomePage route handler to fetch initClassInfo and getFacil… #12358)
4. Refactor routes in kolibri/plugins/coach/assets/src/routes/reportRoutes.js
5. BLOCKED (as of February 2) Refactor routes in kolibri/plugins/coach/assets/src/routes/planExamRoutes.js
6. Finally remove data fetching from the global beforeRoute handler + cleanup temporary ignore list from there as well

[MisRob]

This is a major blocker for any further improvements of loading states on Coach pages --> adding P1 priority

[MisRob]

This might be good for contribution, but probably only if you have a bit of experience with Kolibri already. I would recommend gradual approach - refactoring one page handler at a time, especially for the first few page handlers.

develop branch should be targeted.

[GarvitSinghal47]

@MisRob i would like to work on this issue can you assign it to me.

[MisRob]

Hi @GarvitSinghal47, we'd be happy to have you work on this. Is everything clear in regards to the proposed solution? Feel free to ask any questions before you start working on it, or even jot down briefly the steps you're planning to do here and we will have a look. Please only choose one handler to work on now, for example showExamsPage I referenced and prepare a PR for it. During the review we can make sure that we're on the same page. After this initial step, I can reference other handlers (we have plenty) for which the approach will be exactly the same.

[MisRob]

@GarvitSinghal47 I also updated the issue description with some more references

[MisRob]

Most likely related #11238 (comment)

[MisRob]

Hi @GarvitSinghal47, are you working on this or should we unassign you?

[MisRob]

Unassigning due to no activity

[ShivangRawat30]

I would like to work on this issue.

[nucleogenesis]

Hey @ShivangRawat30 I've assigned you to the issue please let me know if you have any questions

[ShivangRawat30]

I referenced and prepare a PR for it

@MisRob Could you please provide information about the referenced pull request

[ShivangRawat30]

I am having a little trouble understanding the proposal.

[nucleogenesis]

@MisRob this is a pretty complex proposal and I'm wondering if, perhaps, we might be better able to handle the data initialization by way of some kind of useCoachClasses or useCoach which can be initialized when the coach app loads.

I may be misreading a bit -- but if the issue can be reworded as "some pages need some data boostrapped and others don't" then I think moving that logic entirely out of the router would be ideal.

For example, a useCoachClasses module could replace this line promises.push(this.store.dispatch('initClassInfo', to.params.classId)); and the Vuex module attached to it then wherever the things initialized there are used, those components can import or inject useCoachClasses' methods as needed.

Anyway just some thoughts happy to chat through 1:1 or in a larger discussion as well.

[MisRob]

we might be better able to handle the data initialization by way of some kind of useCoachClasses or useCoach which can be initialized when the coach app loads

I agree, @nucleogenesis. This issue is a first step towards us being able to use composables or any other solution. We can't really do much though until we get the promises out of the global hook (unless we want to do everything at once in a big refactor). So the idea is - get rid of the global promises by moving them to current handlers. Then, we can do whatever we want with those separate handlers (e.g. continue refactoring them to composables gradually in the scope of upcoming issues). I think this is aligned with what you suggest?

[nucleogenesis]

@MisRob yeah that makes sense and is probably a good way to do this.

So then the major change here (or like the acceptance criteria?), then, is to just extract the logic from the global beforeEach and put it into the route handlers where that logic is needed?

I think perhaps updating the acceptance criteria to reflect that (ie, that the logic needs to be rehomed) might make things a little bit clearer for @ShivangRawat30 -- in addition to this discussion I hope! Thanks Misha!

[MisRob]

Yes, I will think about a way how to make this more straightforward. Now I can see I mentioned two possible approaches, which doesn't help either.

[MisRob]

@ShivangRawat30 I will follow-up tomorrow

[MisRob]

Also, @ShivangRawat30, meanwhile please ask any clarifying questions, or you could also try to see those problems on you local dev server, preview code I referenced, and perhaps formulate your understanding of the problem in a few bullet points. This will help me to think about ways to reformulate this in a beneficial way. It's a bit more complex issue connected to the way we work with router handlers in the whole Kolibri, but I think if as soon as we are on the same page in regards to what's the core problem, it will be much easier.

Thanks both.

[MisRob]

@MisRob Could you please provide information about the referenced pull request

@ShivangRawat30 By

Please only choose one handler to work on now, for example showExamsPage I referenced and prepare a PR for it.

By that, I was trying to suggest that you prepare a pull request for showExamsPage as a first step. But let's wait until it's all more clear :)

[MisRob]

@ShivangRawat30 Yes, the approach is exactly the same.

The only difference I can think of is that you will need to test a page corresponding to a handler and see if and what comes up after the refactor, and that something may need to be approached differently in different handlers. I wouldn't expect many problems though since the strategy we've chosen should ensure that the functionality stays pretty much the same even when run from different places.

You will also need to understand/find out from the code what promises were relevant to a handler you're working on but I think you already know that. As I mentioned in your first PR, you correctly didn't just copy-paste but left only handlers that were indeed run in the previous implementation. 👍

[MisRob]

@ShivangRawat30 I wanted to note that this is my last day before coming back on January 8. Some of us will be online for a few more days and then The Learning Equality will be closed from December 22 to January 2. So we will most likely follow-up again some time next year. Thanks and be well!

[MisRob]

@ShivangRawat30 In my attempts to be organized, I added a tasklist to the issue description which references files with routes to be examined and possibly refactored as part of this work.

[ShivangRawat30]

@MisRob for the routes in kolibri/plugins/coach/assets/src/routes/index.js most of the handlers are store.dispatch('notLoading'); should I add the promises to these handlers or create the created hook in their respective components.

[MisRob]

Hi @ShivangRawat30, for handlers that do not fetch data, please add promises to their respective components

[ShivangRawat30]

@MisRob there are also some routes that doesn't contain any name { path: '/about/statuses', component: StatusTestPage, handler() { store.dispatch('notLoading'); }, } how should we add them to the if condition ?

[MisRob]

@ShivangRawat30 Ah I see, I think you can just name them somehow :)

[MisRob]

Hi @ShivangRawat30, are you planning to return to this or would it be better to unassign?

[shubh1007]

May I work on Task - 3 of refactoring routes in kolibri/plugins/coach/assets/src/routes/index.js?

[MisRob]

Hi @shubh1007, that'd be great! This issue has a pretty high priority since we're planning some features that will need to have this cleaned up.

Please have a look at the previous pull requests and conversations as that will help you to understand the strategy since you will continue it in your work. I'd ask if your PR description can have a format like here #11900 - containing the table of refactored places for reference and also ways you navigated to test them as this well help review process and QA. But first, let's see if it's all clear in regard to how to approach the work.

[MisRob]

@shubh1007 Also see this place https://github.com/learningequality/kolibri/pull/11900/files#diff-bf0e4e3732211307b4c4c45a916359f5a127f4065157500405c7a5411983a857R64

[shubh1007]

Thank you for assigning me the issue related to refactoring the routes. I have reviewed the previous pull requests and conversations, but I would appreciate a brief explanation to ensure I understand the task correctly.
Could you please provide a bit more detail about the specific changes you are expecting in this refactoring? For instance, any particular routes or handlers I should pay special attention to, or any specific examples of the promises that need to be moved to individual page handlers?
Understanding these details will help me approach the work more effectively and align with the project's overall strategy.
Thank you.

[MisRob]

Thanks for looking into it @shubh1007. To sum it up

The ultimate goal is to prepare all coach pages so that one day, we can completely remove this part from the global route handler

kolibri/kolibri/plugins/coach/assets/src/app.js

Lines 93 to 106 in 7ba25fd

	if (
	to.name &&
	to.params.classId &&
	!['CoachClassListPage', 'StatusTestPage', 'CoachPrompts', 'AllFacilitiesPage'].includes(
	to.name,
	)
	) {
	promises.push(this.store.dispatch('initClassInfo', to.params.classId));
	}
	if (this.store.getters.isSuperuser && this.store.state.core.facilities.length === 0) {
	promises.push(this.store.dispatch('getFacilities').catch(() => {}));
	}

This means that at first, we need to prepare all pages that rely implicitly on data fetched by initClassInfo and/or getFacilities by making sure they call these two promises on their own.

I will use #11900 as example to elaborate on the strategy you would continue. In that PR, the author

• (1) found out out where data are being fetched for a route/page, in this case it was showGroupsPage function
• (2) made showGroupsPage to call initClassInfo and getFacilities
• (3) added pages that call showGroupsPage to this temporary ignore list (to ensure that the promises won't be called twice)
  

  kolibri/kolibri/plugins/coach/assets/src/app.js

  Lines 69 to 91 in 7ba25fd

  	if (
  	to.name &&
  	[
  	PageNames.EXAMS,
  	LessonsPageNames.PLAN_LESSONS_ROOT,
  	LessonsPageNames.LESSON_CREATION_ROOT,
  	LessonsPageNames.SUMMARY,
  	LessonEditDetailsPage.name,
  	LessonsPageNames.SELECTION_ROOT,
  	LessonsPageNames.SELECTION,
  	LessonsPageNames.SELECTION_SEARCH,
  	LessonsPageNames.LESSON_SELECTION_BOOKMARKS,
  	LessonsPageNames.LESSON_SELECTION_BOOKMARKS_MAIN,
  	LessonsPageNames.SELECTION_CONTENT_PREVIEW,
  	LessonsPageNames.RESOURCE_CONTENT_PREVIEW,
  	GroupsPage.name,
  	GroupMembersPage.name,
  	GroupEnrollPage.name,
  	].includes(to.name)
  	) {
  	next();
  	return;
  	}

After all pages are refactored, we will remove the temporary ignore list together with initClassInfo and getFacilities calls from coach/assets/src/app.js which will conclude this task.

Is the approach clear overall?

Note that some handlers can be used in more than one page, and there can be differences in how we fetch data in pages - sometimes it's via route handler functions, and sometimes in a component. It will be up to you to do code search and understand what's happening page by page. It should be pretty straightforward as it's just a few repeating patterns. In case of doubt, we can discuss in pull requests.

It'd be always best to open a PR with just a few places refactored, especially in the beginning. Feel free to use a draft pull request with just one or two routes and ask any questions there. I'd also ask you to note down instructions for testing, similarly to the table in the example PR. Nothing elaborate needed, if you could just note what you did when you were manually previewing refactored pages by yourself. The routes that haven't been refactored yet are listed in the PR tasklist.

[MisRob]

Hey @shubh1007, we need to prioritize this for the upcoming release so the core team is going to finish this issue. Thanks a lot for your contribution!

[shubh1007]

Hey @shubh1007, we need to prioritize this for the upcoming release so the core team is going to finish this issue. Thanks a lot for your contribution!

Thank you for providing me the opportunity to contribute.

[MisRob]

Happy to! We'd be still glad to have you work on any issues currently marked 'help wanted' any time you're interested.