-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Fixes for courseware anonymous access. #18344
Fixes for courseware anonymous access. #18344
Conversation
Thanks for the pull request, @symbolist! I've created OSPR-2462 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams. Feel free to add as much of the following information to the ticket:
All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here. |
@symbolist Thanks for the PR. Feel free to ping me once this is ready. Just so you know, we've had some issues with the quality so it may not pass at moment despite there not be an issue with quality. Could you also add a Description once this is ready to be reviewed? Thanks! |
@mduboseedx I just added details to the description. Thanks for the note about the quality check! And I'll update you when this is ready to be reviewed. |
@mulby @caesar2164 also noticed this in flight PR. |
8d1265c
to
ed71b58
Compare
@xitij2000 There is quality issue but besides that this is ready for review. |
ed71b58
to
6801697
Compare
jenkins run a11y |
2d9c401
to
6801697
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
- I tested this: tested that navigation works when using the waffle flag
- I read through the code
I checked for accessibility issuesIncludes documentation
@mduboseedx Thanks! I have also added a link to a sandbox for testing purposes. |
Hi @mduboseedx, is there an estimate for when this can get reviewed? Also a heads up that OpenCraft is planning to work on some of the ideas discussed in https://github.com/edx/edx-platform/pull/18134 and will be submitting them for upstreaming. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One nitpick about the line length, but otherwise, it looks fine to me. @edx/learner: is my review enough or do you folks feel that you need to look this over as well?
@symbolist, @xitij2000: Thank you for your patience in this.
lms/templates/seq_module.html
Outdated
@@ -1,7 +1,7 @@ | |||
<%page expression_filter="h"/> | |||
<%! from django.utils.translation import ugettext as _ %> | |||
|
|||
<div id="sequence_${element_id}" class="sequence" data-id="${item_id}" data-position="${position}" data-ajax-url="${ajax_url}" data-next-url="${next_url}" data-prev-url="${prev_url}"> | |||
<div id="sequence_${element_id}" class="sequence" data-id="${item_id}" data-position="${position}" data-ajax-url="${ajax_url}" data-next-url="${next_url}" data-prev-url="${prev_url}" data-save-position="${'true' if save_position else 'false'}" data-show-completion="${'true' if show_completion else 'false'}"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Please put a line break or two in here so we don't have a 300 column line.
6801697
to
36b3316
Compare
Your PR has finished running tests. There were no failures. |
@ormsbee Thanks for the review! I have split the attributes across multiple lines as you suggested. @mduboseedx FYI |
@mduboseedx Could you check with the Learner team to see if @ormsbee 's approval here is enough to merge this? Should be squashed and merged if all is ok. |
@ormsbee I believe your review is enough here. |
Beautiful, thank you @staubina and @ormsbee ! @xitij2000 Could you please merge, since you have access to do so? CC @symbolist |
@pomegranited I don't have merge access here. I think only @bradenmacdonald does. |
Ah ok thanks @xitij2000 . @ormsbee maybe, could you merge here if you're happy with this PR? CC @mduboseedx |
@symbolist 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Thanks @mduboseedx! |
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Friday, August 17, 2018. |
EdX Release Notice: This PR has been deployed to the production environment. |
I need to test this anonymous course access but cannot understand how exactly to set the waffle flag... it does appear to be in the latest openedx build, any guidance welcome! |
Hi @micahl98, thanks for reaching out 😄 There's a DOCS PR to be completed and we're prepping a blog post about this now, but am happy to share more details about how to use this feature here. Because this is a brand new feature, there's currently a two-step process to enable it for each course. This may change in the future as demand increases and further optimizations are developed. To use this feature on your course:
|
@pomegranited: Would it cause a problem if you were to enable |
@robrap: I believe that's actually because we (edX) asked that it be restricted more tightly so that it could be rolled out on a course-by-course basis. This is a powerful feature that will likely see enthusiastic uptake from course teams, but copyright/visibility issues will likely have to be discussed at a policy level with different institutions, and we didn't want a blanket switch that let individual course teams turn it on without that discussion having happened. (@pomegranited: Please correct me if I'm misremembering.) That being said, I do think we'll want to have a better experience around this for Juniper. The belt-and-suspenders option was just the lowest risk thing that let us roll out without having to address policies. FYI @marcotuts ^ I know we're all in Ironwood + conference craziness right now, but we should talk about this after things settle down a bit. |
Yes, I agree and understand the points well made here. It would be nice
however to be able to roll this out system wide without having to specify
it for each course... in my case for example, I will be treating it as a
system wide option anyway.
…On Fri, Mar 1, 2019 at 9:58 AM David Ormsbee ***@***.***> wrote:
@robrap <https://github.com/robrap>: I believe that's actually because we
(edX) asked that it be restricted more tightly so that it could be rolled
out on a course-by-course basis. This is a powerful feature that will
likely see enthusiastic uptake from course teams, but copyright/visibility
issues will likely have to be discussed at a policy level with different
institutions, and we didn't want a blanket switch that let individual
course teams turn it on without that discussion having happened. (
@pomegranited <https://github.com/pomegranited>: Please correct me if I'm
misremembering.)
That being said, I do think we'll want to have a better experience around
this for Juniper. The belt-and-suspenders option was just the lowest risk
thing that let us roll out without having to address policies.
FYI @marcotuts <https://github.com/marcotuts> ^ I know we're all in
Ironwood + conference craziness right now, but we should talk about this
after things settle down a bit.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<https://github.com/edx/edx-platform/pull/18344#issuecomment-468713317>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAYBGVbipiStL7MMuUPlOwIqShM904szks5vSU4ugaJpZM4Uf0aZ>
.
|
@ormsbee: Just making sure that it is clear that a CourseWaffleFlag allows someone to set the flag on a per course when the waffle flag itself is unset. Or, the CourseWaffleFlag let's someone turn the flag off per course when the waffle flag itself is set to true. I understand that for edx.org we are only enabling the override per course. I was just pointing out that for an Open edX instance where someone wants this on (or available) to all courses, they could just turn on the waffle flag itself, rather than having to turn the override the flag for each course. They would still need to follow @pomegranited's second step per course, but the first step documented could be done a single time for the waffle flag if so desired. |
Thanks for catching this @robrap and @ormsbee ! I've updated my comment above to include both the site-wide and course-specific options, and will ensure this makes it into the docs and blog post too. @micahl98 -- does this update meet your needs? |
@robrap: Ah! I did not understand that about CourseWaffleFlags, though it makes perfect sense. Thank you! |
https://github.com/edx/edx-platform/pull/16315 added the ability to allow anonymous access to courseware. This PR makes small adjustments to the functionality:
Note that currently only HTML blocks work correctly with anonymous access.
JIRA tickets: OSPR-2462
Screenshots: N/A
Merge deadline: None
Sandbox: https://pr18344.sandbox.opencraft.hosting/courses/course-v1:edX+DemoX+Demo_Course/courseware/interactive_demonstrations/
Reviewers