Improved router handlers architecture in Coach - part 1

[ShivangRawat30]

Summary

This pull request enhances route handling by restricting the responsibility of route handling to local handlers rather than relying on a global route handler.

References

This work is part of #11219 (it doesn't close it though)

Reviewer guidance

---

Testing checklist

• Contributor has fully tested the PR manually
• If there are any front-end changes, before/after screenshots are included
• Critical user journeys are covered by Gherkin stories
• Critical and brittle code paths are covered by unit tests

PR process

• PR has the correct target branch and milestone
• PR has 'needs review' or 'work-in-progress' label
• If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
• If this is an important user-facing change, PR or related issue has a 'changelog' label
• If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

• Automated test coverage is satisfactory
• PR is fully functional
• PR has been tested for accessibility regressions
• External dependency files were updated if necessary (yarn and pip)
• Documentation is updated
• Contributor is in AUTHORS.md

[github-actions]

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.16.0b6.dev0_git.585.g06064890.pex
Windows Installer (EXE)	kolibri-0.16.0b6.dev0+git.585.g06064890-unsigned.exe
Debian Package	kolibri_0.16.0b6.dev0+git.585.g06064890-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.16.0b6.dev0+git.585.g06064890-0.3.0.dmg
Android Package (APK)	kolibri-0.16.0b6.dev0+git.585.g06064890-0.1.0-debug.apk
TAR file	kolibri-0.16.0b6.dev0+git.585.g06064890.tar.gz
WHL file	kolibri-0.16.0b6.dev0+git.585.g06064890-py2.py3-none-any.whl

[marcellamaki]

Added @nucleogenesis as a reviewer just because I think there are some concurrent changes to coach routing in develop for Enhanced Quiz Management

[MisRob]

Hi @ShivangRawat30, in the light of we discussed, this makes sense to me. Thank you for following the suggestion to refactor the global beforeRoute hook into two separate sections - the first with other logic that will stay, and the latter with what will be eventually removed. I am leaving a few comments related to code. I will also test it for regressions.

I will update the PR description since this doesn't close the whole #11570. It's rather a first step. There are plenty of other page handlers that we need to add to the ignore list we now have thanks to this work in the global beforeRoute, and move promises to those page handlers. And after that's done, we will remove the second section of the global beforeRoute finally. Then, the issue will be closed. Would you like to continue working on it? The approach will be the same.

[MisRob]

@nucleogenesis this approach is based on #11219 (comment), #11219 (comment), and https://gist.github.com/MisRob/fe3d75e27883308272089dcfe02356ee to help @ShivangRawat30 to start this work gradually (just to make sure we're on the same board before refactoring all handlers). If he decides to continue, we will talk about in how large chunks this will get addressed.

If you haven't changed the core workings of the global beforeRoute hook, I don't think there should be many significant conflicts, right? Perhaps some in individual page handlers, but that's something that shouldn't be difficult to deal with. Please let me know how things are in the Enhanced Quiz Management.

[nucleogenesis]

The approach looks good but it does seem likely that the changes should all happen in one PR (I'm assuming the changes in the router guard will cause regressions in handlers.js's that haven't had the same update as examsRoot?).

In any case -- we are no longer using the handlers for exam creation in the routes and I'm not sure if the changes would need to be applied there anyway.

(thanks for your work on this @ShivangRawat30 ! <3)

[ShivangRawat30]

@MisRob I've implemented the suggested changes for the review. and I am eager to continue working on this issue.

[MisRob]

@nucleogenesis

The approach looks good but it does seem likely that the changes should all happen in one PR (I'm assuming the changes in the router guard will cause regressions in handlers.js's that haven't had the same update as examsRoot?).

No, the main idea here is actually to prepare ground for rather gradual refactor (for more reasons). Here, you can see that the promises we want to eventually remove are now ignored only for a page in which the first handler that Shivang refactored, while still being run for all other pages. We will keep adding to this "ignore" while refactoring other handlers. After all handlers are done, the ignore condition will be removed together with all promises fetching data from the global router handler:

https://github.com/learningequality/kolibri/pull/11570/files#diff-bf0e4e3732211307b4c4c45a916359f5a127f4065157500405c7a5411983a857R50-R52

     // temporary condition as we're gradually moving all promises below this line to local page handlers and therefore need to skip those that we already refactored here https://github.com/learningequality/kolibri/issues/11219
      if (to.name && to.name === PageNames.EXAMS) {
        next();
      }

The rest of this PR is only reordering the global handler to allow for a cleaner separation and a bit more readable structure during this temporary phase.

[MisRob]

@nucleogenesis

In any case -- we are no longer using the handlers for exam creation in the routes and I'm not sure if the changes would need to be applied there anyway.

This is a bigger concern. If you still rely on the global handler (I assume you do?) then I would say that probably you rely on those data being fetched in the global router being available and used somehow. So I think it's be best to left examCreation module untouched and leave as last part of this refactor after we agree together it's time, right? Due to the approach I explained above everything should still work just fine for all other handlers we didn't explicitly refactored, so I'd be okay with this.

[MisRob]

@nucleogenesis if this plan of attack makes sense to you too, is examCreation module the only module that's under work, or are there some more that'd be best to leave for later to prevent conflicts?

[nucleogenesis]

Only the examCreation :)

[MisRob]

@ShivangRawat30 Thank you! I will re-review and also test in detail that the global beforeRoute still works just the same for all other pages except the one you refactored. Then we will have a solid ground set up and it should be pretty easy now. I think then you could continue working pretty independently on plenty of other handlers. But based on what we've just talked with @nucleogenesis, I will take some time to plan a bit which handlers are good to tackle and provide you with a list.

[MisRob]

@nucleogenesis

Only the examCreation :)

Good news! Alright, thank you, that's good to know in advance.

[MisRob]

Good progress @ShivangRawat30, thank you. I re-reviewed and am leaving some new notes.

[ShivangRawat30]

@MisRob I've implemented the required modifications, kindly review them. Once this pull request receives approval and is merged, I'll proceed to work on additional handlers.

[ShivangRawat30]

@MisRob Apologies for earlier. I've now made the adjustments. Please review and let me know if the approach need some changes.

[MisRob]

Thank you, @ShivangRawat30. It's looking good to me. I also manually tested the page and haven't observed any issues.

Just for your information, I pushed a commit disabling Jest tests for this handler. I will describe what's the problem with updating the current test suite here #11615 later on.

[MisRob]

As I pushed my own update, I'd welcome a second pair of eyes on this PR before merging. cc @rtibbles

[rtibbles]

Changes make sense - we can gradually move the global behaviour for each handler out of this!