Refactor MentoringBlock to use StudioContainerWithNestedXBlocksMixin #121

haikuginger · 2016-09-16T16:58:13Z

Since we initially created ProblemBuilder, the StudioContainerWithNestedXBlocksMixin class has been added to the XBlock runtime. Since this is now the standardized way of dealing with nested XBlocks, we want to move MentoringBlock to use that structure and gain any existing benefits, as well as any benefits for the future.

Dependencies: None

Screenshots:

Sandbox URL:

Partner information: Hosted on edx.org (Davidson College)

Testing instructions:

In either the sandbox or a devstack, verify that you are able to create a mentoring block.
Verify that subelements of the mentoring block can be created as expected.
Verify that you are able to drag and drop subelements to reorder them (not possible previously)

Author notes and concerns:

To prevent a circular import, we had to revert a performance enhancement change that imported MentoringBlock directly into the Questionnaire module. Because the QuestionnaireBlock is now used directly by the MentoringBlock, this resulted in a loop. Suggestions are welcome for a way we can keep the performance enhancement.

Reviewers

@mtyaka
edX reviewer[s] TBD

mtyaka

This is looking pretty good! I tested it and it looks and works great in the Studio, but it seems that we are missing buttons to add "Message"-type children.

You will also need to rebase and resolve a couple of conflicts.

mtyaka · 2016-09-29T07:55:06Z

problem_builder/mentoring.py

-        fragment.add_content(loader.render_template('templates/html/mentoring_url_name.html', {
-            "url_name": self.url_name
-        }))
+        local_context = dict(context)


Nit: I prefer explicit context.copy() over dict(context) (although I see you copied this from step.py, so if you decide to change it here, it would make sense to change it in step.py, too).

mtyaka · 2016-09-29T08:09:55Z

problem_builder/mentoring.py

