Assign course from teacher-dashboard #15673
Conversation
| }; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The approach I've taken involves setting assign_id and is_course on the client, and making sure they are updated as appropriate.
I suspect this isn't the most correct way of doing things in angular, where they have this idea of a model that is kept in sync between client/server. That said, the approach I've taken is one that I can understand/reason about, and will presumably also be somewhat short lived as we plan to move this table to React in the very near future.
| $scope.sections.$promise.then(sections => { | ||
| $scope.sections.forEach(section => { | ||
| section.assign_id = $scope.getAssignmentId(section); | ||
| }); |
There was a problem hiding this comment.
assign_id is the approach i've taken for dealing with the fact that scripts/courses each have their own id space.
| @@ -110,6 +110,8 @@ module ScriptConstants | |||
| # This can be used to override that, with lower numbers ordered sooner, and higher numbers | |||
| # ordered later. | |||
| CATEGORY_ORDERING_PRIORITY = { | |||
| # We want full courses to show up first in our list | |||
| full_course: -1, | |||
There was a problem hiding this comment.
Are these values important (the relative positioning all that matters?)? Are these values ever persisted anywhere?
If no (yes) and no, I'd vote against moving into the negative numbers.
There was a problem hiding this comment.
They are not persisted anywhere afaik, but we do use 0 as default in places (which feels more intuitive than bumping everything by one and having default be 1)
There was a problem hiding this comment.
Ah, then a negative number would be appropriate. Carry on.
| @@ -581,6 +581,9 @@ def self.update_if_owner(params) | |||
| # explicitly clear script_id (unless we're also passed in a valid script id | |||
| # as a param | |||
| fields[:script_id] = nil | |||
| else | |||
| # If no course_id provided, make sure we clear an existing course_id | |||
There was a problem hiding this comment.
This should be "If no valid course_id provided..."? Alternately, I'd expect the else to be an elsif.
| } | ||
| section_id = DashboardSection.create(params) | ||
| row = Dashboard.db[:sections].where(id: section_id).first | ||
| assert_equal course_id, row[:course_id] |
There was a problem hiding this comment.
This was already asserted in "assigns a course to a section without one" and not part of this test. Aside: perhaps adding the assert_nil row[:script_id] to this test is appropriate?
There was a problem hiding this comment.
Re: your aside - I have exactly that on line 523, or am I misunderstanding your comment?
This test is meant to be about the update_if_owner call. The asserts here are to ensure that our initial setup is in the right state before we make that call.
| $scope.sectionToSave = $scope.sections.indexOf(section); | ||
| $('#assign-confirm').modal('show'); | ||
| return; |
There was a problem hiding this comment.
You still want to return here before the $scope.send_save() call, that way the assignment isn't saved until the hoc script assignment warning is dismissed, and so that the script assignment doesn't get saved if they hit cancel.
| if (assignIdChanged) { | ||
| const assignable = $scope.assignable_list.find(a => a.assign_id === section.assign_id); | ||
| // update course/script assigned to section. Right now a section can only | ||
| // have one or the other, but that will change in th efuture. |
There was a problem hiding this comment.
in th efuture.
nit: typo
| @@ -876,7 +893,11 @@ function main() { | |||
| // fill in the course dropdown with the section's default course | |||
| $scope.section.$promise.then( | |||
| function (section) { | |||
| $scope.script_id = section.script.id; | |||
| // TODO:(bjvanminnen) - also handle case where we have a course, but not | |||
There was a problem hiding this comment.
purposefully leaving a TODO here?
There was a problem hiding this comment.
Yeah, this is future work.
When creating/updating a section, in addition to scripts, we will now show you a set of full courses you can also assign to the section (currently we will only show these if you have the assignCourses experiment enabled)
Did a bunch of manual testing (Poorva also did some on an adhoc I created). Also validated that the teacher-dashboard.feature UI test passes.