Show enroll links on public courses only if self enrollment allowed #19837

DevanR · 2019-02-20T17:49:50Z

This PR changes the logic for displaying enroll messages on the course home page.
With this change, public courses will show "Enroll" links only if self-enrollment is allowed

Discussions: Link to any public dicussions about this PR or the design/architecture. Otherwise omit this.

Screenshots:
View when User is unenrolled for Public Outline course (invite-only), before change.

View when User is unenrolled for Public Outline course (invite-only), after change.

View when User is unenrolled for Public course (self-enrollment allowed), before change.

View when User is unenrolled for Public course (self-enrollment allowed), after change.

View when User is anonymous for Public course (self-enrollment allowed), before change.

View when User is anonymous for Public course (self-enrollment allowed), after change.

Sandbox URL:
Studio:https://studio-pr19837.sandbox.opencraft.hosting/
LMS:https://pr19837.sandbox.opencraft.hosting/

Merge deadline:"None"

Testing instructions:
Contains 2 courses:

[edX Demo Course] has seo.enable_anonymous_courseware_access course waffle flag, and the "Advanced Settings > Course Visibility For Unenrolled Learners" set to public_outline and the "Advanced Settings > Invitation Only " set to true.
https://pr19837.sandbox.opencraft.hosting/courses/course-v1:edX+DemoX+Demo_Course/

[Test Course] has seo.enable_anonymous_courseware_access course waffle flag, and the "Advanced Settings > Course Visibility For Unenrolled Learners" set to public and the "Advanced Settings > Invitation Only " set to false.
https://pr19837.sandbox.opencraft.hosting/courses/course-v1:opencraft+test01+2019_T1/

Go to LMS
Sign in
View the edX Demo Course course page
You should not see an Enroll message

Go to LMS
Sign in
View the Test Course course page of the public course you've created
You should see an Enroll message

Open the same link in an anonymous browser
You should not see an Enroll message

Reviewers

Pooja Kulkarni (pkulkark)

Settings

EDXAPP_SESSION_COOKIE_DOMAIN: ".{{ EDXAPP_LMS_BASE }}"
EDXAPP_LOGIN_REDIRECT_WHITELIST:
- "{{ EDXAPP_CMS_BASE }}"

openedx-webhooks · 2019-02-20T17:50:11Z

Thanks for the pull request, @DevanR! I've created OSPR-3099 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:

supporting documentation
edx-code email 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 still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

natabene · 2019-02-20T19:05:53Z

@DevanR Thank you for your contribution. Please let me know once checks pass and this is ready for our review.

lms/djangoapps/courseware/views/views.py

openedx/features/course_experience/tests/views/test_course_home.py

openedx/features/course_experience/views/course_home_messages.py

pkulkark

Thanks for getting this done @DevanR ! Just a couple more things to take care of. Also, can you provide a brief (one-liner) description for each of the screenshots?

openedx/features/course_experience/views/course_home_messages.py

openedx/features/course_experience/tests/views/test_course_home.py

lms/djangoapps/courseware/views/views.py

openedx/features/course_experience/tests/views/test_course_home.py

natabene · 2019-02-26T15:15:29Z

@marcotuts Would you like to give it a first look?

pkulkark · 2019-02-27T07:25:37Z

openedx/features/course_experience/tests/views/test_course_home.py

+        if enable_unenrolled_access and course_visibility == COURSE_VISIBILITY_PUBLIC:
+            if user_type == CourseUserType.UNENROLLED and self.course.invitation_only:
+                if expected_enroll_message:
+                    self.assertContains(response, 'You must be enrolled in the course to see course content.')


The message text to be checked is You should be enrolled in the course to fully access the contents.
It looks like this is not getting checked since the course's invitation_only is set to False. Is it possible to set the invitation_only to True and fetch the response again before this step? If not, you can create a new course like you did earlier with invitation_only set to True (but no need to add a new ddt data). Note that if you create a new course, you will need to add a check for invitation_only in https://github.com/edx/edx-platform/blob/master/openedx/features/course_experience/tests/views/test_course_home.py#L668 and https://github.com/edx/edx-platform/blob/master/openedx/features/course_experience/tests/views/test_course_home.py#L675.

