Coach main navigation refactor #12775

AlexVelezLl · 2024-10-30T13:23:03Z

Summary

Refactor of the main navigation of the coach plugin for class, plans, reports -> class, lessons, quizzes, learners, groups.
Get rid of the old plan and reports main view folders and map all needed files to new main view folders: lesson, quizzes, learners, groups.
Get rid of the old plan and reports routes and map them in lesson, quizzes, learners, groups route files.
Unify LesonPageNames and PageNames, and refactor all places to use the PageNames constants rather than burned strings.
Get rid of many unnecesary routes and components of the old reports tab.
Propagate use of useRouteTerms route segment constants through all routes for consistency.
Unified multiple duplicated components into LearnerQuizPage, ExerciseQuestionListPage and QuestionLearnersReport components.
Fix side panel condition to create new lesson.

Compartir.pantalla.-.2024-10-30.09_24_14.mp4

References

Closes #12729 and #12801

Reviewer guidance

For code review:

Check that the new folders structure, file organization and further refactors make sense

For QA:

Navigate through all coach pages, and notice that:
- All pages work correctly
- There is a proper navigation flow and matches the figma specs
- The correct tab in the main nav is properly selected in all pages

Follow ups

If the tabs overflow and the more button is shown, if you go to any tab in the more button you will need to select the class again, because the navigation in that part isnt forwarding the classId. This can be solved in a follow up issue.

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

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 · 2024-10-30T13:53:26Z

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.18.0.dev0_git.20241111165948.pex
Windows Installer (EXE)	kolibri-0.18.0.dev0+git.20241111165948-windows-setup-unsigned.exe
Debian Package	kolibri_0.18.0.dev0+git.20241111165948-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.18.0.dev0+git.20241111165948.dmg
Android Package (APK)	kolibri-0.18.0.dev0+git.20241111165948-0.1.4-debug.apk
TAR file	kolibri-0.18.0.dev0+git.20241111165948.tar.gz
WHL file	kolibri-0.18.0.dev0+git.20241111165948-py2.py3-none-any.whl

kolibri/plugins/coach/assets/src/constants/index.js

kolibri/plugins/coach/assets/src/constants/lessonsConstants.js

kolibri/plugins/coach/assets/src/modules/lessonsRoot/actions.js

AlexVelezLl · 2024-10-30T14:42:00Z

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

-    name: QuizSummaryPage.name,
-    path: '/:classId/plan/quizzes/:quizId/:tabId?',
+    name: PageNames.EXAM_SUMMARY,
+    path: path(CLASS, QUIZ, '/:tabId?'),


I am planning to expand this path(...) pattern, if there is any objections you can let me know :)

I must admit, I don't particularly like it - but I can't quite put my finger on why (I didn't like its existing use in many different parts of the coach routing). I was hoping that we could use nested routes to do a lot of this sort of concatenation, but given the nature of the routing, I don't think we can.

So, I think this is fine - but I guess, is there any reason to not just do CLASS + QUIZ + '/:tabId?'?

I dont like it neither 😆, because for non-familiarized people its very tedious as we just cant inmediately see what is the actual path of each route. But for this refactor it was much easier to just remove the /plan on the CLASS constant, and I think it ensures the consistency as we will need to rewrite the same base routes a huge lot of times. I'm not opposed to reverting it, But whatever pattern we choose, I would like to replicate in all the coach routes for consistency.

is there any reason to not just do CLASS + QUIZ + '/:tabId?'?

Not really, I can make this change.

would like to replicate in all the coach routes for consistency

for me this is the most important thing.

I think my challenge with this existing routing pattern is that I find it a bit inconsistent within its own logic. For example in the reportsRoutes
const CLASS = '/:classId/reports' feels inconsistent const QUIZ = '/quizzes/:quizId';. I know it's necessary (it's the reports, not the plan tab), but it isn't really something you can logic out with normal API patterns when reading the code.

And that is not even including the differences in the files between reports and plan (which thankfully, will disappear with this refactor! 🎉)

I wonder if there might be some small changes that we could make to have these feel internally more consistent, without too much scope creep? That might help with making them easier to read. I can try to come up with some more concrete suggestions if either of you think this is worth talking about more (or an example, if this feels vague). Some of this might be resolved just in the course of getting rid of the reports and plan differences, I just haven't thought it all through yet.

I think maybe best to do the minimum for now in terms of the actual content of the routes, and then we can do smaller follow up issues for routes cleanup specifically.

After migrating a huge amount of routes, I am glad we had these constants partial terms, it made everything much easier

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

kolibri/plugins/coach/assets/src/views/lessons/LessonSummaryPage/tables/LessonLearnersTable.vue

kolibri/plugins/coach/assets/src/views/lessons/LessonSummaryPage/ManageLessonModals.vue

rtibbles · 2024-10-30T17:55:21Z

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

@@ -1,21 +1,23 @@
 import { PageNames } from '../constants';
+import { LessonsPageNames } from '../constants/lessonsConstants';
+import routes from '.';


The purpose of the baseRoutes is to avoid importing the full routes module into the CoachSideNavEntry module (which is an entry point in itself), as doing so will significantly increase the size of that bundle. So we should undo this import and update the paths in another way.

Oooh, didnt know that, I just wanted to avoid the duplication of these routes. But if its because of performance reasons, I think its better just to revert it and keep the old strategy. At the end we wont be touching this file too much.

If you want to have a single source of truth, you could put the path segments into a constants file and reference those in here (although that would be further obfuscating the actual paths in multiple places!)

I think having a single source of truth might be useful, but I have struggled quite a bit with some of the use of constants throughout the coach pages/routing. I know this refactor overall will help a lot with the parts of this plugin that are more confusing. But, I think if we do go the constants route (...that joke was just for you Richard), it might be helpful to have someone less familiar with the coach architecture in general take a look before approval/merge and see how easy it is for them to parse.

But, I think if we do go the constants route (...that joke was just for you Richard)

This is a very thinly sliced double meaning.

marcellamaki

just added a few thoughts in the threads where you were talking to @rtibbles, @AlexVelezLl !

kolibri/plugins/coach/assets/src/constants/lessonsConstants.js

marcellamaki · 2024-10-31T14:01:40Z

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

@@ -1,21 +1,23 @@
 import { PageNames } from '../constants';
+import { LessonsPageNames } from '../constants/lessonsConstants';
+import routes from '.';


I think having a single source of truth might be useful, but I have struggled quite a bit with some of the use of constants throughout the coach pages/routing. I know this refactor overall will help a lot with the parts of this plugin that are more confusing. But, I think if we do go the constants route (...that joke was just for you Richard), it might be helpful to have someone less familiar with the coach architecture in general take a look before approval/merge and see how easy it is for them to parse.

marcellamaki · 2024-10-31T14:27:53Z

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

-    name: QuizSummaryPage.name,
-    path: '/:classId/plan/quizzes/:quizId/:tabId?',
+    name: PageNames.EXAM_SUMMARY,
+    path: path(CLASS, QUIZ, '/:tabId?'),


would like to replicate in all the coach routes for consistency

for me this is the most important thing.

I think my challenge with this existing routing pattern is that I find it a bit inconsistent within its own logic. For example in the reportsRoutes
const CLASS = '/:classId/reports' feels inconsistent const QUIZ = '/quizzes/:quizId';. I know it's necessary (it's the reports, not the plan tab), but it isn't really something you can logic out with normal API patterns when reading the code.

And that is not even including the differences in the files between reports and plan (which thankfully, will disappear with this refactor! 🎉)

I wonder if there might be some small changes that we could make to have these feel internally more consistent, without too much scope creep? That might help with making them easier to read. I can try to come up with some more concrete suggestions if either of you think this is worth talking about more (or an example, if this feels vague). Some of this might be resolved just in the course of getting rid of the reports and plan differences, I just haven't thought it all through yet.

rtibbles

Just one immediate thing that sprung out to me.

My only other thought is whether there are any other components (I think the routes are unavoidable) that could be moves with edits rather than new components? That could make the diff a bit clearer in terms of what is actually updated here, and what has been created anew.

rtibbles · 2024-11-07T16:59:39Z

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

