Add ui in studio for conditional_module.

[oksana-slu]

Background: Usage and modification of Conditional Module can be undertaken only with course import/export mechanism. Now Creation and modification of Conditional Mode are enabled within Studio functionality.

Studio Updates: The next file is modified: common/lib/xmodule/xmodule/conditional_module.py . For new functionality, the next tests are corrected: common/lib/xmodule/xmodule/tests/test_conditional.py; the new tests are added.

LMS Updates: None

Testing:

[openedx-webhooks]

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

If you like, you can add yourself to the AUTHORS file for this repo, though that isn't required. Please see the CONTRIBUTING file for more information.

[AlesyaD]

@jbarciauskas

[ormsbee]

Thanks for the PR! I just took a quick glance at it, and the approach looks good to me (simple, and doesn't impact our longer term plans to make this an XBlock). I'm not really familiar with this module, but my only concern would be around data migration -- testing that old courses still import correctly with the new fields, and that there aren't odd ways to create legitimate conditional blocks that would break your UI, etc.

[nedbat]

@scottrish product review?

[oksana-slu]

@justinabrahms @ormsbee
We have fixed a tests, so all tests finished successfully . Also, we tested this features with the old courses ( Export / import). Old courses still import correctly with the new fields

[jbarciauskas]

@oksana-slu This will need UX review as well as regular engineering review. I've created a second ticket in JIRA, you can find it there.

[oksana-slu]

@jbarciauskas
we have sandbox with this feature, please follow the link to make UX review
LMS: http://178.62.209.246/
CMS: http://178.62.209.246:18010/

[marcotuts]

Hi! Thank you for the sandbox setup here and the details, I'm excited for what this will enable for our course teams in Studio! I would love to set up some time to review this in more detail to make sure I understand all the configuration options and the workflow for authors. Would it be possible to find some time to meet virtually? If it helps with scheduling we can use calendly.com/marcotuts

Thanks!
Marco

[oksana-slu]

@marcotuts
Hi, we are glad that you find this feature useful for your course team. Of course, we can meet virtually.
Olesya booked 21 April at 4:15PM via calendly.com/marcotuts. Or we can chat anytime.

[jbarciauskas]

@marcotuts did you get together to review this?

[marcotuts]

@jbarciauskas We did meet, and THIS is the PR that I owe a product review / response. I will get to that before Monday for sure, apologies for the delay on this @oksana-slu!

Thanks

[nedbat]

@marcotuts ping

[nedbat]

@marcotuts we need to do something with this PR. If we aren't going to be able to respond, we should say that.

[marcotuts]

Here is a summary of the product feedback so far.

1. The use of the A/B testing / Randomized Problem Block pattern of nesting the components inside of the block is great!
2. A bit more effort is necessary on naming and wording in a number of places. I've suggested a list of areas where these changes could happen, but @catong may have other suggestions as well. I've also slightly reordered the settings as well.
3. Setting name: Source Components --> originally "sources list"
   3a. The component location(s) whose attributes are used to determine whether a student should be shown the content inside of this conditional block.
4. Setting: Conditional Attribute --> (originally "conditional_attr")
   4a. Setting subtitle - The attribute from the source component used to determine whether students should be shown the content in this conditional block.
5. Setting: Conditional Value --> (originally "conditional_value")
   5a. Setting subtitle - The value of the conditional attribute that needs to be true for a given student to see the content in this conditional block.
6. Setting name: Blocked content message --> (originally "message")
   6a. Setting subtitle - The message learners see when not all conditions are met for this block. You can use the {link} variable to give learners a direct link to the required module.
7. Warning message on component: This component has no source components configured yet.
8. This is a part of a different PR but we should ensure the "component location" ID is the last setting at the bottom of the settings tab for the components in Studio. We also want to make sure it is read only and not a (seemingly) editable field at a glance. The difference between using a disabled input field (what we have today) and something that doesn't look like an input field is subtle but seems like a useful change. Also, which components / blocks in Studio now render the location ID? Is it all blocks / xmodules / xblocks? Only some?

@oksana-slu - I'm able to occasionally access the sandbox, could that be updated again? Apologies again for the delay on this.

FYI's
@andy-armstrong
@felipemontoya - who has worked on an XBlock similar to this. (Flow control XBlock)
@chrisndodge - this conditional block has a "message" concept / area similar to the gated content message area we show for proctoring / timed exams. I was reminded about the LTI consumer xblock changes you are making and I thought you might want to take a peek at this work in case any of the ideas are helpful.

[marcotuts]

@felipemontoya I know this PR still needs to undergo a review, but I wanted to call out the Flow control XBlock work you did and see if it would be possible spark some discussions with @oksana-slu and team in the future (after this PR, more than likely) about how certain conditional block fields might be improved. As an example, the flow control XBlock's message area being a WYSIWYG area is interesting.

Similarly, the (more extensible) actions / options you have in the flow xblock might be interesting to explore further within the 'conditional' block world.

[oksana-slu]

@marcotuts
Hello,
we would like to clarify about PRs.
Paragraph 1-7 - everything is clear and we fix it asap.
In paragraph 8 you say about location attribute. But, everything what concerns location we set in PR Location (https://github.com/edx/edx-platform/pull/11999).
Should we merge them?

[marcotuts]

@oksana-slu Sorry, I can move my comment for 8 to the other PR, you are correct that it mostly concerns this other PR's effort. Thanks!

[oksana-slu]

@marcotuts
We updated the sandbox with conditional module
LMS: http://edx-sendr-dogwood.raccoongang.com/
CMS: http://studio-sendr-dogwood.raccoongang.com/

[marcotuts]

@oksana-slu - Any sense of when you will be able to get to the conditional poll bokchoy test failure? After this @andy-armstrong can continue his review.

[gsong]

@marcotuts See @oksana-slu's comment regarding getting help about that test specifically. Any idea who can help with that from our end?

[marcotuts]

@andy-armstrong - Are there any confluence pages / suggestions we could link @oksana-slu to re: common issues with: "local test success / jenkins test fail" situations. I know it came up in conversation that it might be a situation of needing some sort of wait() or time related delay? I couldn't find the correct Confluence resource myself.

edit: cc'ed @benpatterson in case he has suggestions on 'common / semi-common testing issues' documentation that might exist currently.

[andy-armstrong]

@oksana-slu @marcotuts here's a wiki with some tips for Bok Choy:

https://openedx.atlassian.net/wiki/pages/viewpage.action?pageId=39583991

[oksana-slu]

@marcotuts @andy-armstrong
I will try to fix this test next week, and inform you about progress.
Thanks

[oksana-slu]

@andy-armstrong @marcotuts
I have already fixed the test. You can continue to review the pull request.
Looking forward to your feedback.

[andy-armstrong]

@oksana-slu That's great news that you got all the tests to pass! I'll be able to re-review this on Monday. Could you check that the sandbox is up-to-date before I start. Also, did you address the label changes that @catong recommended?

[oksana-slu]

@andy-armstrong
Hello,
the sandbox is updated.
Follow please the link to test it.
And I have also mada changes that you recommended.
LMS: http://edx-ui-conditional-module.raccoongang.com
Studio: http://studio-ui-conditional-module.raccoongang.com

login: staff@example.com
password: edxPass123

[andy-armstrong]

@oksana-slu for some reason, I'm not seeing the "Conditional Location ID" on the sandbox for any components that I add to the course. That makes it hard to use the feature (although I can work around it). Does it need rebasing to pick up that feature?

[andy-armstrong]

@oksana-slu the feature works great! I have a few pieces of feedback on the UI but nothing that cannot be treated as follow-on stories just so you can merge all your hard work here.

1. The UI is quite complicated to understand, so I think it would be good to have documentation on the right-hand side like we do for content experiments. Maybe @catong could suggest some options. Here's how content experiments looks:

You can see a live component in my test course on your sandbox:

http://studio-ui-conditional-module.raccoongang.com/container/block-v1:raccoongang+AA101+1+type@split_test+block@51f733a536cf41c6b38df1274e71b6d0

1. I think it would also be good to show the components that are influencing the content on the Conditional UI page itself. As it stands, it isn't clear precisely when this content will be shown. I would model it on how the content experiment page shows a label above the content, i.e the part that says: "This content experiment uses group configuration 'Andy's Test Experiment'".
2. This is pre-existing behavior, but it is unfortunate that you can't use a conditional on the same page as the question it is conditional on. If you do (as in my test course), the conditional content doesn't appear when the condition becomes true. Instead, you have to refresh the page to get it to show up. Maybe this is just something that needs to be documented.

I'm going to review the code now.

[andy-armstrong]

@oksana-slu thanks for addressing all of my feedback. This is a fantastic feature and looks ready to go from my standpoint. I'll merge it once we have the necessary sign off from the other reviewers.

[andy-armstrong]

@catong can you re-review when you have a chance to verify that the strings are all as you had in mind. Thanks!

[Colin-Fredericks]

It looks like it takes multiple "source" components and applies the same
logic to all of them. Is that the intent?

And yes, some documentation will definitely be necessary, if nothing else
to explain what the different options are for "Conditional Value" and what
the options for "Conditional Attribute" mean. It may be more helpful to
give that inline underneath the attribute than in the sidebar as shown for
Content Experiments, since getting back to that sidebar requires cancelling
out of what you're doing.

Very glad to have this functionality moving into Studio.

On Mon, Oct 3, 2016 at 12:15 PM, Andy Armstrong notifications@github.com
wrote:

@catong https://github.com/catong can you re-review when you have a
chance to verify that the strings are all as you had in mind. Thanks!

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/edx/edx-platform/pull/11710#issuecomment-251150948,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AB1xJG8BbkMoKnagJ4Yc2ltEH1uyEp9Bks5qwSongaJpZM4HmcpG
.

[andy-armstrong]

@Colin-Fredericks Thanks for the feedback, Colin. I agree that we need a variety of documentation to help folks with this feature. Note that there is some help text beneath each attribute in the settings modal:

I'd still like to see documentation on the page detailing what to do, because it starts out as a blank page with a single edit button.

We also need online documentation if authors are going to be able to work out what to do.

[andy-armstrong]

@catong do you have an ETA for a review of this PR? The documentation changes you requested have all been made, so my preference would be to merge this and have a follow-up story to add some inline help text. Thanks!

[catong]

@andy-armstrong I agree that improvements to the inline help can follow, but the accompanying course author documentation is still in progress. I think it is Product's (@sstack22) call whether to get this feature out on prod without accompanying doc.

[catong]

Sorry - I guess it was @marcotuts who was involved with this PR.

[andy-armstrong]

@catong Note that the RC has been cut for next week's release, so the feature won't be live until the following week at the earliest. I also think that it is okay if the documentation isn't released at the same time, since this is an esoteric feature that must be enabled through an advanced setting, so folks won't be affected by it unless they've been following this work.

@sstack22, are you comfortable giving thumbs in @marcotuts's absence, or shall we wait for him to return? Thanks.

[catong]

@andy-armstrong Thanks for clarifying the release timing. I thought this week's RC had not yet been cut today and did want @sstack22's confirmation.

[sstack22]

@andy-armstrong - I can definitely review, but most likely will be able to on Tuesday. Let me know if this is urgent though!

[marcotuts]

cc'ed @andy-armstrong, @catong, @sstack22 - Ok to merge this in as in I think. A separate follow-on story for in-context documentation can be added to the backlog, and any announcement of this UI in studio can be gated until we have the base documentation. Thoughts on that?

(Excited to see this nearing merge!)

[andy-armstrong]

@oksana-slu @marcotuts @catong I've created a new JIRA story to add some inline documentation to this view:

https://openedx.atlassian.net/browse/TNL-5732

I'm going to go ahead and merge this PR now. Thanks for the great work here, @oksana-slu!