Skip to content

feat: set course for wiki based on the wiki_slug#33338

Merged
Agrendalath merged 1 commit intoopenedx:masterfrom
open-craft:maxim/fixing-navigation-from-wiki-back-to-course
Oct 16, 2023
Merged

feat: set course for wiki based on the wiki_slug#33338
Agrendalath merged 1 commit intoopenedx:masterfrom
open-craft:maxim/fixing-navigation-from-wiki-back-to-course

Conversation

@Cup0fCoffee
Copy link
Contributor

@Cup0fCoffee Cup0fCoffee commented Sep 25, 2023

Description

Learners want to have the usual course navigation when viewing a wiki, so that they can go back to the course related to the wiki and browse other tabs/sections of the course.

Wiki reads the course from the request.course. If it's not present, i.e. None or not set on the request, it will not show the course navigation UI.

It seems like WikiAccessMiddleware already has the code that parses course id from the request (when the request is for a wiki view) and sets the course for the request. However, it doesn't work in most scenarios, because the course id is not in the it's normal format in most requests that go to wiki.

For example, when a leaner clicks on a wiki tab from the course overview, they are redirected to /wiki/<wiki_slug>/ path. The wiki slug is taken from course's wiki_slug field. This slug can be used to figure out what course this wiki belongs to in most (not all) cases.

This commit adds code to the WikiAccessMiddleware that attempts to find a course based on wiki slug, and in case of success, sets the course to the request.course, so that wiki can display course navigation UI.

Testing instructions

  • Setup devstack of latest version

  • make dev.up.lms+cms+frontend-app-learning

  • Log in at localhost:18000 as edx@example.com (edx as password)

  • Enroll into the demo course

  • Go to studio for this course, in the navigation find "Content -> Pages", click on it, and make the Wiki tab visible by clicking the crossed eye icon

  • Go back to the course, and click the wiki tab

  • You will see that the wiki doesn't have any course tabs, there is no way to go back to the course you came from, etc.

    Screenshot

    image

  • Now checkout the branch from the PR

  • Go back to the wiki page and reload (it might need a minute for the dev server to restart)

  • You will now see that the course navigation UI is showing correctly.

    Screenshot

    image

  • Check that the links are pointing to the correct course

Deadline

"None"

Notes

This solution doesn't work well when there a lot of courses that have the same wiki_slug. (#31794 (comment))

And alternative or complementary solution could be to, when the learners click on the wiki tab, instead of redirecting them to /wiki/<path>, redirect them to /courses/<course_id>/wiki/<path>. Such urls are supported, and the middleware would be able to pick up the course id from the url. One of the issue with this solution, is that it seems like these views were deliberately made un-reversable, so we would have to hard code the url. Essentially, the solution that would not rely on the wiki_slug would only require the following change:

--- a/lms/djangoapps/course_wiki/views.py
+++ b/lms/djangoapps/course_wiki/views.py
@@ -98,7 +98,7 @@ def course_wiki_redirect(request, course_id, wiki_path=""):
                             'other_write': True,
                             })

-    return redirect("wiki:get", path=urlpath.path)
+    return redirect(f"/courses/{course_id}/wiki/{urlpath.path}")

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Sep 25, 2023
@openedx-webhooks
Copy link

Thanks for the pull request, @Cup0fCoffee! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@mphilbrick211
Copy link

Hi @Cup0fCoffee! Is this ready for review?

@Cup0fCoffee
Copy link
Contributor Author

@mphilbrick211 Yes. I think @Agrendalath was planning to review it in the upcoming week or two as a core committer.

@Agrendalath Agrendalath force-pushed the maxim/fixing-navigation-from-wiki-back-to-course branch from c985eeb to cc6bda8 Compare October 15, 2023 15:55
Copy link
Member

@Agrendalath Agrendalath left a comment

Choose a reason for hiding this comment

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

👍

  • I tested this: verified that the "Course" tab is displayed in wiki
  • I read through the code
  • I checked for accessibility issues: n/a
  • Includes documentation: n/a
  • I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository: n/a

@Cup0fCoffee Cup0fCoffee force-pushed the maxim/fixing-navigation-from-wiki-back-to-course branch from cc6bda8 to 789e299 Compare October 16, 2023 12:10
@Cup0fCoffee
Copy link
Contributor Author

@Agrendalath I'm not sure why the checks are marked as failed. Something cancelled the jobs that were running. I'll leave it as is for now.

Learners want to have the usual course navigation when viewing a wiki,
so that they can go back to the course related to the wiki and browse
other tabs/sections of the course.

Wiki reads the course from the `request.course`. If it's not present,
i.e.  None or not set on the request, it will not show the course
navigation UI.

It seems like `WikiAccessMiddleware` already has the code that parses
course id from the request (when the request is for a wiki view) and
sets the course for the request. However, it doesn't work in most
scenarios, because the course id is not in the it's normal format in
most requests that go to wiki.

For example, when a leaner clicks on a wiki tab from the course
overview, they are redirected to `/wiki/<wiki_slug>/` path. The wiki
slug is taken from course's `wiki_slug` field. This slug can be used to
figure out what course this wiki belongs to in most (not all) cases.

This commit adds code to the `WikiAccessMiddleware` that attempts to
find a course based on wiki slug, and in case of success, sets the
course to the `request.course`, so that wiki can display course
navigation UI.
@Agrendalath Agrendalath force-pushed the maxim/fixing-navigation-from-wiki-back-to-course branch from 789e299 to bd31c7c Compare October 16, 2023 13:42
@Agrendalath Agrendalath merged commit 755fa7f into openedx:master Oct 16, 2023
@Agrendalath Agrendalath deleted the maxim/fixing-navigation-from-wiki-back-to-course branch October 16, 2023 19:16
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@openedx-webhooks
Copy link

@Cup0fCoffee 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

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

Labels

open-source-contribution PR author is not from Axim or 2U

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants