Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update coach routing #11224

Merged

Conversation

thanksameeelian
Copy link
Contributor

@thanksameeelian thanksameeelian commented Sep 12, 2023

Summary

this PR updates routing in Coach, making it more parallel to Facility in terms of its behavior upon subtopic selection in the sidenav.

previously, if a user was working elsewhere in kolibri and selected a Coach subtopic from the sidenav, we'd only retain & apply that subtopic information if they had a single facility holding single class. otherwise, they'd be redirected to All Facilities or All Classes as appropriate and, once making the necessary selection, would be redirected to the class they wanted, but always to the home page regardless of their initially chosen subtopic.

with the changes introduced here, now if a user selects a subtopic in the sidenav, that subtopic information will persist within the url as the user selects a facility and/or specific class, and once class selection is made the user is redirected to that selected subtopic, rather than the home page by default.

References

fixes #10650
fixes #10855

Reviewer guidance

there are four cases to be considered:

  1. multi-facility user, in a facility with one class
  2. multi-facility user, in a facility with multiple classes
  3. single facility user, in a facility with one class
  4. single facility user, in a facility with multiple classes

for each case,

  • from another plugin in kolibri, select a Coach subtopic - Plan or Reports will show the changed behavior
  • navigation should behave as normal for the case you are testing - a redirect to "All facilities" and/or "All classes" as needed to winnow down class selection
  • the subtopic initially selected should persist within the current page's url & also be included in the facility- or class-specific links on the "All facilities" and "All classes" pages
  • once class selection is made, user should be redirected to the subtopic initially selected (rather than "Class home," as was previously always the case)

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 github-actions bot added APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) SIZE: medium labels Sep 12, 2023
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense so far - one question about the non-multi-facility case though!

kolibri/plugins/coach/assets/src/routes/index.js Outdated Show resolved Hide resolved
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nearly there - just one more thing.

kolibri/plugins/coach/assets/src/routes/index.js Outdated Show resolved Hide resolved
@thanksameeelian thanksameeelian marked this pull request as ready for review September 14, 2023 20:30
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All changes make sense, and manual testing checks out.

@rtibbles rtibbles merged commit 9a4b49b into learningequality:release-v0.16.x Sep 14, 2023
@rtibbles
Copy link
Member

@radinamatic I had manually confirmed these fixes, so have merged, but feel free to check my working!

@pcenov
Copy link
Member

pcenov commented Sep 15, 2023

Thanks @thanksameeelian I've also manually confirmed the fixes here and was able to identify just a couple of minor issues which I've reported separately here #11253

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) SIZE: medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issues with coach routing/side nav when there is more than one class update Coach routing behavior
3 participants