@@ -15,3 +15,31 @@ export function classIdParamRequiredGuard(toRoute, subtopicName, next) {
    return true;
  }
 }
+
+export function useRouteTerms() {
+  return {


This feels like it could just be a module level constant object rather than needing to be returned from a function? The composable seems unnecessary here.

pcenov · 2024-11-08T14:39:27Z

Hi @AlexVelezLl, I was able to identify the following issues, which I can also report separately if not in scope for this PR:

The tabs in the navigation should go to the second line when there isn't enough space, and only after that we should be showing the ... button. See the video:

responsive.mp4

The 'Edit lesson' option actually creates a brand new lesson:

Edit.lesson.mp4

Nothing happens when I click the 'Preview' button for a quiz:

preview.quiz.mp4

When I delete a quiz I am not immediately brought back to the Quizzes page:

delete.a.quiz.mp4

When I make a copy of a quiz and I click the 'All quizzes' link then I see an empty page:

copy.quiz.and.go.back.mp4

When there is only one class in the facility and I navigate from another plugin to a page in the Coach plugin, then I always see the Classes page before going to the desired page which should be avoided if possible. This is especially obvious in slow 3g network throttling:

classes.page.mp4

AlexVelezLl · 2024-11-08T15:15:58Z

Thank you @pcenov!

Yes, I think we can add this as a follow up issue as this is an existing behaviour even in the current implementation.
I will fix this!
This is an issue from Update preview plan quiz #12685. ~~I think we could just hide this button for now~~. I just linked the preview page!
Will fix this :)
~~Will fix this too~~. This is also happening in develop so is a general bug when we copy a quiz and go back to the quizzes pages rather than a bug introduced in this PR. We can open another issue for this.
~~Is also possible to replicate this in the current version in develop? Or is something that has been introduced in this PR~~. This is also happening in develop.

AlexVelezLl · 2024-11-08T16:04:09Z

I have fixed 2, 3 and 4 @pcenov :). 1, 5 and 6 are issues that also happen in develop so I think it will be better to have different issues for them.

pcenov · 2024-11-11T16:38:18Z

Thanks @AlexVelezLl - I confirm that 2, 3 and 4 are fixed now and I have filed follow-up issues for the rest.

While regression testing today I noticed the following issues, which again if not caused by changes made here, I'll report separately:

The Export as CSV button in Coach > Lessons is not working, seeing the following error in the console: TypeError: Cannot read properties of undefined (reading 'map'):

export.lessons.mp4

When creating/editing a quiz the icons are positioned right next to the 'Select all':

Attempting to view the Learners page as a Learner user results in seeing a blank page, instead of seeing the message "You must be signed in as an admin or coach to view this page":

AlexVelezLl · 2024-11-11T17:02:25Z

Thank you @pcenov! I have fixed numbers 1 and 2. Number 1 was also happening in develop but just fixed this as this was a quick fix.

Number 3 is part of a broader issue which is that in coach we dont have a not found or permissions error page in general, not as part of this PR, so I think this could be reported separately.

pcenov · 2024-11-12T12:05:04Z

Thanks @AlexVelezLl - I've verified that points 1 and 2 are fixed now and I didn't see any other issues while regression testing.
I've reported separately the permissions issue.

marcellamaki

Hi @AlexVelezLl - this is good to go! I did add one non-blocking question, mostly because we have some ongoing to-dos about loading state in the coach plugin, and I want to be sure I'm paying attention to the changes here that might impact how we address that. Thank you!! This was a lot of work 😅 nicely done

marcellamaki · 2024-11-12T18:47:09Z

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

 export default [
  {
    name: PageNames.GROUPS_ROOT,
    path: path(CLASS, ALL_GROUPS),
-    component: ReportsGroupListPage,
-    handler: () => {
-      store.dispatch('notLoading');


Why did we remove this notLoading dispatch? I don't mean that you shouldn't have, just want to make sure I understand if this is connected to other changes and that I understand them

Oh its because we already handle this in the showGroupsPage

kolibri/kolibri/plugins/coach/assets/src/composables/useGroups.js

Line 22 in 87418dd

store.dispatch('notLoading');

that was needed to show this report groups page.

github-actions bot added APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend labels Oct 30, 2024