Simplify class links by removing special cases for teachers & owners - #1003
Conversation
Test coverage93.57% line coverage reported by SimpleCov. |
66dff8c to
189175f
Compare
zetter-rpf
changed the title
zetter-rpf
changed the title
zetter-rpf
changed the title
There was a problem hiding this comment.
🟡 Changes recommended
It introduces an undocumented API response change (removal of redirect_url) and leaves now-dead status/controller branches that should be cleaned up for clarity and contract consistency.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR simplifies the join-code flow so that only students can join classes via join codes, removing the previously introduced special cases for teachers and owners.
Changes:
- Updated
JoinStatusServiceso only student membership/roles produce joinable statuses; teacher/owner users now yield:not_a_student. - Updated
Api::JoinController#createto remove teacher/owner join behaviors and to return an empty JSON object on success. - Updated request/service specs to match the new status and authorization behavior.
File summaries
| File | Description |
|---|---|
| app/services/join_status_service.rb | Removes role-based special cases and tightens status computation around student eligibility. |
| app/controllers/api/join_controller.rb | Aligns POST join behavior to forbid non-students and simplifies successful responses. |
| spec/services/join_status_service_spec.rb | Updates expectations for teacher/owner scenarios to :not_a_student. |
| spec/requests/join_controller_spec.rb | Updates request specs for teacher/owner flows and simplifies successful POST assertions. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
189175f to
51a9ebf
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The response-contract and teacher non-enrollment behavior need explicit regression coverage, and obsolete teacher-join code remains.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
spec/requests/join_controller_spec.rb:241
- The example pre-creates the teacher's school role and only checks the status, so it does not verify the newly intended behavior that posting a class code cannot create a
ClassTeacher. Assert no class-teacher row is added and name the example after that behavior.
it 'does not add the teacher to the school' do
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
When taking to Jason, he was unsure if it was safe for teachers to be able to use the same join codes as students. I've decided to remove it handling teachers adds more complexity to the frontend. Instead all non-student accounts will be a shown a link to get to the class which they can use if they are already a teacher or owner. This also removes the owner status. Now all non-students are treated the same. We may think this is a useful feature in the future so could add it back in if we want. fixup
This is not a URL, it's not even full root-relative path as the locale needs to be added. The frontend knows all that it needs to construct the URL from the other params in the get response so this can be removed.
51a9ebf to
13b5eec
Compare
| when :joinable | ||
| add_student_to_school_and_class | ||
| render json: { redirect_url: class_redirect_path }, status: :ok | ||
| render json: {}, status: :ok |
There was a problem hiding this comment.
this and :already_member both return {}
can we reduce this ?
There was a problem hiding this comment.
The behaviour is different as there's a call to add_student_to_school_and_class in the joinable case- combining the case statements would move the condition elsewhere.
Status
What's changed?
We had started implementing the ability for class codes to be used by teachers and owners, but this functionality wasn't fully working in the frontend. We're also not sure if teachers should be able to join classes they are not already members of.
See https://github.com/RaspberryPiFoundation/digital-editor-issues/issues/1780 for re-introducing.
Note this is safe to deploy before https://github.com/RaspberryPiFoundation/editor-standalone/pull/1012
as no one is using join codes on production yet/