Improve router handlers architecture in Coach

MisRob commented 1 year ago

Blocks

https://github.com/learningequality/kolibri/issues/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:

https://github.com/learningequality/kolibri/blob/0f7b06cc320476cbe0aca4ecaacae107b184cd45/kolibri/plugins/coach/assets/src/app.js#L38-L57

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.

coach-loading

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

https://github.com/learningequality/kolibri/blob/0f7b06cc320476cbe0aca4ecaacae107b184cd45/kolibri/plugins/coach/assets/src/app.js#L40

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

https://github.com/learningequality/kolibri/blob/0f7b06cc320476cbe0aca4ecaacae107b184cd45/kolibri/plugins/coach/assets/src/app.js#L26

to individual page handlers.

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

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

https://github.com/learningequality/kolibri/blob/b88a8043fd4a0071d281699ed853c791a55518d8/kolibri/plugins/coach/assets/src/routes/planExamRoutes.js#L27-L37

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
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
- [x] Refactor routes in _kolibri/plugins/coach/assets/src/routes/planLessonsRoutes.js_ (DONE in https://github.com/learningequality/kolibri/pull/11675)
- [x] Refactor routes in _kolibri/plugins/coach/assets/src/routes/planRoutes.js_ (DONE in https://github.com/learningequality/kolibri/pull/11900)
- [ ] Refactor routes in _kolibri/plugins/coach/assets/src/routes/index.js_ (WIP, first related PR merged https://github.com/learningequality/kolibri/pull/12358)
- [ ] Refactor routes in _kolibri/plugins/coach/assets/src/routes/reportRoutes.js_
- [ ] BLOCKED (as of February 2) Refactor routes in _kolibri/plugins/coach/assets/src/routes/planExamRoutes.js_
- [ ] Finally remove data fetching from the global `beforeRoute` handler + cleanup temporary ignore list from there as well

MisRob commented 1 year ago

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

MisRob commented 1 year ago

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 commented 1 year ago

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

MisRob commented 1 year ago

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 commented 1 year ago

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

MisRob commented 1 year ago

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

MisRob commented 1 year ago

Unassigning due to no activity

ShivangRawat30 commented 11 months ago

I would like to work on this issue.

nucleogenesis commented 11 months ago

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

ShivangRawat30 commented 10 months ago

I referenced and prepare a PR for it

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

ShivangRawat30 commented 10 months ago

I am having a little trouble understanding the proposal.

nucleogenesis commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

@ShivangRawat30 I will follow-up tomorrow

MisRob commented 10 months ago

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 commented 10 months ago

@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 commented 10 months ago

@nucleogenesis I updated the acceptance criteria as you suggested @ShivangRawat30 I also changed the "Proposal" to more concrete "Guidance", and made some other tweaks to the issue description. Please read through the issue again and let me know if it makes more sense now.

ShivangRawat30 commented 10 months ago

Thank you for the guidance @MisRob I'll open a PR soon for the issue.

ShivangRawat30 commented 10 months ago

@MisRob I have some questions

is this the right approach for the before route handler and do we have to copy all the promise logic to the showExamPage Handler ?

MisRob commented 10 months ago

@ShivangRawat30

is this the right approach for the before route handler

The direction is good. However we only want to ignore promises that are fetching data in the global beforeRoute for this page. Other logic, for example:

if (this.store.state.core.snackbar.isVisible) {
  this.store.dispatch('clearSnackbar');
}
this.store.commit('SET_PAGE_NAME', to.name);

this.store.dispatch('coachNotifications/stopPolling');

should still run when it's relevant.

and do we have to copy all the promise logic to the showExamPage Handler

Yes, the promises that are not run in the global beforeRoute anymore should now be run instead from the showExamPage.

It might help, before refactoring, to do some logging after visiting the page associated with the showExamPage handler to see what is actually triggered for it.

The main task is to simply move fetching data to showExamsPage. I think you will need to study the global beforeRoute logic in detail to gain a mental map and to figure out which promises need to be moved as it's a bit of mess due to all current conditions. Feel free to re-organize it in a helpful way.

MisRob commented 10 months ago

@ShivangRawat30

Feel free to re-organize it in a helpful way

As first step, you may for example rewrite global beforeRoute handler to two separate parts - one with promises fetching data and one with other logic (now it's all mingled). Even though this may result in some duplicated conditions, it'd be okay at this point and it might help with adding new ignore conditions since you would only wrap the promises section.

You don't need to do it in this way, it's just an idea I got after looking at the file now.

ShivangRawat30 commented 10 months ago

@MisRob I tried separating the parts of the before route handler please have a look

MisRob commented 10 months ago

@ShivangRawat30 Almost! I should be more specific what I mean by promises fetching data. Please see this gist https://gist.github.com/MisRob/fe3d75e27883308272089dcfe02356ee I don't know if it's functional as it's just a quick draft I wrote to demonstrate what can now stay in the handler and what will be moved. You may need to play around with it and test it out. There are certainly ways it can be further cleaned up. But as first step, this separation should be sufficient for us to be able to start moving promises to handlers.

MisRob commented 10 months ago

Over time, we may want to remove more things from the global hook, but for now let's keep it simple and focus only on those specific promises in the second section

MisRob commented 10 months ago

Thanks for your patience here @ShivangRawat30 ;) Now I realize it'd be best to specify in the very beginning what promises I am referring to.

ShivangRawat30 commented 10 months ago

@MisRob Thanks for the clarification, I have opened a PR using the approach.

MisRob commented 10 months ago

@ShivangRawat30 Since you expressed your interest to continue - after we merge https://github.com/learningequality/kolibri/pull/11570, here is the outline of next steps. The ultimate goal is to refactor all Coach page handlers. You can preview all Coach routes here https://github.com/learningequality/kolibri/tree/170b74f4f5b55497355b1c8fd5262bb19f1709dd/kolibri/plugins/coach/assets/src/routes and by examining them, you can find the handlers.

As discussed with @nucleogenesis in your PR, please do not work on examCreation module handlers.

You don't need to open one PR per one handler, however I'd recommend opening reasonable sized PRs gradually with just a few handlers rather than doing all at once so that we can review fast, test easily, and prevent conflicts. This will be possible after we merge https://github.com/learningequality/kolibri/pull/11570.

If you could provide information on what page was refactored (e.g. URL or steps to navigate there) in your PRs, that'd be helpful.

Thank you for volunteering for this issue, it touches some of the core architecture and is not a trivial task to work on.

ShivangRawat30 commented 10 months ago

@MisRob Would the approach be the same for all local handlers?

MisRob commented 10 months ago

@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. :+1:

MisRob commented 10 months ago

@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 commented 8 months ago

@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 commented 7 months ago

@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 commented 7 months ago

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

ShivangRawat30 commented 7 months ago

@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 commented 7 months ago

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

MisRob commented 5 months ago

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

shubh1007 commented 4 months ago

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

MisRob commented 4 months ago

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 https://github.com/learningequality/kolibri/pull/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 commented 4 months ago

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

shubh1007 commented 3 months ago

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 commented 3 months ago

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

https://github.com/learningequality/kolibri/blob/7ba25fd14e2df6127879111922316f1225a6d787/kolibri/plugins/coach/assets/src/app.js#L93-L106

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 https://github.com/learningequality/kolibri/pull/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) https://github.com/learningequality/kolibri/blob/7ba25fd14e2df6127879111922316f1225a6d787/kolibri/plugins/coach/assets/src/app.js#L69-L91

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

learningequality / kolibri