-        self.render_children(context, fragment, can_reorder=True, can_add=False)
-        fragment.add_content(u'</div>')
-        fragment.add_content(loader.render_template('templates/html/mentoring_add_buttons.html', {}))
-        fragment.add_content(loader.render_template('templates/html/mentoring_url_name.html', {


Nit: I'm not sure how important this is, but I think we can put the URL name back in to make it consistent with Step Builder block (see screenshot).

mtyaka · 2016-09-29T08:10:46Z

problem_builder/mentoring.py

-        fragment = Fragment(u'<div class="mentoring">')  # This DIV is needed for CSS to apply to the previews
-        self.render_children(context, fragment, can_reorder=True, can_add=False)
-        fragment.add_content(u'</div>')
-        fragment.add_content(loader.render_template('templates/html/mentoring_add_buttons.html', {}))


We should remove the mentoring_add_buttons.html template if we're not using it anymore.

Now that were' using StudioContainerWithNestedXBlocksMixin, do we still need the code in this file: https://github.com/open-craft/problem-builder/blob/4ffa21c36b534f9b86f50b128815800a33b69899/problem_builder/public/js/mentoring_edit.js or is that handled by the mixin now? It might be worth checking and remove that file if it's not needed anymore.

It looks like we're still using it in the questionnaire block; I haven't taken a long look to determine what it's being used for, though.

mtyaka · 2016-09-29T08:17:48Z

problem_builder/mentoring.py

+        return [
+            NestedXBlockSpec(AnswerBlock, boilerplate='studio_default'),
+            MCQBlock, RatingBlock, MRQBlock,
+            NestedXBlockSpec(None, category="html", label=self._("HTML")),


It looks like there are three types of children missing from this list: "Message (Complete)", "Message (Incomplete)", and "Message (Max # Attempts)" (see screenshot). It's not visible in the screenshot, but I think there is also "Message (Assessment Review)" when using the deprecated assessment mode.

mtyaka · 2016-09-29T08:20:41Z

problem_builder/questionnaire.py

-        # of questionnaire.[css/js] into the DOM. So we use the mentoring block here if possible
-        block_with_resources = self.get_parent()
-        if not isinstance(block_with_resources, MentoringBlock):
-            block_with_resources = self


We normally work around circular dependencies by deferring imports until they are needed, for example:

problem-builder/problem_builder/mentoring.py

Line 418 in 4ffa21c

from .questionnaire import QuestionnaireAbstractBlock # Import here to avoid circular dependency

Would that work here? If it works, it would be great if we could apply the same optimization to Step Builder.

haikuginger · 2016-10-02T22:33:12Z

@mtyaka, I'm updating the sandbox now - care to take another look?

mtyaka

I found some issues related to the MentoringMessageBlock component.

Besides the issues outlined in inline comments, I also noticed that when you add a message type block, it renders the student view initially, and only renders the author view if you refresh the page in the Studio.
It looks like in order to get it to render author_view immediately, you have to set the has_author_view flag to True.

mtyaka · 2016-10-03T06:24:27Z

problem_builder/questionnaire.py

+        # If we use local_resource_url(self, ...) the runtime may insert many identical copies
+        # of questionnaire.[css/js] into the DOM. So we use the mentoring block here if possible
+        block_with_resources = self.get_parent()
+        from .mentoring import MentoringBlock


Nit: Can you add a comment to explain we're importing the class here to work around a circular dependency?

mtyaka · 2016-10-03T06:28:09Z

problem_builder/message.py

+
+
+def get_message_label(type):
+    return MESSAGE_TYPES.get(type, 'Message')


Do we want to default to 'Message' here? I think I would prefer to let it fail if the label is missing, since we expect the label to always be present.

mtyaka · 2016-10-03T07:19:04Z

problem_builder/message.py

+    'incomplete': _(u'Message (Incomplete)'),
+    'max_attempts_reached': _(u'Message (Max # Attempts)'),
+    'on-assessment-review': _(u'Message (Assessment Review)'),
+}


Since there's already a MESSAGE_TYPES dict with specific message type data defined inside the MentoringMessageBlock class, would it make sense to move these studio labels into that dict, to keep message type data in a single place?

In the get_message_label function, you would then do something like:

return MentoringMessageBlock.MESSAGE_TYPES[type]['studio_label']

mtyaka · 2016-10-03T07:22:11Z

problem_builder/mentoring.py

        fragment.add_css_url(self.runtime.local_resource_url(self, 'public/css/problem-builder-tinymce-content.css'))
        fragment.add_javascript_url(self.runtime.local_resource_url(self, 'public/js/util.js'))
-        fragment.add_javascript_url(self.runtime.local_resource_url(self, 'public/js/mentoring_edit.js'))
        fragment.initialize_js('MentoringEditComponents')


The MentoringEditComponents function is defined inside mentoring_edit.js, which we removed, so we should also remove this line.

Sorry for not catching this before, but we have to import and initialize the container_edit.js file to properly utilize nested xblock JS helpers from the xblock-utils repository, just like we do for the MentoringWithExplicitStepsBlock.

mtyaka · 2016-10-03T07:33:04Z

problem_builder/mentoring.py

+                'completed',
+                'incomplete',
+                'max_attempts_reached',
+                'on-assessment-review',


As of #122, The on-assessment-review message type button should only be displayed when using the deprecated "assessment" mode, so we should only add on-assessment-review to this list when the self.is_assessment flag is True.

mtyaka · 2016-10-03T08:19:36Z

problem_builder/mentoring.py

+                MentoringMessageBlock,
+                category='pb-message',
+                boilerplate=message_type,
+                label=get_message_label(message_type),


The Message block specs should have single_instance=True, because we only allow a single instance of each message type inside a Mentoring block, BUT I tried that and it looks like single_instance functionality is currently implemented in a rather hackish way and it can only limit blocks by category. Here we still want to allow multiple pb-message blocks, as long as they are of different message_type, but it doesn't look like we can do that currently.

If you set single_instance=True, the button that you click gets correctly disabled when adding a Message type block to a mentoring container, but if you refresh the page, all pb-message buttons are disabled rather than just the one type that you added to the container.

One way to fix this would be to introduce subclasses of MentoringMessageBlock, so that each message type would be a separate class.
Another way would be to expand the single_instance feature of nested xblocks in xblock-utils so that it would be able to toggle the button on parameters other than just the block category.

I think I prefer the second option, but neither of these options are simple, so in order to not block this PR, I think I am fine with leaving single_instance=True out for now, and create a task to handle this in next sprint.

@mtyaka Given the larger work that both options would represent, which we don't have time to address in the scope here, could we try to find a third option?

What would happen if two messages of the same type were added? Would there be an explicit validation error appearing for the bloc? If so, would it make sense to simply let the validation error inform the author that he needs to fix it - we can't prevent every editing issue proactively anyway, so this is already a pattern the author should be familiar with. And improve this case when Studio and xblock-utils support the use case?

@antoviaque If two messages of the same type are added, there is an explicit error message on the parent (mentoring) block, although it's only visible after you update the page. It's not a big problem, so I think it would be fine to leave it for when Studio/xblock-utils support the use case.

Okay, sounds like I should just go ahead and merge? @antoviaque, @mtyaka?

haikuginger · 2016-10-03T09:51:38Z

@mtyaka, those changes have been made.

mtyaka · 2016-10-03T10:05:31Z

Thanks @haikuginger! Once the build is green:

👍

I tested this: Adding child items to the mentoring block container works correctly after refactoring.
I read through the code
~~Includes documentation~~ (this is internal refactoring, no documentation needed)

haikuginger changed the title ~~[WIP] Refactor MentoringBlock to use StudioContainerWithNestedXBlocksMixin~~ Refactor MentoringBlock to use StudioContainerWithNestedXBlocksMixin Sep 28, 2016

mtyaka requested changes Sep 29, 2016

View reviewed changes

Jesse Shapiro added 5 commits October 2, 2016 18:31

Initial work

c51c6eb

Avoiding circular import

6385bee

Pointing to mentoring edit JS

14da31c

Remove unnecessary import

702f265

Review comments

d1fae47

haikuginger force-pushed the haikuginger/nested-xblock-refactor branch from 9f83f22 to d1fae47 Compare October 2, 2016 22:32

mtyaka requested changes Oct 3, 2016

View reviewed changes

Review comments

eff467d

mtyaka approved these changes Oct 3, 2016

View reviewed changes

Remove trailing whitespace

97749d8

itsjeyd mentioned this pull request Oct 4, 2016

Problem Builder - Review for release on edx.org #119

Merged

3 tasks

haikuginger merged commit cbc895f into master Oct 4, 2016

haikuginger deleted the haikuginger/nested-xblock-refactor branch October 10, 2016 23:12



		def get_message_label(type):
		return MESSAGE_TYPES.get(type, 'Message')

Refactor MentoringBlock to use StudioContainerWithNestedXBlocksMixin #121

Refactor MentoringBlock to use StudioContainerWithNestedXBlocksMixin #121

Uh oh!

Conversation

haikuginger commented Sep 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtyaka left a comment

Choose a reason for hiding this comment

Uh oh!

mtyaka Sep 29, 2016 • 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!

mtyaka Sep 29, 2016 • 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!

mtyaka Sep 29, 2016 • 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!

haikuginger commented Oct 2, 2016

Uh oh!

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

mtyaka Oct 3, 2016 • 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!

haikuginger commented Oct 3, 2016

Uh oh!

mtyaka commented Oct 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

haikuginger commented Sep 16, 2016 •

edited

Loading

mtyaka Sep 29, 2016 •

edited

Loading

mtyaka Sep 29, 2016 •

edited

Loading

mtyaka Sep 29, 2016 •

edited

Loading

mtyaka Oct 3, 2016 •

edited

Loading

mtyaka commented Oct 3, 2016 •

edited

Loading