Nice catch. I can creat a private course in the setup and make sure that test uses the reponse from that. In that way I will not need to modify any other tests.

if enable_unenrolled_access and course_visibility == COURSE_VISIBILITY_PUBLIC: if user_type == CourseUserType.UNENROLLED and self.private_course.invitation_only: if expected_enroll_message: self.assertContains(private_response, 'You should be enrolled in the course to fully access the contents.')

Is this ok with you?

Yup that should be fine. Make sure you handle the check in the other two lines mentioned in my previous comment as well. Otherwise the test cases will fail.

I tested it and the additional tests you mentioned don't actually fail. It's becaue those tests are not using the private course (self.private_course) but are using the public one (self.course). Anyways, I'm pushing the changes to do a full run of tests.

Ah perfect! I missed that. Thanks.

marcotuts · 2019-02-27T15:59:26Z

@DevanR - Quick product input and questions here.

When an unenrolled student (non-staff) account accesses invite only courses and is logged in - can we redirect to the dashboard page or mark the page as unavailable? (aka 403)?
When a not logged in account account accesses invite only courses - should we redirect to login? (this will then trigger Put back in the blocks for Advanced Settings. #1 if the student is unenrolled)

Previous to adding this message / banner in 2017 (?) we forced login for learners attempting to access the course home or course content pages. Is providing any level of visibility into an invite only-course's home page what we should do for invite only? Perhaps we should just jump to the earlier redirect rules for invite only courses? Any challenges with this approach?

pkulkark · 2019-02-27T16:42:04Z

@marcotuts Not sure if this covers what you're asking, but it depends on what the course visibility is set to. By default, it will be set to private which does not change the earlier behaviour i.e. you will still see the banner warnings and the enroll messages that were shown to anonymous and unenrolled users. If the course visibility is set to public and the course is invite only, it is assumed that the course creators/admins would like certain parts of the course to be accessible by anyone but others (hidden using content groups) should only be accessible by those who have been invited. In this case, we don't display any messages/warnings to anonymous users but for unenrolled users, we display a message in the course home page that they will need to be enrolled in the course to fully access it. If the course allows self-enrollment, we provide the link to enroll into the course (to registered but unenrolled users).

pkulkark · 2019-02-28T06:11:05Z

@DevanR Thanks for getting this done! I have reviewed and tested on the sandbox. It looks good to me. Can you format the course links properly in the testing instructions? Looks like you put them within triple quotes (```) which are disabling the actual formatting.

pkulkark

I tested this by setting the course visibility to public and enabling invitation_only and verified that the correct messages are displayed.
I read through the code
I checked for accessibility issues. N/A.
Includes documentation

pkulkark · 2019-02-28T06:18:33Z

@natabene This is good for a review now.

DevanR · 2019-02-28T09:47:28Z

@DevanR Thanks for getting this done! I have reviewed and tested on the sandbox. It looks good to me. Can you format the course links properly in the testing instructions? Looks like you put them within triple quotes (```) which are disabling the actual formatting.

@pkulkark Will do that and update OCIM instances.

natabene · 2019-02-28T15:47:55Z

@marcotuts Are you ok with moving this to engineering review?

marcotuts · 2019-03-04T16:28:54Z

Got it, thanks for clarifying @DevanR. Sounds like this helps clean up the behavior within a certain combination of public + invite only. Ok to move to engineering review from the product standpoint @natabene

pomegranited · 2019-03-05T01:16:57Z

Thank you @marcotuts and @DevanR !

FYI @natabene , @ormsbee has been doing the engineering reviews for these Anonymous Courseware tickets, so it would be great if he has time to look at this one too.

We're hoping to get this into Ironwood as well since it's technically a bugfix, but of course is up to @nedbat .

natabene · 2019-03-05T17:31:41Z

@ormsbee Could you give this a look?

openedx/features/course_experience/views/course_home_messages.py

ormsbee

Please squash your commits and mention invitation_only in your commit message (I don't think most people know what controls self-enrollment).

Thank you. And thank you to @pomegranited for making the only change request that I would have (the text update).

This commit updates the Info messages on the course page to display more informative messages to the user. Specifically, messages for Public and Invitation-only courses.

pomegranited · 2019-03-12T22:38:16Z

jenkins run bokchoy

pomegranited · 2019-03-12T23:32:01Z

Thanks for your review @ormsbee -- I think these bokchoy failures are flaky tests, but unrelated to this code change:

Is this ok to merge?

ormsbee · 2019-03-13T15:11:31Z

jenkins run bokchoy

ormsbee · 2019-03-13T15:18:49Z

Yeah. I was worried because I saw the failing test was PagesTest.test_users_can_toggle_visibility, but yeah, previous runs do look like they were just flaky, so I'm okay to merge it. Giving it one last kick to bokchoy, but unless the same thing fails, I'll merge.

edx-status-bot · 2019-03-13T15:57:31Z

Your PR has finished running tests. There were no failures.

openedx-webhooks · 2019-03-13T18:25:56Z

@DevanR 🎉 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 · 2019-03-13T19:13:56Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Thursday, March 14, 2019.

edx-pipeline-bot · 2019-03-14T17:40:43Z

EdX Release Notice: This PR may have caused e2e tests to fail on Stage. If you're a member of the edX org, please visit #e2e-troubleshooting on Slack to help diagnose the cause of these failures. Otherwise, it is the reviewer's responsibility. E2E tests have failed. https://gocd.tools.edx.org/go/tab/pipeline/history/STAGE_edxapp_M-D

edx-pipeline-bot · 2019-03-15T14:54:28Z

EdX Release Notice: This PR has been deployed to the production environment.

openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Feb 20, 2019

openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels Feb 20, 2019

pkulkark reviewed Feb 22, 2019

View reviewed changes

lms/djangoapps/courseware/views/views.py Outdated Show resolved Hide resolved

pkulkark reviewed Feb 22, 2019

View reviewed changes

openedx/features/course_experience/tests/views/test_course_home.py Outdated Show resolved Hide resolved

pkulkark reviewed Feb 22, 2019

View reviewed changes

openedx/features/course_experience/views/course_home_messages.py Outdated Show resolved Hide resolved

pkulkark requested changes Feb 22, 2019

View reviewed changes

pkulkark reviewed Feb 25, 2019

View reviewed changes

openedx/features/course_experience/views/course_home_messages.py Outdated Show resolved Hide resolved

pkulkark reviewed Feb 25, 2019

View reviewed changes

openedx/features/course_experience/tests/views/test_course_home.py Outdated Show resolved Hide resolved

pkulkark reviewed Feb 26, 2019

View reviewed changes

lms/djangoapps/courseware/views/views.py Outdated Show resolved Hide resolved

pkulkark reviewed Feb 26, 2019

View reviewed changes

openedx/features/course_experience/tests/views/test_course_home.py Outdated Show resolved Hide resolved

openedx-webhooks added product review PR requires product review before merging and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Feb 26, 2019

pkulkark reviewed Feb 27, 2019

View reviewed changes

pkulkark approved these changes Feb 28, 2019

View reviewed changes

DevanR force-pushed the opencraft/edx-platform/devan/show-enroll-links-on-public-courses-only-if-self-enrollment-allowed branch from 06778f0 to 89aadfa Compare February 28, 2019 14:01

natabene mentioned this pull request Feb 28, 2019

Opencraft/edx platform/devan/show enroll links on public courses only if self enrollment allowed #19897

Closed

2 tasks

openedx-webhooks added awaiting prioritization and removed product review PR requires product review before merging labels Mar 4, 2019

natabene requested a review from ormsbee March 5, 2019 17:31

pkulkark mentioned this pull request Mar 11, 2019

Public Courseware Access open-craft/edx-platform#158

Merged

DevanR changed the title ~~Opencraft/edx platform/devan/show enroll links on public courses only if self enrollment allowed~~ Show enroll links on public courses only if self enrollment allowed Mar 11, 2019

openedx-webhooks added engineering review and removed awaiting prioritization labels Mar 11, 2019

pomegranited reviewed Mar 12, 2019

View reviewed changes

openedx/features/course_experience/views/course_home_messages.py Outdated Show resolved Hide resolved

ormsbee approved these changes Mar 12, 2019

View reviewed changes

Show Enroll links on public courses only if self-enrollment is allowed

c6f87d4

This commit updates the Info messages on the course page to display more informative messages to the user. Specifically, messages for Public and Invitation-only courses.

DevanR force-pushed the opencraft/edx-platform/devan/show-enroll-links-on-public-courses-only-if-self-enrollment-allowed branch from 13ed3af to c6f87d4 Compare March 12, 2019 21:25

ormsbee merged commit 07e93bc into openedx:master Mar 13, 2019

pomegranited deleted the opencraft/edx-platform/devan/show-enroll-links-on-public-courses-only-if-self-enrollment-allowed branch March 15, 2019 01:13

clemente mentioned this pull request Apr 2, 2019

[WIP] SE-760 Public Courses: Show "Enroll" links on public courses only if self-enrollment is allowed #19822

Closed

2 tasks

openedx-webhooks added merged and removed engineering review labels Jan 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show enroll links on public courses only if self enrollment allowed #19837

Show enroll links on public courses only if self enrollment allowed #19837

DevanR commented Feb 20, 2019 •

edited

Loading

openedx-webhooks commented Feb 20, 2019

natabene commented Feb 20, 2019

pkulkark left a comment

natabene commented Feb 26, 2019

pkulkark Feb 27, 2019

DevanR Feb 27, 2019 •

edited

Loading

pkulkark Feb 27, 2019

DevanR Feb 27, 2019

pkulkark Feb 28, 2019

marcotuts commented Feb 27, 2019

pkulkark commented Feb 27, 2019

pkulkark commented Feb 28, 2019

pkulkark left a comment

pkulkark commented Feb 28, 2019

DevanR commented Feb 28, 2019

natabene commented Feb 28, 2019

marcotuts commented Mar 4, 2019

pomegranited commented Mar 5, 2019

natabene commented Mar 5, 2019

ormsbee left a comment

pomegranited commented Mar 12, 2019

pomegranited commented Mar 12, 2019

ormsbee commented Mar 13, 2019

ormsbee commented Mar 13, 2019

edx-status-bot commented Mar 13, 2019

openedx-webhooks commented Mar 13, 2019

edx-pipeline-bot commented Mar 13, 2019

edx-pipeline-bot commented Mar 14, 2019

edx-pipeline-bot commented Mar 15, 2019

Show enroll links on public courses only if self enrollment allowed #19837

Show enroll links on public courses only if self enrollment allowed #19837

Conversation

DevanR commented Feb 20, 2019 • edited Loading

openedx-webhooks commented Feb 20, 2019

natabene commented Feb 20, 2019

pkulkark left a comment

Choose a reason for hiding this comment

natabene commented Feb 26, 2019

pkulkark Feb 27, 2019

Choose a reason for hiding this comment

DevanR Feb 27, 2019 • edited Loading

Choose a reason for hiding this comment

pkulkark Feb 27, 2019

Choose a reason for hiding this comment

DevanR Feb 27, 2019

Choose a reason for hiding this comment

pkulkark Feb 28, 2019

Choose a reason for hiding this comment

marcotuts commented Feb 27, 2019

pkulkark commented Feb 27, 2019

pkulkark commented Feb 28, 2019

pkulkark left a comment

Choose a reason for hiding this comment

pkulkark commented Feb 28, 2019

DevanR commented Feb 28, 2019

natabene commented Feb 28, 2019

marcotuts commented Mar 4, 2019

pomegranited commented Mar 5, 2019

natabene commented Mar 5, 2019

ormsbee left a comment

Choose a reason for hiding this comment

pomegranited commented Mar 12, 2019

pomegranited commented Mar 12, 2019

ormsbee commented Mar 13, 2019

ormsbee commented Mar 13, 2019

edx-status-bot commented Mar 13, 2019

openedx-webhooks commented Mar 13, 2019

edx-pipeline-bot commented Mar 13, 2019

edx-pipeline-bot commented Mar 14, 2019

edx-pipeline-bot commented Mar 15, 2019

DevanR commented Feb 20, 2019 •

edited

Loading

DevanR Feb 27, 2019 •

edited

Loading