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.

Sign up for GitHub

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

Jump to bottom

fix: Handle session timeout during Quiz Creation in Coach #11875

Closed
wants to merge 3 commits into from
Closed

fix: Handle session timeout during Quiz Creation in Coach #11875

wants to merge 3 commits into from

Conversation

GarvitSinghal47
Copy link
Contributor

@GarvitSinghal47 GarvitSinghal47 commented Feb 14, 2024 •
edited
Loading

Fixes #11773

Summary

  1. Modified the router.beforeEach guard in Coach to handle session timeouts.
  2. Added a check for user login status.
  3. If the session times out, redirected users to the 'AuthMessage' page with the 'next' query parameter set to the path they were trying to access.
  4. After signing in, users are redirected back to the original destination.

…

References

…

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 github-actions bot added APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend SIZE: small labels Feb 14, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 14, 2024 •
edited
Loading

Build Artifacts

Asset type Download link
PEX file kolibri-.pex
Windows Installer (EXE) kolibri-0.16.1.dev0+git.20240414101445-unsigned.exe
Debian Package kolibri_0.16.1.dev0+git.20240414101445-0ubuntu1_all.deb
Mac Installer (DMG) kolibri-0.16.1.dev0+git.20240414101445-0.4.0.dmg
Android Package (APK) kolibri-0.16.1.dev0+git.20240414101445-0.1.0-debug.apk
TAR file kolibri-0.16.1.dev0+git.20240414101445.tar.gz
WHL file kolibri-0.16.1.dev0+git.20240414101445-py2.py3-none-any.whl

@MisRob MisRob added the TODO: needs review Waiting for review label Feb 15, 2024
@nucleogenesis nucleogenesis self-assigned this Feb 16, 2024
nucleogenesis
nucleogenesis reviewed Feb 16, 2024
View reviewed changes
Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

Thank you @GarvitSinghal47 - this is a much better user experience when timing out.

@AllanOXDi
Copy link
Member

I am not well-versed- still learning about this. @rtibbles can you help and give a final look and we it merged?

rtibbles
rtibbles requested changes Mar 1, 2024
View reviewed changes
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.

There are a couple of issues with the current approach, and I think it might be neater to use the existing permissions tracking components for this.

kolibri/plugins/coach/assets/src/app.js Outdated
@@ -83,6 +83,18 @@ class CoachToolsModule extends KolibriApp {
} else {
next();
}

if (!this.store.getters.isUserLoggedIn) {
Copy link
Member

Choose a reason for hiding this comment

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

Hrm, it might be worth investigating the withAuthMessage component defined here:

kolibri/kolibri/plugins/device/assets/src/views/withAuthMessage.js

Line 17 in 4dae628

export default function withAuthMessage(component, authorizedRole) {
(search the code base for other uses), alternatively, the AuthMessage component can be used inside the Coach index page.

This particular implementation will kind of work, although I am fairly sure the next query parameter will be lost during the redirect to the auth page.

It is also possible for Kolibri to be configured to redirect unauthenticated users to the Learn page, so in that case, this redirect will not prompt someone to login.

Copy link
Member

Choose a reason for hiding this comment

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

@GarvitSinghal47 -- would you be able to revise this PR according to the feedback above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nucleogenesis , my apologies for the oversight. I completely forgot about this open pull request. I will make the necessary adjustments and provide the updates promptly.

@GarvitSinghal47
Copy link
Contributor Author

GarvitSinghal47 commented Mar 30, 2024 •
edited
Loading

@rtibbles, I attempted something and am simply seeking clarification: Is the video below the desired output we're aiming for?

User.Sign.In.-.Kolibri.mp4

@rtibbles
Copy link
Member

Yes, that looks like the same sort of pattern as we have used elsewhere.

@MisRob MisRob requested a review from nucleogenesis April 27, 2024 07:59
nucleogenesis
nucleogenesis requested changes May 7, 2024
View reviewed changes
Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

@GarvitSinghal47 - did something get lost in a merge or something? Seems there is only a single small change here.

@nucleogenesis nucleogenesis mentioned this pull request May 23, 2024
Closed
1 task
@MisRob
Copy link
Member

MisRob commented Jun 11, 2024

Hi @GarvitSinghal47, just chiming in - how are things going? Can we do something to support merging this work?

@GarvitSinghal47
Copy link
Contributor Author

Hi @MisRob ,

What I've observed in this issue is that there is already a system present for redirection and handling, but it is not working when the session expires or ends. This can be easily achieved by just passing the next parameter at the appropriate position, and the behavior in the above video is achieved using that.

I'd like to help further if I've misunderstood the required behavior once one of the core members reviews and confirms it. As mentioned above, I want to clarify that nothing is lost between merges.

@MisRob
Copy link
Member

MisRob commented Jun 13, 2024

@GarvitSinghal47 Ah I see, so is it you who needs to check on the merges at some point, right? Just trying to understand if you're waiting for something from the core team. Thank you.

@nucleogenesis
Copy link
Member

Closed by #12414

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nucleogenesis nucleogenesis nucleogenesis requested changes

@AllanOXDi AllanOXDi Awaiting requested review from AllanOXDi

@rtibbles rtibbles Awaiting requested review from rtibbles

Assignees

@nucleogenesis nucleogenesis

Labels
APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend SIZE: small SIZE: very small TODO: needs review Waiting for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Quiz creation route not redirecting to user_auth on 401/403
5 participants
@GarvitSinghal47 @AllanOXDi @rtibbles @MisRob @nucleogenesis