Add Studio support for cohorted courseware by andy-armstrong · Pull Request #6227 · openedx/openedx-platform

andy-armstrong · 2014-12-12T03:24:55Z

TNL-652

andy-armstrong · 2014-12-12T03:29:01Z

@explorerleslie @catong @talbs @cahrens @dan-f - I've completed a read-only version of the Studio component visibility modal. You can check it out on my sandbox with this project:

http://studio.andy-armstrong.m.sandbox.edx.org/container/i4x://AndyA/AA101/vertical/db3106691153424ba4551410f0db42d6

It correctly handles all the different cases (no cohort groups, staff only inheritance, missing/deleted groups etc). Next I'm going to work on implementing save which will be based upon the work we did in the hackathon.

dan-f · 2014-12-12T18:34:36Z

Just a reminder not to miss this NOTE :)

explorerleslie · 2014-12-12T19:07:57Z

@andy-armstrong @talbs great work! Couple of comments:

Right now the eyeball is always black, but @talbs's intention was to have it grey unless the component was only shown to specific groups.
It should be "Save/Cancel" buttons, not an "Ok" button, but I'm not sure if that's because it's not wired up.
I trust your word for all the different cases, but I can't see them until the backend is hooked up, so I'll comment on them in the future.

catong · 2014-12-12T19:41:03Z

@andy-armstrong @talbs @explorerleslie I'm wondering about the wording of "Set visibility to". Sorry I didn't think about this in the previous pass.
Could we make the label more clear about what the user is setting?
How about: "Make this component visible to:"?

catong · 2014-12-12T19:44:21Z

In this sandbox, should I be seeing some of the changes I think that @talbs made to the wording of the note: "The Unit this component is contained in is hidden from students..."?

andy-armstrong · 2014-12-12T20:28:34Z

Thanks @explorerleslie.

@cahrens is working on the changes to the container page so that will come shortly.
Yes, this won't show the correct buttons until I implement save.
Agreed! I had to fake the code on my local machine to test it. Once we have save we can try it.

dan-f · 2014-12-13T07:01:54Z

I ran into an issue when writing my python unit tests - depending how I instantiated UserPartition I would get an error when calling store.update_item() on the course. When I pass scheme=CohortPartitionScheme I got the error, but when I pass scheme_id = 'cohort', things worked. This is the error:

AttributeError: type object 'CohortPartitionScheme' has no attribute 'name'

Looks like UserPartition.to_json() assumes the scheme has a name field, but that field is only being set in certain circumstances. It looks like setup.py defines some type of mapping to make the latter use case work. Is this a concern?

dan-f · 2014-12-13T07:04:11Z

This was another strange thing I ran into when writing these tests. This was coming from deep in the modulestore. Perhaps @dmitchell could kindly shed some light on this? The exception had a message about mongo not wanting to accept a non-string key.

Can you reproduce this w/ me looking over your shoulder and we'll debug it together?

cahrens · 2014-12-14T17:23:01Z

I'm seeing a strange empty-scrollbar area next to the name of the groups.

cahrens · 2014-12-14T18:02:43Z

You are not using relative_url, instead hard-coding /js/xblock/authoring.js.

@andy-armstrong what is the purpose of relative_url?

Given the TODO below, I have the feeling this code was not done.

The intention was to use relative_url instead of the constant string as @cahrens pointed out. The idea was to isolate the STATIC_URL hack into one place as we start to add other dependencies.

cahrens · 2014-12-16T02:11:38Z

@andy-armstrong and @dan-f I think I'm done with the visibility state updates (text in publishing sidebar, "eyeliner" when appropriate). Bok choy tests still need to be written, but they should be trivial once the rest of the bok choy test setup is done.

I squashed my work into a single commit and force pushed.

andy-armstrong · 2014-12-16T03:59:05Z

@explorerleslie @catong @talbs @cahrens @dan-f @jimabramson - I've implemented saving of the visibility state and I've updated my sandbox:

http://studio.andy-armstrong.m.sandbox.edx.org/container/i4x://AndyA/AA101/vertical/db3106691153424ba4551410f0db42d6

My test course now demonstrates the various combinations of error states:

a single deleted group
a mix of deleted and regular groups
two deleted groups (@talbs @explorerleslie - this one has some UX challenges)

I'm going to kick off an update to pick up @cahrens's changes too.

explorerleslie · 2014-12-17T22:36:53Z

looking good to me. Is this final changes at this point? If so, 👍 . If not, ping me again before merge.

catong · 2014-12-17T23:11:38Z

I'm not crazy about mentioning "xblocks" to users, but not sure what generic term can replace it. Perhaps reword to skirt the issue: "The referenced content group configuration is invalid or has been deleted."?

@catong This is a good catch, thanks. I was thinking about plugging in the xblock type, so it would read as:

This unit refers to...

or

This problem refers to...

Does that work? Note that I tweaked the wording so that it works for both one invalid reference as well as for two or more. So your suggested rewording would need both singular and plural forms.

A problem with my suggestion is that I don't know if we have localized xblock type names, so we might need to use quote the name.

Why not just "this component"?

@explorerleslie I'm thinking about the case when we support visibility controls on things other than just components. This code will work just as well for sections, subsections and units, so I don't want to call them components.

@dan-f Let's work out a plan for this...

cahrens · 2014-12-20T20:53:46Z

i18n throughout this file.

cahrens · 2014-12-23T19:15:29Z

👍

andy-armstrong · 2014-12-23T19:15:45Z

Nit: I'd make this an inner function, as it is only used in test_no_visibility_set.

andy-armstrong · 2014-12-23T19:26:55Z

Why the abbreviation?

andy-armstrong · 2014-12-23T21:35:11Z

I'm finished with my review. Nice work everyone! There are just a few minor issues to iron out.

andy-armstrong · 2014-12-23T22:21:13Z

👍 once the 'xblock' is removed from the two messages (however you choose to do it).

TNL-652

Add Studio support for cohorted courseware

dan-f reviewed Dec 12, 2014
View reviewed changes

Comment thread cms/templates/studio_xblock_wrapper.html Outdated

dan-f Dec 12, 2014

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.

Just a reminder not to miss this NOTE :)

andy-armstrong force-pushed the andya/studio-cohorted-courseware branch from 0649573 to 68fc2b1 Compare December 13, 2014 05:49

dan-f reviewed Dec 13, 2014
View reviewed changes

cahrens reviewed Dec 14, 2014
View reviewed changes

cahrens force-pushed the andya/studio-cohorted-courseware branch from 9f1bfea to fa62d45 Compare December 16, 2014 02:08

andy-armstrong force-pushed the andya/studio-cohorted-courseware branch from 07935de to cc53782 Compare December 17, 2014 19:08

talbs mentioned this pull request Dec 17, 2014

WIP - Studio: Group Access Level UI #6095

Closed

catong reviewed Dec 17, 2014
View reviewed changes

jimabramson force-pushed the cohorted-courseware branch 2 times, most recently from 23ab6ed to b4ba648 Compare December 18, 2014 19:06

dan-f force-pushed the andya/studio-cohorted-courseware branch from 0d560e2 to 893894b Compare December 19, 2014 14:50

cahrens reviewed Dec 20, 2014
View reviewed changes

Comment thread cms/templates/visibility_editor.html Outdated

cahrens Dec 20, 2014

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

i18n throughout this file.

andy-armstrong reviewed Dec 23, 2014
View reviewed changes

dan-f force-pushed the andya/studio-cohorted-courseware branch from 15afacc to f8f7adc Compare December 23, 2014 19:22

andy-armstrong reviewed Dec 23, 2014
View reviewed changes

Comment thread cms/djangoapps/contentstore/views/item.py Outdated

andy-armstrong Dec 23, 2014

Copy link
Copy Markdown

Contributor Author

Choose a reason for hiding this comment

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

Why the abbreviation?

dan-f force-pushed the andya/studio-cohorted-courseware branch from f8f7adc to 6dd561f Compare December 24, 2014 15:27

andy-armstrong force-pushed the cohorted-courseware branch from a66401c to ba43e44 Compare December 24, 2014 18:58

andy-armstrong force-pushed the andya/studio-cohorted-courseware branch from 6dd561f to 5695dda Compare December 24, 2014 19:00

dan-f force-pushed the andya/studio-cohorted-courseware branch from 5695dda to 33aef62 Compare December 24, 2014 19:57

Studio: adding UI and styling for component-level access modal

692b422

dan-f force-pushed the andya/studio-cohorted-courseware branch 3 times, most recently from ef1f765 to 942e31b Compare December 24, 2014 22:42

Studio support for cohorted courseware

cb2cb9e

TNL-652

dan-f force-pushed the andya/studio-cohorted-courseware branch from 942e31b to cb2cb9e Compare December 25, 2014 17:10

dan-f added a commit that referenced this pull request Dec 25, 2014

Merge pull request #6227 from edx/andya/studio-cohorted-courseware

5c8dd9e

Add Studio support for cohorted courseware

dan-f merged commit 5c8dd9e into cohorted-courseware Dec 25, 2014

dan-f deleted the andya/studio-cohorted-courseware branch December 25, 2014 18:36

Uh oh!

Conversation

andy-armstrong commented Dec 12, 2014

Uh oh!

andy-armstrong commented Dec 12, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

explorerleslie commented Dec 12, 2014

Uh oh!

catong commented Dec 12, 2014

Uh oh!

catong commented Dec 12, 2014

Uh oh!

andy-armstrong commented Dec 12, 2014

Uh oh!

dan-f commented Dec 13, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cahrens commented Dec 14, 2014

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!

cahrens commented Dec 16, 2014

Uh oh!

andy-armstrong commented Dec 16, 2014

Uh oh!

explorerleslie commented Dec 17, 2014

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!

cahrens commented Dec 23, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-armstrong commented Dec 23, 2014

Uh oh!

andy-armstrong commented Dec 23, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants