Add existing sidebar information to the course outline page by dianakhuang · Pull Request #14828 · openedx/openedx-platform

dianakhuang · 2017-04-05T16:17:19Z

dianakhuang.sandbox.edx.org - outline is enabled for all users

andy-armstrong

@dianakhuang @bjacobel this is looking great. I would like us to clean up the terminology in both the back end and front end, and then I think this looks good to go from my perspective.

@bjacobel A very minor UX nit is that the Resume button goes too low when resized to a very narrow width:

andy-armstrong · 2017-04-11T14:44:22Z

    """
    # create ordered list of transformers, adding BlocksAPITransformer at end.
    transformers = BlockStructureTransformers()
+    can_view_special_exam = False


Nit: this isn't an access control check, so can_view seems confusing. How about include_special_exam?

andy-armstrong · 2017-04-11T14:45:29Z

+
+class MilestonesTransformer(BlockStructureTransformer):
    """
    Excludes all special exams (timed, proctored, practice proctored) from the student view.


Is this docstring still correct?

andy-armstrong · 2017-04-11T14:47:08Z

+                        unicode(block_key.course_key),
+                        unicode(block_key)
+                    )
+                except ProctoredExamNotFoundException as ex:


It seems odd to only handle proctored exam failures. Does this class apply to all special exams (and hence needs renaming), or is there a more generic class you could use here?

This does apply to all special exams, but it is defined and used in the edx-proctoring repo, and I don't believe this story should warrant an edx-proctoring release.

andy-armstrong · 2017-04-11T14:47:48Z

+            if self.is_special_exam(block_key, block_structure):
+
+                #
+                # call into edx_proctoring subsystem


Nit: I'd prefer that this comment not refer to proctoring since this is intended to apply to all special exams. It is a strange implementation detail that it is implemented in edx-proctoring that I hope one day will be cleaned up.

I left this in place just because it's referring to edx_proctoring the package/repo.

andy-armstrong · 2017-04-11T14:48:17Z

+                    log.exception(ex)
+
+                if timed_exam_attempt_context:
+                    # yes, user has proctoring context about


Nit: again I would generalize this to special exams and not call out proctoring specifically.

andy-armstrong · 2017-04-11T14:55:29Z

        self.setup_gated_section(self.blocks[gated_block_ref], self.blocks[gating_block_ref])

-        with self.assertNumQueries(6):
+        with self.assertNumQueries(8):


How do we feel about having added two queries to this? Is there any way to make these changes without introducing extra overhead? @nasthagiri can you advise us on this?

@dianakhuang do you know what these two additional queries are?

I believe they're the required content and user_can_skip_entrance_exam queries.

andy-armstrong · 2017-04-11T14:56:08Z

        self.setup_gated_section(self.blocks['H'], self.blocks['A'])
        self.get_blocks_and_check_against_expected(self.staff, expected_blocks)

+    def test_can_view_special(self):


Nit: I'd rename this to test_special_exams.

andy-armstrong · 2017-04-11T14:58:32Z

+                                        ## There are behavior differences between rendering of subsections which have
+                                        ## special_exam/timed examinations and those that do not.
+                                        ##
+                                        ## Proctoring exposes a exam status message field as well as a status icon


Nit: an exam

andy-armstrong · 2017-04-11T15:01:53Z

+                                    <span class="subsection-title">${ subsection['display_name'] }</span>
+
+                                    <div class="details">
+                                        ## There are behavior differences between rendering of subsections which have


It would be good to not mention proctoring in the comments in this file, because there are no special proctoring-related features. I would prefer that this only use one term, but I'm not sure if that is special exams or timed exams. The latter seems better to me, but I don't think I understand the distinction. Maybe it is possible to just call these exams, or is there different behavior for 'non-special' exams?

A lot of this confusion is also in the block API response, so it would be good for us to all get on the same page as to what we're handling here.

Cleaned this up a bit! 968a870

andy-armstrong · 2017-04-11T15:09:10Z

    # 'student_view_multi_device'
    SupportedFieldType(StudentViewTransformer.STUDENT_VIEW_MULTI_DEVICE, StudentViewTransformer),

+    SupportedFieldType('special_exam', MilestonesTransformer),


