Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show multiple versions of csd course in EditSectionForm #22113

Merged
merged 26 commits into from
May 2, 2018

Conversation

davidsbailey
Copy link
Member

@davidsbailey davidsbailey commented Apr 27, 2018

Background

Description

Show multiple versions of the CSD course in the EditSectionForm component, which is visible when creating or editing a section from the teacher homepage.

before:
screen shot 2018-04-27 at 2 42 34 pm

after:
screen shot 2018-04-27 at 2 41 05 pm

Terminology

It's worth calling out that the terminology in this part of the product is not very good at all. See spec for product terminology. Also see terminology comparison spreadsheet for my desperate attempt to make sense of it all.

Under the hood

Although the visible difference is minimal, this required the following changes under the hood:

  • create csd-2018 course and add scripts to it
  • create "assignment groups", which is similar to the "primaryAssignmentIds" list from earlier, but with primary assignment ids which are different versions of each other "grouped" together (see terminology comparison spreadsheet again)
  • put existing scripts into assignment groups each containing only one script, so that the UI can function in the same way for all courses and scripts
  • return additional grouping info (assignment_group_name and version_year) for courses from the server

Caveats

  • a lot of the data we are showing (base_name assignment_group_name, version_year) are "fudged". for courses, these are derived from the course name (tbd whether this will change in the future). for scripts, we assume the year is 2017 and that the assignment_group_name is equal to the script name (this will definitely change in the future).
  • we are showing all hidden scripts to users in the courseVersions experiment, so that the scripts (code term) csd2-2018, csd3-2018, etc will show up under the course csd-2018. If we can unhide these scripts before launch, then this behavior can be removed before the experiment is launched, as described in comments. if not, we'll need to find a way for valid_scripts to return only the hidden scripts associated with additional course versions. Poorva will get back to us on whether this is necessary.

@davidsbailey
Copy link
Member Author

FYI @poorvasingal , mostly to see that things are happening, but also to review the insidious terminology comparison spreadsheet

@davidsbailey
Copy link
Member Author

@islemaster I should add that I'm actively soliciting better names for "assignment group" (e.g. "all versions of the CSD course"). I don't like any of the names I've come up with, but in case any of them help inspire you, here are the alternatives I've considered:

  • assignable group
  • multi-versioned course
  • unversioned course
  • top-level course
  • Full course

@islemaster
Copy link
Contributor

Stream-of-consciousness on terminology:

There's a partial fit here with terminology for car models, model years and design revisions; it's too bad 'model' is taken in our namespace. My one takeaway from this idea is that I like 'revision' a little better than 'version' in this context, and having a hard time saying why - there's no agreement on how the two terms are different, but outside of software 'revision' seems to be associated with annual releases and incremental improvements. It's also less likely to collide with how we usually talk about software versions.

It looks like one of the gaps is an umbrella term for courses and scripts. I like "assignable" better than "assignment" but either might still be too generic. AFAIK we're not using "curriculum/curricula" as an exact term anywhere, and this might be an appropriate place for it.

Of course, I tend to be overly verbose in all things, so consider this food for thought and not a finalized proposal:

  • Course family or Course brand: /courses/csd (instead of "course group")
  • Curricula families: /s/frozen, /courses/csd, /s/coursea
  • Assignable curricula revisions: /s/frozen, /s/coursea-2018, /courses/csd-2018, /courses/csd1-2018

Copy link
Contributor

@islemaster islemaster left a comment

Choose a reason for hiding this comment

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

Awesome work in a very tangled system. Now that I understand the terms, changes LGTM.


// If our current secondaryId is in this course, default to that
Copy link
Member Author

Choose a reason for hiding this comment

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

I've removed the behavior which did the following -- when a section is assigned to a course and unit, and you change the primary assignment to something else and then back again, try to set the secondary assignment back to the originally assigned unit.

strangely, this was passing in unit tests but not working end to end. This is because the reportChange method caused redux to clear the assigned script id, which doesn't happen in the unit test on the unconnected component.

@davidsbailey
Copy link
Member Author

davidsbailey commented May 2, 2018

Apologies for all the commit spam, @islemaster . I believe this is ready to go now. If you'd feel guilty not re-reviewing, you can probably focus on 9008231 and 0cf32e6 which make sure that users assigned to e.g. csp1 but not csp will have csp1 added back to the assignment family dropdown. Just for fun, I counted and there are 331 users in prod assigned to scripts csp1-6 or csd1-6 but not the corresponding course, whom this would affect.

Copy link
Contributor

@islemaster islemaster left a comment

Choose a reason for hiding this comment

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

👍

@@ -15,11 +15,17 @@ const noAssignment = assignmentId(null, null);
const decideLater = '__decideLater__';
const isValidAssignment = id => id !== noAssignment && id !== decideLater;

const hasAssignmentFamily = (assignmentFamilies, assignment) => (
assignment && !!assignmentFamilies.find(assignmentFamily => (
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: array.some(predicate) would be more idiomatic here than !!array.find(predicate).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants