Skip to content

Conversation

@mannat2634
Copy link

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 projects2 parent state, inheriting the project data via the standard project$ observable pattern used across the repository. This resolved initial TypeError issues by ensuring that the component only renders once the unit data has been successfully resolved.

Key Changes:

  • Removed legacy projects/states/groups/groups.coffee and groups.tpl.html.
  • Defined the projects/groups state in doubtfire.states.ts as a child of projects2.
  • Updated ProjectGroupsComponent and its template to handle the project$ observable resolve, matching the architecture of the ProjectDashboardComponent.
  • Unlinked the doubtfire.projects.states.groups module from the AngularJS bootstrap process.

Fixes # (migration)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

The migration was verified by testing the end-to-end workflow between Staff and Student roles:

  1. Admin Setup: Logged in as a Convenor, navigated to Unit Administration -> Groups, and created a new Group Set with student-creation enabled.
  2. Student Navigation: Logged in as a Student, selected the subject from the Home Page, and used the header task dropdown to select Groups List.
  3. Data Verification: Confirmed the new Angular route loaded correctly at #/projects2/:id/groups.
  4. Functional Testing: Verified the student can see existing groups, create a new group, and click Join to successfully enter a group.
  5. Edge Case Testing: Navigated to a unit with no Group Sets enabled and verified the "No Group Work" placeholder message and icon were displayed.

Screenshots

Before Migration (AngularJS)

image

After Migration (Angular) - Groups Enabled

image image

After Migration (Angular) - No Groups

image

Testing Checklist:

  • Tested in latest Chrome
  • Tested in latest Safari
  • Tested in latest Firefox

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code in hard-to-understand areas
  • My changes generate no new warnings
  • I have requested a review from @macite and @jakerenzella on the Pull Request

Copy link

@31Husain31 31Husain31 left a 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.

@ishika021
Copy link

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.

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.

3 participants