Allow pd-workshop sections to accept members who are in other workshops from the same organizer.#9660
Conversation
workshops from the same organizer. Removed unique constraint from followers.[user_id, student_user_id]. Added unit tests for Section.add_student.
|
When asked about the All tests are passing. The functionality should remain the same in the default case with this change, and now it has a unit test. The |
|
Can you give more context on the issues that triggered this change and what the impact is? What are the use cases for teachers/facilitators? |
|
I happened upon the I chatted with @alicesteinglass, and we agreed to change this behavior for workshops. The default logic (for classroom sections), which is unchanged, is that any student can only be in one section for a given teacher. Joining another section for the same teacher would move the student. This behavior remains unchanged, and now it has a unit test :) I first made the change only in the model ( |
|
Ah, ok. So we are keeping the constraint that students can only belong in 1 section per teacher. But we allow adults to belong in multiple sections per facilitator, when we know that those sections are workshop sections. Is that correct? |
|
And so this fix is to remove the unique constraint so that adults can join multiple workshop sections per facilitator? |
|
Organizer, not facilitator, but essentially that is correct. Every workshop has a single organizer, who is also the section owner, and may have many facilitators. Currently the facilitators have no relationship to the section. The only code path which deviates from the existing behavior, and will not move followers, is this one from the workshop join section confirmation page, which only applies to workshop sections. Though I did just realize, someone could browse directly to the workshop section join confirmation page (/pd/workshops/join/:section_code) instead of |
|
Nevermind. If there is no workshop associated with a section code (whether or not a section exists for that code), |
|
Thanks for explaining, makes more sense while reviewing. |
|
Also worth noting: the first 2 unit tests I added test the existing functionality, and succeed without any of the changes (you can test this locally on staging). Only the 3rd one, which validates the new functionality for workshop sections, won't work without this new functionality. |
| assert new_section.students.exists?(student.id) | ||
| end | ||
|
|
||
| test 'add_student with move_for_same_teacher: false does not move student' do |
There was a problem hiding this comment.
I wonder if this is confusing for anyone else. Terminology wise we tend to use student and follower interchangeably (same with teacher and section owner). In this test we're confirming the follower stays in both sections, but in reality we never want this to be true for real students right? Not something to block this PR, just some thoughts.
There was a problem hiding this comment.
True. The section has a single user (i.e. owner / teacher) and multiple student_users (i.e. followers / students). Soon, sections will be allowed to have multiple users (owners) as well. The terminology is pretty confusing.
Here I was trying to stick with the existing terms. The point of this test case is merely that section.add_student honors the move_for_same_teacher bool param if it's supplied. The section doesn't really care if the user type is student or teacher. This is never validated. Teachers can (and do intentionally for workshops) join sections as followers.
We could more clearly express the intent by making them both teachers and calling one the organizer and the other the attendee or follower? What do you think?
There was a problem hiding this comment.
That would be clearer to me. :)
|
OK, I renamed the variables and added a comment. Please take another look. |
|
TY - LGTM |
906f3ad Merge pull request #9559 from code-dot-org/topInstructions-integrate-hints (Elijah Hamovitz) 40e882e Merge pull request #9546 from code-dot-org/animation-sizes (Caley Brock) b3a9bbb Merge pull request #9660 from code-dot-org/pd-workshop-join-section (Andrew Oberhardt) ea5c048 Merge branch 'staging' into topInstructions-integrate-hints (Elijah Hamovitz) de6955d missing semicolon fix (Caley Brock) 6bc0459 PR feedback: renamed unit test variables and added comment (Andrew Oberhardt) 85f27bb code review: fix typo (Caley Brock) 2653697 staging content changes (-Ram) (Continuous Integration) f0fcf5c Remove not needed comment (Caley Brock) 8fb23f7 Merge pull request #9675 from code-dot-org/levelbuilder (Ram Kandasamy) ad499a1 Modify p5play to initialize sprite height and width and not change them during update (Caley Brock) 79e2eff levelbuilder content changes (-Ram) (Continuous Integration) 851caa6 Merge pull request #9620 from code-dot-org/photo-credit (Tanya Parker) 0e067b3 Merge pull request #9661 from code-dot-org/peer_review_in_course_ui (Mehal Shah) df67e16 I am annoyed that this works (Mehal Shah) ac5bcd1 staging content changes (-Ram) (Continuous Integration) 6043087 Automatically built. (Continuous Integration) c43f297 remove custom scroll handler, instead hide native scrollbar (Elijah Hamovitz)
|
|
||
| def down | ||
| remove_index :followers, [:user_id, :student_user_id] | ||
| add_index :followers, [:user_id, :student_user_id], unique: true |
There was a problem hiding this comment.
The down migration is problematic, as it cannot run (successfully) after data is added that violates the uniqueness constraint. Really, the migration should probably have removed uniqueness violations (perhaps by deleting the (chronologically) oldest sections) before adding the constraint.
No action required or requested at this point.
Removed unique constraint from followers.[user_id, student_user_id].
Added unit tests for Section.add_student.