Can we just call this exam_info and have all exam information in one place? The distinction between regular exams, special exams, timed exams, and proctored exams is not obvious to me (and I imagine to others). It would be good to keep this API as simple as possible so that clients don't have to understand these distinctions either. I understand if this isn't possible due to the complicated way special exams have been implemented.

If we do want to keep this only to special exams, I'd still rename it to special_exam_info.

bjacobel · 2017-04-11T17:02:38Z

@andy-armstrong I'll fix the alignment issue in the branch for minor UI fixes, I'm working on it over there.

dianakhuang · 2017-04-11T19:03:41Z

jenkins run a11y

robrap

Just adding a couple of thoughts. I like @andy-armstrong's questions about the different types of exams. Anything we can do to understand the differences, and document or clean up code to clarify those differences would be great.
Also, is the plan to get Nimisha to review this after team review is complete? (At least the Transformer part?)

robrap · 2017-04-11T19:33:36Z

+            """
+            if self.is_special_exam(block_key, block_structure):
+
+                #


Nit: formatting on this multi-line comment seems odd. The blank lines above and below seem unnecessary, and the line length seems shorter than necessary.

robrap · 2017-04-11T19:34:30Z

+                    log.exception(ex)
+
+                if timed_exam_attempt_context:
+                    # yes, user has proctoring context about


Nit: Again, line length on this comment seems shorter than necessary.

robrap · 2017-04-11T19:41:05Z

-            return [block_structure.create_universal_filter()]
+        root_key = block_structure.root_block_usage_key
+        course_key = root_key.course_key
+        user_can_skip = EntranceExamConfiguration.user_can_skip_entrance_exam(usage_info.user, course_key)


@andy-armstrong I'm wondering 2 related things:

As I think @dianakhuang had intended, should some of this be in a separate EntranceExamTransformer?

If the transformer could be supplied from elsewhere, would that solve your issue of it not living in the api?
I'm not sure how much work it would be to do Fix js that prevented single step OE problems (ie only self assessment) ... #2, but possible doing Put back in the blocks for Advanced Settings. #1 would better enable that?

robrap · 2017-04-11T20:13:44Z

@dianakhuang: The sandbox has various issues, and it is unclear whether or not that they are relevant to our work.

"Week 1 > Problems!" has various CAPA errors.
There is a section titled "Section" toward the bottom with no subsections and no messaging.
The final subsection "Gated Subsection > Fake Midterm you can't complete without doing the first Week Subsection" leads to a page with no units, broken breadcrumb, and no messaging.

I recommend you configure certain users with the new view and certain with the old view, so we can compare against the old courseware and left-hand navigation.

FYI: @marcotuts

bjacobel · 2017-04-11T20:18:23Z

@robrap I just reconfigured the sandbox to have the feature on for staff@ but off for non-staff users.

robrap · 2017-04-11T21:01:11Z

Thanks @bjacobel. Seems the same issues are happening in the old view. I'm hoping/assuming we have not broken the old view, but it would be nice to have a valid enough course to test, including the proper gating. This would include some gating and an entrance exam.

dianakhuang · 2017-04-12T16:59:39Z

jenkins run js

dianakhuang · 2017-04-12T17:08:48Z

jenkins run bokchoy

dianakhuang · 2017-04-12T17:13:53Z

@andy-armstrong i believe i've addressed all your comments.

andy-armstrong · 2017-04-12T17:18:48Z

Should this be special_exam_info?

andy-armstrong · 2017-04-12T17:21:17Z

Nit: I was thinking of this as a separate top-level method so that it reduces the complexity of the transform method. It also makes it easier to move the method to a different file.

andy-armstrong

👍 two nits from me, so address them or not as you see fit.

@nasthagiri I think this is so close to landing that we won't ask you for a formal review. If you are able to look at it and see issues, we can address them in a follow-on PR if this has already merged. Thanks!

dianakhuang · 2017-04-12T17:39:59Z

@robrap i would definitely love a review once you get a chance to do so.

bjacobel · 2017-04-12T18:24:12Z

jenkins run js

robrap · 2017-04-12T18:38:19Z

+            if user_gated_from_block(block_key):
+                block_structure.remove_block(block_key, False)
+            else:
+                self.add_special_exam_info(block_key, block_structure, usage_info)


@dianakhuang @nasthagiri: My very imperfect understanding was that the collect phase added data and the transform phase mostly pruned. However, it is called "transform" and not "prune". Can one of you help me understand what should happen where, and why and if add_special_exam_info makes sense during transform? I admit I did not dig into code, I am just going by names and previous understanding.
: Is this the right place to add this info based on your intentions of collect vs transform?

Talked to @robrap in person, but I think we are in agreement that while currently, the transform phase only prunes right now, it is set up such that this is a reasonable use case because the special_exam_info field contains user-specific data that is pulled from the RDS.

The transform phase is not meant for only pruning. If you see the implementations of the transform methods in the course_api/blocks/transformers directory, you'll find that many of them don't prune, but instead add necessary data for the requester.

robrap · 2017-04-12T21:46:17Z

@@ -35,22 +45,39 @@ def collect(cls, block_structure):
        block_structure.request_xblock_fields('is_proctored_enabled')


@nasthagiri: @dianakhuang and I agree that it would seem cleaner to have special exams (which aren't milestones), to be in its own transformer. @dianakhuang is under the impression that you don't think that is a good idea. Could you clarify why? Thank you!

nasthagiri · 2017-04-13T13:56:45Z

    # create ordered list of transformers, adding BlocksAPITransformer at end.
    transformers = BlockStructureTransformers()
+    include_special_exams = False
+    if requested_fields is not None and 'special_exam_info' in requested_fields:


Nit: The "info" part is redundant; the name can be simplified to just "special_exams".

nasthagiri · 2017-04-13T13:59:10Z

        block_structure.request_xblock_fields('is_proctored_enabled')
        block_structure.request_xblock_fields('is_practice_exam')
        block_structure.request_xblock_fields('is_timed_exam')
+        block_structure.request_xblock_fields('entrance_exam_id')


Since you are adding a collected field here, we'll need to coordinate the release of this transformer and update its version number accordingly. Please see the docstring at https://github.com/edx/edx-platform/blob/master/openedx/core/djangoapps/content/block_structure/transformer.py#L17.

nasthagiri · 2017-04-13T14:02:41Z

+                return False
+            elif self.has_pending_milestones_for_user(block_key, usage_info):
+                return True
+            elif self.gated_by_required_content(block_key, block_structure, user_can_skip, required_content):


Why is this call needed? Isn't this already covered by has_pending_milestones_for_user?

Unfortunately, no. I'm not sure exactly where the behavior is not not overlapping, but tests started to fail once one of these was used but not the other.

nasthagiri · 2017-04-13T14:05:44Z

+            if user_gated_from_block(block_key):
+                block_structure.remove_block(block_key, False)
+            else:
+                self.add_special_exam_info(block_key, block_structure, usage_info)


The transform phase is not meant for only pruning. If you see the implementations of the transform methods in the course_api/blocks/transformers directory, you'll find that many of them don't prune, but instead add necessary data for the requester.

nasthagiri · 2017-04-13T14:07:31Z

+            except ProctoredExamNotFoundException as ex:
+                log.exception(ex)
+
+            if special_exam_attempt_context:


Nit: For readability and simpler code, put this in an else clause of the try-except. And you can also eliminate the special_exam_attempt_context boolean,

The special_exam_attempt_context isn't a boolean. It's a dictionary with the special exam data contained within it. If there isn't any for this user for this exam, I believe it returns an empty dictionary, so this additional check is necessary.

robrap · 2017-04-13T13:54:43Z

+                return False
+            elif self.has_pending_milestones_for_user(block_key, usage_info):
+                return True
+            elif self.gated_by_required_content(block_key, block_structure, user_can_skip, required_content):


@dianakhuang This part around entrance exams is new and seems to affect the api overall, so will it affect mobile as well? What would have happened, what is happening, and is this change ok without another special parameter?

As discussed in person, this is probably a miss on the original mobile for entrance exams. So, hopefully it is a good thing to have this in both cases.

robrap · 2017-04-13T15:20:35Z

+                                                else:
+                                                    data_string = _("{subsection_format} due {{date}}").format(subsection_format=subsection.get('format'))
+                                        %>
+                                        % if subsection.get('format') or 'special_exam_info' in subsection:


@bjacobel Do we need an empty <div class="details"> if there are no details, or is it ok to use this if to gate the entire div?

Includes refactor of course home/course outline smoke test to its own file.

Add comment explaining why we can't have a separate transformer.

edx-pipeline-bot · 2017-04-14T15:59:51Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Monday, April 17, 2017.

edx-pipeline-bot · 2017-04-18T14:45:10Z

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

dianakhuang force-pushed the diana/new-transformer branch from 3d2f9f8 to f98c34e Compare April 6, 2017 15:47

robrap force-pushed the diana/new-transformer branch from 54b8cb3 to b08e6b9 Compare April 7, 2017 13:55

dianakhuang force-pushed the diana/new-transformer branch from 6bba802 to c72a613 Compare April 7, 2017 17:05

bjacobel requested review from andy-armstrong and robrap and removed request for robrap April 11, 2017 14:26

bjacobel changed the title ~~WIP: add new data to milestones transformer~~ Add new data to milestones transformer and icon/date UI to outline Apr 11, 2017

andy-armstrong changed the title ~~Add new data to milestones transformer and icon/date UI to outline~~ Add existing sidebar information to the course outline page Apr 11, 2017

andy-armstrong suggested changes Apr 11, 2017

View reviewed changes

bjacobel mentioned this pull request Apr 11, 2017

Implement minor UI/A11Y fixes for outline #14859

Merged

11 tasks

robrap reviewed Apr 11, 2017

View reviewed changes

andy-armstrong reviewed Apr 12, 2017

View reviewed changes

Comment thread lms/djangoapps/course_api/blocks/api.py Outdated

andy-armstrong Apr 12, 2017

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

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

Should this be special_exam_info?

andy-armstrong reviewed Apr 12, 2017

View reviewed changes

andy-armstrong approved these changes Apr 12, 2017

View reviewed changes

dianakhuang force-pushed the diana/new-transformer branch from 54985eb to 76d68c1 Compare April 12, 2017 17:52

robrap reviewed Apr 12, 2017

View reviewed changes

dianakhuang force-pushed the diana/new-transformer branch from 811de72 to fbb08d4 Compare April 13, 2017 13:29

nasthagiri reviewed Apr 13, 2017

View reviewed changes

robrap reviewed Apr 13, 2017

View reviewed changes

dianakhuang force-pushed the diana/new-transformer branch 2 times, most recently from c5beaea to 5959e1c Compare April 13, 2017 17:08

dianakhuang and others added 10 commits April 13, 2017 14:38

Initial version of new transformer.

9e73444

Fix preview change from 'Student' to 'Learner'.

83dd33a

Add new info to the course outline

b7e1a0d

Add graded homework to a11y test of course outline.

584e7fd

Includes refactor of course home/course outline smoke test to its own file.

Fix failures with simplified subsection title selector.

cca52b9

Begin client-side date processing using DateutilFactory

31aa776

Add 'format' as a requested field.

aafd6a0

Clean up docs re: exam formats in outline html

69ba8eb

Fix review comments.

0899ac5

Rename MilestonesTransformer.

196d379

Add comment explaining why we can't have a separate transformer.

dianakhuang force-pushed the diana/new-transformer branch from 8e6c118 to 196d379 Compare April 13, 2017 18:39

dianakhuang merged commit c4f0a89 into master Apr 13, 2017

robrap deleted the diana/new-transformer branch October 11, 2017 18:13

		@@ -35,22 +45,39 @@ def collect(cls, block_structure):
		block_structure.request_xblock_fields('is_proctored_enabled')

Uh oh!

Conversation

dianakhuang commented Apr 5, 2017 • edited by bjacobel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andy-armstrong 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!

bjacobel Apr 11, 2017 • 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!

bjacobel commented Apr 11, 2017

Uh oh!

dianakhuang commented Apr 11, 2017

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!

robrap commented Apr 11, 2017

Uh oh!

bjacobel commented Apr 11, 2017

Uh oh!

robrap commented Apr 11, 2017

Uh oh!

dianakhuang commented Apr 12, 2017

Uh oh!

dianakhuang commented Apr 12, 2017

Uh oh!

dianakhuang commented Apr 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-armstrong left a comment

Choose a reason for hiding this comment

Uh oh!

dianakhuang commented Apr 12, 2017

Uh oh!

bjacobel commented Apr 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dianakhuang commented Apr 5, 2017 •

edited by bjacobel

Loading

bjacobel Apr 11, 2017 •

edited

Loading