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

Redirect for Bookmarks when user is not logged in #9142

Merged

Conversation

sairina
Copy link
Contributor

@sairina sairina commented Feb 26, 2022

Summary

Added a check to see whether or not a user is logged in. If a user is not logged in, they are redirected to the "content unavailable" view.

References

Fixes #9057

Reviewer guidance

Follow the guidance from the original issue:

  1. Select "Allow users to explore resources without signing in" in device settings as an admin
  2. Sign out
  3. Type /en/learn/#/bookmarks URL to the browser address bar
  4. Make sure that you see the image below

message

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

@sairina sairina requested a review from MisRob February 26, 2022 06:36
@pcenov
Copy link
Member

pcenov commented Feb 28, 2022

Seems that this is partially fixed because navigating to /en/learn/#/bookmarks as a signed out user results in the user seeing the /en/learn/#/resources-unavailable page while as pointed out above the expected result is for the user to see /en/device/#/permissions.

Observed:
2022-02-28_12-32-36

Expected:
2022-02-28_12-47-43

@MisRob
Copy link
Member

MisRob commented Feb 28, 2022

I can confirm, I was just in the middle of the review when I saw this too.

Also, after this is resolved, we also need to make sure that for "Allow users to explore resource without signing in" device settings, users will get redirected to the bookmarks page after they sign in. This can be achieved by using the next query parameter similarly it's currently done for "Learners must sign in to explore resources" device settings (see next=http%253A%252F%252F127.0.0.1%253A8000%252Fen%252Flearn%252F%2523%252Fbookmarks in the URL)
02

MisRob
MisRob previously requested changes Feb 28, 2022
Copy link
Member

@MisRob MisRob 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, @sairina, it's definitely better than it was. It would be good to resolve the issues mentioned above before merging.

@sairina sairina force-pushed the bookmark-logged-out branch from ee5de0a to eb7cd94 Compare March 2, 2022 22:17
@sairina sairina requested a review from MisRob March 2, 2022 22:17
@sairina sairina force-pushed the bookmark-logged-out branch from eb7cd94 to fcc472e Compare March 2, 2022 22:20
@sairina sairina added TODO: needs QA re-review For stale issues that need to be revisited TODO: needs review Waiting for review labels Mar 3, 2022
@nucleogenesis nucleogenesis self-requested a review March 3, 2022 23:32
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.

I think this is best targeted in LearnIndex at the moment, not in the router - router guards are best used to redirect to other pages, not conditionalize behaviour within a page.

kolibri/plugins/learn/assets/src/routes/index.js Outdated Show resolved Hide resolved
kolibri/plugins/learn/assets/src/app.js Outdated Show resolved Hide resolved
@sairina sairina force-pushed the bookmark-logged-out branch from f1fcc63 to cf73708 Compare March 4, 2022 18:11
@sairina sairina requested a review from rtibbles March 4, 2022 18:11
@sairina
Copy link
Contributor Author

sairina commented Mar 4, 2022

With the changes, this is the behavior:

User (not logged in) goes to /learn/#/bookmarks and clicks on "sign in" link User is able to sign in (note the update in the URL)
Screen Shot 2022-03-04 at 9 58 26 AM Screen Shot 2022-03-04 at 9 58 39 AM

@@ -95,6 +95,9 @@
...mapState('examReportViewer', ['exam']),
...mapState(['pageName']),
userIsAuthorized() {
if (this.pageName === PageNames.BOOKMARKS) {
Copy link
Member

Choose a reason for hiding this comment

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

Delightfully simple!

@rtibbles rtibbles dismissed MisRob’s stale review March 4, 2022 18:16

Changes addressed!

@sairina sairina merged commit 660ca68 into learningequality:release-v0.15.x Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs QA re-review For stale issues that need to be revisited TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Signed-out user can access the bookmarks page when users are allowed to explore resources without signing in
4 participants