WIP: Remove warnings for public courses by pkulkark · Pull Request #19624 · openedx/openedx-platform

pkulkark · 2019-01-18T07:58:54Z

Work in progress : Do not merge. We will ping once our internal review is done.

This PR depends on #19385.

Description:

Currently the warnings show up for anonymous and unenrolled users irrespective of whether the course is marked public. This PR hides the different warnings and messages asking the user to sign-in and enroll in the course, when the course is marked public.
This PR also modifies the default public_view text to include the component display_name when unenrolled access is not available.

Sandbox server:

Contains 2 course:

edX Demo Course has seo.enable_anonymous_courseware_access course waffle flag, and the "Advanced Settings > Course Visibility For Unenrolled Learners" set to private. Anonymous and unenrolled users can see the warning messages.
Test Course has seo.enable_anonymous_courseware_access course waffle flag, and the "Advanced Settings > Course Visibility For Unenrolled Learners" set to public. Anonymous and unenrolled users should not see any of the warning messages. Unit-2 is a library content which does not have unenrolled access. The warning message should include the display_name (in this case, Randomised content block).

Testing Instructions:

If testing on devstack, create a course and mark the course as public.
Access the course as an anonymous or unenrolled user. There should not be any warning messages asking the user to sign in or register or enroll in the course.
Add any content that does not support unenrolled access (library content for example) and access the course as an anonymous or unenrolled user. The warning message should contain the display_name if the component has one.

Shows "View Course" link to the course outline on the course about page, for courses marked public or public_outline. Makes course handouts public only for courses marked as public.

This fix hides the warnings for unenrolled users when the course is marked public and modifies the default public_view message to include display_name if exists.

openedx-webhooks · 2019-01-18T07:59:01Z

Thanks for the pull request, @pkulkark! I've created OSPR-2996 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.

SSPJ

This looks great Pooja! I've left a minor style comment and a question about functionality that you might know the answer to or might want to clarify with Jill.

I'm sorry to say that I don't know the issue with Jenkins. It may be the jobs aren't kicked off for wip PRs, but I'm sure Natalia when you are ready to ping her will know.

SSPJ · 2019-01-22T13:37:22Z

                # Save back any changes
-                if any(block_keys[changed] for changed in ('invalid', 'overlimit', 'added')):
+                if usage_info.user.is_authenticated() and any(block_keys[changed]
+                                                              for changed in ('invalid', 'overlimit', 'added')):


How about refactoring the two line one-liner? Something like:

block_keys_changed = any(block_keys[changed] for changed in ('invalid', 'overlimit', 'added')) if usage_info.user.is_authenticated() and block_keys_changed:

This is from an earlier PR and was based on the review comments. Once that is merged, it won't show up here.

SSPJ · 2019-01-22T13:42:45Z

            )
        else:
-            if not CourseEnrollment.is_enrolled(request.user, course_key):
+            if not CourseEnrollment.is_enrolled(request.user, course_key) and not allow_anonymous:


Will this line prevent an unenrolled (but logged in) student from enrolling in the class?

The idea is that there's no need for enrollment if the course is marked public. Marking a course as public would mean anybody, whether a registered student or not can access the course contents.

Yup, I got that idea :) I just wondered if things like grades, etc, would be effected.

Ah yes! As per the proposal #18134 , the parts of the course that contains exams and grading would not be allowed public access. Do you think having a message like below in the course landing page makes more sense in that case?

I've thought about it and I think the answer is no. The message doesn't explain what parts of the course are hidden, leaving a student potentially confused about whether they should click the button or not. For now, let's assume that if a course is public, the instructor will explicitly invite any students who need to be enrolled for grading or exams.

Fair enough :-)

SSPJ · 2019-01-22T13:48:24Z

+            CourseHomeMessages.register_info_message(
+                request,
+                Text(_(
+                    '{open_enroll_link}Enroll now{close_enroll_link} to access the full course.'


Same question as above. If a course is marked for public access, how do students enrol?

symbolist · 2019-01-23T17:32:52Z

+        if self.display_name:
+            display_text = DEFAULT_PUBLIC_VIEW_MESSAGE %self.display_name
+        else:
+            display_text = DEFAULT_PUBLIC_VIEW_MESSAGE %"This content"


This is not going to work with the translation system so you will need a string each for the two cases.

pkulkark · 2019-01-24T07:26:08Z

@symbolist Would the translation work fine for this?

Thanks. DEFAULT_PUBLIC_VIEW_MESSAGE will also need to be in a _() so that it gets translated. Also second sentence is missing a full-stop at the end. In case you haven't seen it, i18n docs should cover all the details.

Thanks @symbolist . I've corrected it now.

pkulkark · 2019-01-24T13:39:00Z

Hi @natabene ! Is there a way to trigger the Jenkin checks for this PR?

SSPJ · 2019-01-24T15:34:05Z

👍 I think this is ready for edX review now, @pkulkark .

I tested this: Saw warnings when logged out on a course set to private and no warnings on a course set to public.
I read through the code
~~I checked for accessibility issues~~ n/a
~~Includes documentation~~ n/a

natabene · 2019-01-26T06:31:18Z

jenkins ok to test

natabene · 2019-01-26T06:32:56Z

jenkins run all

natabene · 2019-01-26T06:33:17Z

@edx/testeng Can you help us out with these tests?

edx-status-bot · 2019-01-26T07:20:25Z

Your PR has finished running tests. The following contexts failed:

jenkins/quality

pkulkark · 2019-01-27T08:48:44Z

I have merged this change into #19385. Closing this PR in favour of #19385.

openedx-webhooks · 2019-01-27T08:48:56Z

@pkulkark Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

pkulkark added 3 commits January 17, 2019 13:27

Implement Public Cohort

ad7ea5a

Changes to about page and course handouts

196b3d4

Shows "View Course" link to the course outline on the course about page, for courses marked public or public_outline. Makes course handouts public only for courses marked as public.

Remove warnings for public courses

cdc57d4

This fix hides the warnings for unenrolled users when the course is marked public and modifies the default public_view message to include display_name if exists.

openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels Jan 18, 2019

SSPJ suggested changes Jan 22, 2019

View reviewed changes

symbolist reviewed Jan 23, 2019

View reviewed changes

pkulkark commented Jan 24, 2019

View reviewed changes

symbolist mentioned this pull request Jan 24, 2019

Implement public cohort for anonymous and unenrolled users #19385

Merged

pkulkark force-pushed the pooja/remove-warnings-for-public-courses branch from 3ad05f4 to f5ca00a Compare January 24, 2019 13:14

Ensure change is for "public" courses only

88765f6

pkulkark force-pushed the pooja/remove-warnings-for-public-courses branch from f5ca00a to 88765f6 Compare January 24, 2019 14:01

pkulkark closed this Jan 27, 2019

openedx-webhooks added rejected and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Jan 22, 2021

bradenmacdonald deleted the pooja/remove-warnings-for-public-courses branch January 12, 2022 01:59

Uh oh!

Conversation

pkulkark commented Jan 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openedx-webhooks commented Jan 18, 2019

Uh oh!

SSPJ left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pkulkark Jan 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pkulkark commented Jan 24, 2019

Uh oh!

SSPJ commented Jan 24, 2019

Uh oh!

natabene commented Jan 26, 2019

Uh oh!

natabene commented Jan 26, 2019

Uh oh!

natabene commented Jan 26, 2019

Uh oh!

edx-status-bot commented Jan 26, 2019

Uh oh!

pkulkark commented Jan 27, 2019

Uh oh!

openedx-webhooks commented Jan 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pkulkark commented Jan 18, 2019 •

edited

Loading

pkulkark Jan 22, 2019 •

edited

Loading