-
Notifications
You must be signed in to change notification settings - Fork 137
Migration/groups.coffee #431
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
base: 9.x
Are you sure you want to change the base?
Conversation
31Husain31
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Peer Review for PR #431: Migration/groups.coffee
Reviewer: Husainuddin Mohammed (223380186)
General Information
Type of Change: New feature (AngularJS → Angular migration)
Code Quality
Repository: Correct repo
Readability: Code changes are straightforward and easy to follow
Maintainability: Clean refactor that follows existing patterns in the codebase
Functionality
Correctness: Based on your screenshots, the groups functionality is working as expected
Existing Functionality: UI looks identical between before/after - no visual regressions
Testing
Test Coverage: Manual testing documented with screenshots
Test Results: Your screenshots show:
Groups displaying correctly with tutorials and capacity
Join button working
"Joined" status showing properly
"No Group Work" placeholder displaying when no groups exist
Only tested in Chrome though - Safari and Firefox weren't checked
Documentation
Documentation: Really clear PR description with good visual evidence
PR Details
PR Description: Well explained with screenshots showing it works
Checklist Completion: Most items checked
Feedback
The migration looks clean from what I can see in the code. Your screenshots show the functionality is working properly - groups load, students can join them, and the empty state displays correctly.
One thing I noticed from your checklist - you only tested in Chrome. Since this is a migration, it'd be good to verify in Safari and Firefox too to make sure nothing broke browser-specific.
I couldn't test it locally (Docker issues on my end), but based on the code changes and your testing screenshots, this looks good to merge.
|
This looks like a solid step forward in the migration. Moving the Groups List under the projects2 route and aligning it with the project$ pattern feels consistent with the rest of the newer project pages, and the update to unwrap the observable in the template avoids the usual timing issues around unit data not being ready yet. From a behaviour point of view, nothing here looks like it would change how the groups workflow works for students. The placeholder state for units without groupwork still makes sense, and the overall UI flow appears to be preserved. One small thing to note is that the component currently exposes project$, project, and unit as inputs, while the template mainly relies on project$. That’s fine as-is, but it might be worth simplifying later to avoid having multiple sources of truth as the migration continues. Not blocking for this change. It’s also worth double-checking that all links and navigation now point to the new projects2/:id/groups route, since the old AngularJS wiring has been removed. Overall, the changes look reasonable and low risk, and I don’t see anything that would block this from being merged. |
|
Moving the Groups List under the projects2 route and aligning it with the existing project$ pattern feels consistent with the newer project pages. Unwrapping the observable directly in the template is a good call and avoids the usual timing issues where unit or project data isn’t ready when the view initialises. From a behaviour perspective, I don’t see anything here that would change the groups workflow for students. The placeholder state for units without groupwork still makes sense, and the overall UI flow appears to be preserved. One minor observation: the component currently exposes project$, project, and unit, while the template mainly relies on project$. That’s fine for now, but it might be worth simplifying later to avoid multiple sources of truth as the migration continues. Not blocking for this PR. It’s also worth double-checking that all links and navigation now point to projects2/:id/groups, since the old AngularJS wiring has been removed. Overall, the changes look reasonable, low risk, and I don’t see anything that would block this from being merged. |
|
Thank you all for reviewing my PR. As for the exposing project and unit, it seemed better choice for now so I'm going with that. I think there's no need to overengineer this portion. I've checked the links which are within scope of this change and they seem fine to me. Thank you! |
BrianDangDev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work Mannat, you can now open a PR to upstream repo.
Remember to update the planner board with the upstream PR
Description
This PR migrates the student Groups List state from legacy CoffeeScript/AngularJS to TypeScript/Angular. This move completes the transition of the student-facing group management interface to the modern Angular framework.
The state has been updated to sit under the
projects2parent state, inheriting the project data via the standardproject$observable pattern used across the repository. This resolved initialTypeErrorissues by ensuring that the component only renders once the unit data has been successfully resolved.Key Changes:
projects/states/groups/groups.coffeeandgroups.tpl.html.projects/groupsstate indoubtfire.states.tsas a child ofprojects2.ProjectGroupsComponentand its template to handle theproject$observable resolve, matching the architecture of theProjectDashboardComponent.doubtfire.projects.states.groupsmodule from the AngularJS bootstrap process.Fixes # (migration)
Type of change
How Has This Been Tested?
The migration was verified by testing the end-to-end workflow between Staff and Student roles:
#/projects2/:id/groups.Screenshots
Before Migration (AngularJS)
After Migration (Angular) - Groups Enabled
After Migration (Angular) - No Groups
Testing Checklist:
Checklist: