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

Assignment Updates: auto select first unit if a course is assigned to a new section #31179

Merged
merged 8 commits into from Oct 14, 2019

Conversation

Erin007
Copy link
Contributor

@Erin007 Erin007 commented Oct 9, 2019

Description

New Sections
When a teacher is a creating a new section and selects a course for the primary assignment dropdown, we will now default the secondary assignment dropdown to the first unit in that course. Previously, we defaulted the secondary assignment dropdown to the empty option.
new-section

Existing Sections
If a teacher is editing an existing section and a primary and secondary assignment is already selected, we preserve those selections.
editing-course-unit-already

Even if the secondary assignment selected is empty. This is because the teacher may have intentionally not assigned a unit. This is a rare, but valid, use case.
editing-course-no-section

Additionally, the "no assignment" option is now the last option in the secondary assignment dropdown rather than the first.

Links

Testing story

Reviewer Checklist:

  • Tests provide adequate coverage
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

Copy link
Member

@davidsbailey davidsbailey left a comment

Choose a reason for hiding this comment

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

Nice work Erin!

When you fix the UI tests that are failing in drone, please add a check that the correct secondary assignment is appearing in the dropdown.

apps/src/templates/teacherDashboard/AssignmentSelector.jsx Outdated Show resolved Hide resolved
Comment on lines 259 to 269
// If the user is setting up a new section and selects a course as the
// primary assignment, default the secondary assignment to the first
// script in the course.
const noCurrentSecondaryAssignment = selectedSecondaryId === 'null_null';
const defaultScriptId = assignments[selectedPrimaryId]
? assignments[selectedPrimaryId].scriptAssignIds[0]
: null;
const secondaryId =
newSection && noCurrentSecondaryAssignment
? defaultScriptId
: selectedSecondaryId;
Copy link
Member

Choose a reason for hiding this comment

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

nice structure!

@@ -323,12 +336,11 @@ export default class AssignmentSelector extends Component {
</div>
<select
id="uitest-secondary-assignment"
value={selectedSecondaryId}
value={secondaryId}
Copy link
Member

Choose a reason for hiding this comment

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

this could end up being undefined (if there are no scriptAssignIds) or null. does the right thing happen in both cases?

@Erin007
Copy link
Contributor Author

Erin007 commented Oct 11, 2019

@davidsbailey I revised and updated/extended test coverage. Can you take another look when you get a chance, please? Thanks!

Copy link
Member

@davidsbailey davidsbailey left a comment

Choose a reason for hiding this comment

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

Great work Erin! Just one question, otherwise LGTM!

@@ -337,6 +348,7 @@ export default class AssignmentSelector extends Component {
</option>
)
)}
<option value={noAssignment} />
Copy link
Member

Choose a reason for hiding this comment

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

why is this at the end now? I think we would want to have a good reason to change this if it affects the ordering in the UI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, Amanda asked me to move it to make it slightly less likely that folks will pick this option.

@Erin007 Erin007 merged commit 19535e3 into staging Oct 14, 2019
@Erin007 Erin007 deleted the auto-select-first-unit branch October 14, 2019 20:45
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