Handle anonymous and unenrolled users on the new course home page by andy-armstrong · Pull Request #15456 · openedx/openedx-platform

andy-armstrong · 2017-07-03T17:16:47Z

LEARNER-1696

Description

This change adds support for anonymous and unenrolled users to access the new course home page. As defined in the ticket, the page hides the following for unenrolled users:

the welcome message, if any
the course outline
the Bookmarks course tool
the course sock
the "Start Course"/"Resume Course" button

The following content is still shown:

the "Course Tools" section (except for "Bookmarks")
the "Course Dates" section

This PR also changes the "Sign In" button so that the user returns to this page once they have successfully logged in. Before this change, the user would land on the dashboard, which would make it hard to get back to the course since they haven't yet enrolled.

Here's a screenshot of the test course for an anonymous user:

@lizcohen @marcotuts - note that with the absence of the welcome message and course outline that the left hand side of the page is completely empty. I'm wondering if the upgrade message should go here instead of at the top, just so as not to leave a big hole.

Note: messaging to encourage the user to enroll will be added with this story: https://openedx.atlassian.net/browse/LEARNER-1697

Sandbox

There are three different courses that I used for testing:

The first two are configured with unified course tabs on, while the third has it off.

Testing

Unit, integration, acceptance tests as appropriate

Reviewers

FYI: @dianakhuang @HarryRein @lizcohen @catong

Post-review

Rebase and squash commits

robrap · 2017-07-05T14:48:57Z

@andy-armstrong @marcotuts @lizcohen: 1. Why do we hide the "welcome message" and show the "updates" link, if it just gets you to the same message?
2. If we keep hiding the welcome message, could we provide a replacement generic welcome message?

andy-armstrong · 2017-07-05T15:24:19Z

@robrap Great questions. I noticed that course updates are shown on the old course info tab, so maybe we should keep the welcome message as well. It would solve the empty space, at least for courses that have at least one update.

andy-armstrong · 2017-07-05T15:25:56Z

Hmm, actually they are not, although the "Course Updates and News" title is still shown! Try this in the three cases (anonymous, logged in but not enrolled, and enrolled):

https://edge.edx.org/courses/course-v1:edX+AC101+2016/info

marcotuts · 2017-07-05T15:28:43Z

@robrap
1.) I think part of the point is to not trigger the logic around a given user / browser setting the welcome message cookie of 5-7 days (I forget what it is set to). Ok in my mind to let people see the updates area / messages similar to how we let people see the handouts section and important course dates, though I don't expect many will. (Fine to hide as well, I don't feel strongly either way.)
2.) Instead of a replacement welcome message I think Liz is looking into a message to replace the course outline area. (This also relates to @andy-armstrong's comment above about the empty outline area.) It isn't clear to me yet if we will want both the message in that area as well as a page level banner as we have now. (For what it's worth, I think we want the page level banner because eventually when viewing course content pages that might be the way to provide visibility into the enroll now/ sign in / register messaging. (And I don't think it would hurt to have it twice on the home page necessary, though it depends on the form factor and copy of the outline area message.) cc'ed @lizcohen

robrap

Thanks @andy-armstrong. Looks clean. Minor comments (I hope).

robrap · 2017-07-05T14:55:13Z

            and is_active is whether the enrollment is active.
        Returns (None, None) if the courseenrollment record does not exist.
        """
+        if user.is_anonymous():


Why not the same not user or user.is_anonymous() check as above?

Any reason why not to push this into cls._get_enrollment_state()?

I'm not a fan of allowing functions to be loose when handling parameters. I'll try fixing the one above to require a user to be passed (there were strange test failures that I gave up debugging but I'll try again).

Interesting idea. The logic is a little more complicated in _get_enrollment_state since it returns two values, but it simplifies two callers.

For #2, it already returns CourseEnrollmentState(None, None) when the record is missing, so it seems like it should be able to handle this.

Indeed, that's what I did.

robrap · 2017-07-05T14:56:29Z

+
+    Note: users with staff access return True even if not enrolled.
+    """
+    if user.is_anonymous():


Any reason why CourseEnrollment.is_enrolled() and has_access() couldn't take care of anonymous user appropriately?

Good point. This check doesn't seem to be necessary as both methods handle anonymous users. I'm not sure why I thought it was.

robrap · 2017-07-05T15:00:45Z


+def is_enrolled_in_course_or_staff(course_key, user):
+    """
+    Returns true if the user is enrolled in the specified course.


Seems more correct to combine into one statement:

Returns True for all staff users, or if the user is enrolled in the specified course.

I've decided to remove this method because it only adds confusion. It is easy enough to do the two checks if necessary.

robrap · 2017-07-05T15:05:21Z


 <%def name="login_query()">${
-  u"?next={0}".format(urlquote_plus(login_redirect_url)) if login_redirect_url else ""
+  u"?next={next}".format(


Great. Do we want a test around this somewhere so it doesn't vanish and it is clear why this was added?

I considered adding a test but it would have to be a Bok Choy test and I couldn't find a natural place to add this.

Why does it need to be bokchoy? Can't you just validate that the rendered template has the appropriate url under the various conditions?

I was thinking that it should verify that logging in does bring you back to the correct place. I'll happily add a simple test that the login URL is as I expect. :-)

robrap · 2017-07-05T15:10:44Z

        """
        # validate arguments
-        assert issubclass(type(course_id), CourseKey), "The course_id '{}' must be a CourseKey.".format(str(course_id))
+        assert issubclass(type(course_key), CourseKey), "The course_id '{}' must be a CourseKey.".format(


course_id => course_key
Note: I agree this param name makes more sense in this context. I feel like I found something weird where a course_key elsewhere wasn't actually a CourseKey, but was a string, and it gave me a lot of confusion, but hopefully I'm misremembering. :)

robrap · 2017-07-05T15:23:16Z

+    """
+    Test access to the course home page.
+    """
+    @override_waffle_flag(UNIFIED_COURSE_TAB_FLAG, active=True)


Do we need a test for @override_waffle_flag(UNIFIED_COURSE_TAB_FLAG, active=False) ?

I don't personally, but I can add one if you do.

It seems like a very simple test that would verify that this flag is doing what we need it to do. If it is complicated, you could skip.

robrap · 2017-07-05T15:29:20Z

        # Render the course dates as a fragment
        dates_fragment = CourseDatesFragmentView().render_to_fragment(request, course_id=course_id, **kwargs)

        # TODO: Use get_course_overview_with_access and blocks api


I know you didn't add this comment, but I'd remove the prefix TODO, and either remove this comment entirely, or update it to read something like:

Cannot use get_course_overview_with_access unless XXX.

What is XXX? Course updates are moved into xblocks? Other? If we can't provide anything useful, than just remove altogether.

robrap · 2017-07-05T15:32:32Z

        """
        course_key = CourseKey.from_string(course_id)
-        course = get_course_with_access(request.user, 'load', course_key, check_if_enrolled=True)
+        course = get_course_with_access(request.user, 'load', course_key, check_if_enrolled=False)


If (for now) we are saying that we won't display the welcome message unless the user is enrolled, why is this changing?

This was failing because the fragment was being rendered but then not included in the page. Now that the course home fragment doesn't render fragments that it doesn't need, this change isn't needed. However, I find it odd that the fragment would be verifying access and issuing page redirects. It would be better if the fragment itself just didn't render itself, but I don't want to make that change in this PR.

robrap · 2017-07-05T15:34:03Z



-class TestCourseHomePage(SharedModuleStoreTestCase):
+class CourseHomePageTestCase(SharedModuleStoreTestCase):


This is not a comment on this PR, but when reviewing names elsewhere, I noticed that none of the other new features have a tests directory. Does something seem odd about that?

Yeah, that doesn't seem ideal...

Is this a case of needing to rearrange our tests, or do we just not have unit tests for these features? It seems like a simple single smoke test for each feature would be necessary. Do we need a story to add this?

It seems we don't have unit tests at all for these features, but they kind of got tested through the course home page. I've made a one point story to add these tests:

https://openedx.atlassian.net/browse/LEARNER-1780

robrap · 2017-07-05T15:36:11Z

      % if not settings.FEATURES['DISABLE_LOGIN_BUTTON'] and not combined_login_and_register:
        % if course and settings.FEATURES.get('RESTRICT_ENROLL_BY_REG_METHOD') and course.enrollment_domain:
-          <a class="btn btn-login" href="${reverse('course-specific-login', args=[course.id.to_deprecated_string()])}${login_query()}">${_("Sign in")}</a>
+          <a class="btn btn-brand btn-login" href="${reverse('course-specific-login', args=[course.id.to_deprecated_string()])}${login_query()}">${_("Sign in")}</a>


Why was btn-brand added here and the other template?

This PR was the first time that a pattern library page could be rendered for an anonymous user, so I had to make a number of changes to the header. This one was necessary to make the button render as a blue 'brand' button, rather than a normal button with a white background.

Thanks. I didn't know where btn-brand was defined.

robrap · 2017-07-05T15:42:46Z

@andy-armstrong When navigating to your sandbox, I get bumped to the login page when I click the "Updates" link. Do we want anonymous access to that? Should that be part of this PR?

robrap · 2017-07-05T15:50:15Z

@andy-armstrong: Looks like Course Info page doesn't display much at all in Production if you are not logged in. I tested with this course: https://courses.edx.org/courses/course-v1:UCSDx+CSE167x+1T2017
Should the Course Info page keep working as it did, or do we want to make this info visible now on the Course Info page as it is? Do you want a sandbox course that displays the non-unified-course-tab experience?

andy-armstrong · 2017-07-06T03:23:16Z

@robrap

When navigating to your sandbox, I get bumped to the login page when I click the "Updates" link. Do we want anonymous access to that? Should that be part of this PR?

That's a good question. For now, I've hidden them too, which is the same behavior as on the course info page.

Should the Course Info page keep working as it did, or do we want to make this info visible now on the Course Info page as it is?

I'd rather not make any changes to the Course Info page. Hopefully it will be switched off by the end of the month, and then removed not long afterwards.

andy-armstrong · 2017-07-07T19:18:37Z

Thanks @robrap. I've addressed all of your feedback (I believe) and pushed up the changes.

robrap · 2017-07-07T19:51:02Z

This is changing the logic. You confirmed that the earlier access_response = has_access(user, action, course, course.id) would never deny access for staff?

I wonder why it was written below as it was? You think there was a performance issue calling this for every user? I'm not asking for it to go back...just curious.

Good questions:

My understanding is that this isn't changing the logic, because has_access would return true for staff. It just moves the staff check earlier to make it clearer (to my mind) that staff gets to skip all the other logic.

I'm not sure what you're saying. Are you thinking there's a performance impact to moving the staff check earlier? That probably is why some of the counts increased, so we could optimize it. However, I think that the clarity of understanding that staff always get access is a worthwhile trade off.

Thanks @andy-armstrong.

I didn't check the code, so was just double-checking with you.

I agree with going for clarity. And I am guessing that is why the count increased.

robrap · 2017-07-07T19:55:06Z

We are not showing course updates when there are two tabs? Note, this is part of why I asked for a test with UNIFIED_COURSE_TAB_FLAG as False so it was clear what should happen in that case.

I forgot about your request for that other test... I'll add one.

I'm not sure why I added this logic. It now makes sense to me that the updates should be shown the same way regardless of the value of the flag, so I'll remove that item.

I've added a test with the flag off.

robrap · 2017-07-07T20:06:27Z

Thanks for adding this. There wasn't a more general place for this? Either way, maybe you could add a note that this is a more generic test that happens to be using course home?

I couldn't think of a more general place, because we don't have tests for templates. Every page is essentially equally valid. It seemed reasonable to me that this behavior was important for the course experience. I don't see the need for a comment.

I guess the case for the comment would be to not have every template start copying this test and retesting for every template. There would be nothing entirely wrong with this, except test time and code duplication. Also, if I saw the test and wanted to find the implementation, I would know not to look in the course home code.
I'd still add a comment, but I approve you if you still don't want to. :)

robrap · 2017-07-07T20:07:16Z

robrap · 2017-07-07T20:08:11Z

robrap · 2017-07-07T20:15:13Z

I'm confused. Do we want this to be is_enrolled_or_staff or is_enrolled? Does is_enrolled() return True for staff regardless? Note, depending on whether this is a defect, or just need clearer comments, we might be missing a test that would catch this case.

I got myself a bit confused here too. I think we want to allow course staff to access a course even if they are not enrolled, so this should be is_enrolled_or_staff. If so, then I lost the check. I don't think we want is_enrolled to return True for staff that are not enrolled.

robrap · 2017-07-07T20:16:19Z

Might need a staff test? See below.

Great idea. I've added an "unenrolled_staff" test.

robrap · 2017-07-07T20:18:49Z

Is this comment wrong? Why do we have "who are not staff"?

We want to let enrolled users or staff see all the content, so we render the subset for unenrolled users who are not staff. This seems right to me.

I've rewritten this in the positive sense which makes it clearer, because the first clause of the if statement is for the positive result.

robrap · 2017-07-07T20:21:32Z

Nit:

Wondering if these render comments are useful.

Why do we feel compelled to use a different name in the comment? Should this really be a VerificationSockFragmentView, or would this always be the CourseSock and just might contain something different at some point, etc.?

I'll take the comments out.

This should be a course sock fragment IMO, because we could use it for many purposes.

andy-armstrong · 2017-07-08T11:40:00Z

@robrap Thanks for the thorough review. I've pushed up a new round of commits, so let me know what you think. I'm also updating the sandbox.

robrap · 2017-07-09T02:36:19Z

@andy-armstrong: I thought we had the Course Tools, like Course Handouts, appearing on both the Home and Course tabs when not unified. On your sample course, I only see Course Tools (with Bookmarks) on the Home tab, but not the Course tab.

Additionally, I see Updates on the Home tab, but I don't see an Updates link anywhere. Is that as it should be?

robrap

Code looked ok, but I may have missed something, because I still have questions about the sandbox before we merge. See other comment. Thanks.

robrap · 2017-07-09T02:29:16Z

+        Create a test user of the specified type.
+
+        Valid values for user type:
+          anonymous - an anonymous user


Note: No change required, but you could use an Enum to use code rather than comments to define this.

Enum is only supported in Python 3, as best I can tell:

https://stackoverflow.com/questions/36932/how-can-i-represent-an-enum-in-python

I did a search through the codebase and couldn't find any enums. Do you have an idiom in mind?

It occurred to me that maybe six supports enums, but it doesn't:

https://pythonhosted.org/six/search.html?q=enum&check_keywords=yes&area=default

andy-armstrong · 2017-07-09T16:05:47Z

@robrap I recreated the sandbox but forgot to re-enable all the Waffle flags. I've enabled them now, so you can test it with this URL:

https://andy-armstrong.sandbox.edx.org/courses/course-v1%3AedX%2BTest101%2Bcourse/course/

I thought we had the Course Tools, like Course Handouts, appearing on both the Home and Course tabs when not unified. On your sample course, I only see Course Tools (with Bookmarks) on the Home tab, but not the Course tab.

The reason why Course Tools wasn't showing was that there were no enabled tools. Both Bookmarks and Updates now require the user to be enrolled. I've set the Course Reviews flag which is always enabled, so now Course Tools shows up too. Interestingly, we didn't update the course info tab to use the course tools, so those links are hard-coded and don't appear dynamically. I'm going to update the template so that the logic is shared.

Additionally, I see Updates on the Home tab, but I don't see an Updates link anywhere. Is that as it should be?

The logic currently hides the "Updates" tab for non-unified courses, because the thinking is that we don't need the Course Updates page when the Home page acts as one.

andy-armstrong · 2017-07-09T16:40:23Z

@robrap I've changed the course info page to use the same logic for Course Tools as the course home page. I've also squashed commits and kicked off a sandbox update. Let me know if there's anything else you'd like to see here.

robrap · 2017-07-10T13:44:51Z

            # TODO: (Experimental Code). See https://openedx.atlassian.net/wiki/display/RET/2.+In-course+Verification+Prompts
            'upgrade_link': check_and_get_upgrade_link(request, user, course.id),
            'upgrade_price': get_cosmetic_verified_display_price(course),
+            'course_tools': course_tools,


@andy-armstrong: This TODO/ENDTODO seems like an odd construct. Not sure if you want to get rid of it by mentioning which elements the TODO applies to in the comment (preferred), or just move the course_tools outside of this weird block?

robrap · 2017-07-10T13:55:47Z

@andy-armstrong It doesn't make sense to me that bookmarks would be hidden for unenrolled staff, since they can see the content of the course and add bookmarks.

robrap · 2017-07-10T16:07:02Z

@andy-armstrong Let me know when the sandbox is ready for review with:

Bookmarks appearing for unenrolled staff.
Course Handouts title being dropped if it is empty (or let me know if you want to move that to a separate ticket).

Also, let me know your thoughts about the following as written on HipChat:

Also, I found another inconsistency with old and new, but maybe we covered this already.
In the old view (course Info), the "Course Handouts" header was visible in the unauthorized view, with no purpose because the handouts were not visible. An authenticated user that was not enrolled could see the handouts. In the new page, we make handouts available even in the unauthenticated view. I imagine this is fine, but just wanted to make sure.

andy-armstrong · 2017-07-10T19:30:40Z

Reviewers: I moved this code to helpers.py as otherwise there are circular imports when accessing this package.

andy-armstrong · 2017-07-10T19:32:10Z

@robrap I've pushed up a new commit to address your latest feedback:

the "Bookmarks" link is now shown to unenrolled staff
course handouts is no longer shown when there are no handouts
I've imported an enum library and created a CourseUserType enum
I've added a new unit test file for course bookmarks that verifies just the link behavior

I've also updated the sandbox.

Regarding course handouts being accessible to anonymous users, I don't have a strong preference. @marcotuts or @lizcohen, what do you recommend?

robrap

Looks good. Thanks @andy-armstrong. I'm approving, but of course there is still the conflict and failing tests, so let me know if you need me to look again later.
We'll wait to hear about anonymous handout access.

robrap · 2017-07-10T20:40:39Z

Oh yeah. And nice enum. :)

LEARNER-1696

andy-armstrong · 2017-07-10T23:04:38Z

jenkins run bokchoy

edx-pipeline-bot · 2017-07-11T15:56:21Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Wednesday, July 12, 2017.

edx-pipeline-bot · 2017-07-12T18:43:29Z

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

pomegranited · 2018-10-03T02:50:26Z

Hi @robrap @marcotuts CC @mduboseedx
We've got an issue with one part of this change: the course handouts are available to anonymous users, and one of our clients has high-value handouts they don't want shared.

Would you accept a PR that shows the handouts with only enrolled/staff users instead? Would that need to be gated by a waffle flag or anything? Also noted that there's some distinction between "guest handouts" and "handouts", being made, though I couldn't find know how guest_handouts are designated. But our PR could show guest_handouts to anonymous users, and handouts to everyone else, if that's better.

andy-armstrong force-pushed the andya/anonymous-course-access branch from 9880f3b to 1bae32e Compare July 4, 2017 20:54

andy-armstrong requested a review from robrap July 4, 2017 21:04

andy-armstrong force-pushed the andya/anonymous-course-access branch 4 times, most recently from ed9c3a6 to fc48dfe Compare July 5, 2017 02:36

robrap requested changes Jul 5, 2017

View reviewed changes

andy-armstrong force-pushed the andya/anonymous-course-access branch from fc48dfe to 41bf726 Compare July 6, 2017 03:33

andy-armstrong force-pushed the andya/anonymous-course-access branch 2 times, most recently from 56170d5 to 7185f4a Compare July 7, 2017 19:24

robrap reviewed Jul 7, 2017

View reviewed changes

andy-armstrong force-pushed the andya/anonymous-course-access branch from 7185f4a to 3452672 Compare July 8, 2017 11:37

robrap reviewed Jul 9, 2017

View reviewed changes

andy-armstrong force-pushed the andya/anonymous-course-access branch from 3452672 to 7459372 Compare July 9, 2017 16:39

robrap reviewed Jul 10, 2017

View reviewed changes

andy-armstrong commented Jul 10, 2017

View reviewed changes

robrap approved these changes Jul 10, 2017

View reviewed changes

Handle anonymous and unenrolled users on the new course home page

a815003

LEARNER-1696

andy-armstrong force-pushed the andya/anonymous-course-access branch from 0268f0a to a815003 Compare July 10, 2017 21:52

andy-armstrong merged commit 447bee2 into master Jul 10, 2017

andy-armstrong deleted the andya/anonymous-course-access branch July 18, 2017 03:59



		class TestCourseHomePage(SharedModuleStoreTestCase):
		class CourseHomePageTestCase(SharedModuleStoreTestCase):

Uh oh!

Conversation

andy-armstrong commented Jul 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Sandbox

Testing

Reviewers

Post-review

Uh oh!

robrap commented Jul 5, 2017

Uh oh!

andy-armstrong commented Jul 5, 2017

Uh oh!

andy-armstrong commented Jul 5, 2017

Uh oh!

marcotuts commented Jul 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robrap 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!

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!

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!

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!

robrap commented Jul 5, 2017

Uh oh!

robrap commented Jul 5, 2017

Uh oh!

andy-armstrong commented Jul 6, 2017

Uh oh!

andy-armstrong commented Jul 3, 2017 •

edited

Loading

marcotuts commented Jul 5, 2017 •

edited

Loading