-
Notifications
You must be signed in to change notification settings - Fork 137
feature/projects-index-state-migration #437
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?
feature/projects-index-state-migration #437
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.
I've reviewed the code changes and have some feedback:
Amazing things I noticed:
- Really like the proper RxJS patterns with takeUntil for cleanup - good memory management
- Error handling with the isLoading and hasError states is solid
- The switchMap pattern for handling route params is the right approach
- Using OnDestroy to clean up subscriptions properly
Concerns/Questions:
-
Template Issue (High Priority):
Looking at index.component.html, I see the <ng-template #loadingOrError> is defined twice (lines 37-41 and 42-45). That's going to cause issues. Should probably remove one of those duplicates. -
UI/UX:
The new component has a "Complete" button for tasks (completeTask(task)) but I don't see that method implemented in the .ts file. Is that missing or is the template from a different version?
The new UI looks quite different from the old AngularJS version (which reused units/states/index/index.tpl.html). Was this intentional? Should we keep the same UI for consistency? -
Service Injection:
I see you're injecting ProjectService and ListenerService but the ListenerService doesn't seem to be used anywhere in the component. Is that needed? -
Type Safety:
project: any and unit: any - could we create proper interfaces for these? Would make the code more maintainable
Same with the ViewType.PROJECT - good that you're using an enum, but worth checking if project type aligns with what setView expects -
Empty Observable:
When there's no valid projectId, you're returning new Subject() but never completing it. Would EMPTY from rxjs be cleaner here?
Minor Issues:
The .scss file is empty - do we need it at all?
The old CoffeeScript controller had roleWhitelist: ['Student', 'Tutor', 'Convenor', 'Admin', 'Auditor'] - how is this being handled in the new Angular routing? Route guards?
Thanks for the detailed reviews I’ve addressed all high‑priority feedback across three commits- Removed duplicate <ng-template #loadingOrError> Removed unused ListenerService injection Added Project and Unit interfaces for type safety Replaced new Subject() with EMPTY for cleaner observable handling Removed the unused “Complete” button from the template Verified ViewType.PROJECT usage aligns with setView() expectations
Deleted the empty .scss file since the component has no styles About roleWhitelist |
|
PR Review Feedback Nice use of RxJS patterns overall — switchMap + takeUntil is the right combo for route-driven streams and cleanup. Loading/error state handling is clean (isLoading, hasError) and makes the template logic easy to follow. Good call using OnDestroy for subscription lifecycle management — avoids the “silent leak” issues we’ve seen before. Generally readable flow: params → fetch → set state. Easy to reason about. Concerns / Questions
Same concern as the other reviewer: in index.component.html the template reference <ng-template #loadingOrError> is declared twice (looks like lines ~37–41 and ~42–45). That’ll cause confusion / unexpected behavior. Suggest removing the duplicate and keeping a single source of truth for the loading/error block.
The template shows a “Complete” button calling completeTask(task), but I don’t see that method in the component TS. Is it missing? Or did the template come from a different revision? The new UI looks noticeably different than the old AngularJS screen (which reused units/states/index/index.tpl.html). Was the redesign intentional? If not, we may want to preserve the existing layout for consistency (especially if users are used to the legacy UI patterns).
ProjectService and ListenerService are injected, but ListenerService doesn’t appear to be used. If it’s not needed, I’d remove it to avoid confusion and keep DI clean. If it is needed later, maybe add a comment or implement the intended usage so it doesn’t look accidental.
project: any and unit: any are a bit risky long-term. Could we define interfaces (even minimal ones) for Project and Unit? This would also help prevent template/runtime mismatches. Also worth verifying ViewType.PROJECT aligns with what setView() expects. Enum usage is great — just want to confirm the target type matches the service contract.
When projectId is invalid/missing, the code returns new Subject(). That subject never completes and is a bit unusual as a “do nothing” return. Using EMPTY from RxJS would be cleaner and communicates intent better. Minor / Cleanup Items .scss file is empty — do we actually need it? If not, could remove it to reduce noise. Legacy controller had: roleWhitelist: ['Student', 'Tutor', 'Convenor', 'Admin', 'Auditor'] How is that enforced now? Route guard? Resolver permission check? Backend enforcement only? Would be good to document where this is handled so we don’t accidentally drop access control during migration. |
Thanks for the detailed review I’ve gone through each point and applied the required changes |
Description
This PR completes the migration of the Projects Index state from AngularJS to Angular.
The previous implementation relied on an AngularJS state, controller, and template.
As part of the migration effort, I created a new Angular component and moved all
relevant logic, routing, and view behavior into the Angular framework.
I also cleaned up the old AngularJS state by disabling it so that the new Angular
component is now the source of truth for the Projects Index page.
This PR contains only the intended migration files.
Fixes # (issue) N/A
Type of change
How Has This Been Tested?
/projects/<validProjectId>and confirming the page loads.Testing Checklist:
Checklist:
##Screenshots (Evidence)
Screenshot 1 - Projects Index Page Rendering

Shows the page loading at /projects/ with correct layout and navigation. Confirms that the new Angular component is rendering successfully and the user interface is visible.
Screenshot 2 - Console Showing No Errors

Displays the browser’s DevTools Console tab after refreshing the page. Confirms that there are no red errors or warnings related to the migrated component. Only expected optimization warnings from legacy modules are present.
Screenshot 3 - Network Tab Showing Successful API Calls

Shows the DevTools Network tab after page load. Confirms that getProject and getUnit API calls return 200 OK, verifying that project and unit data are loading correctly.
Screenshot 4 - Angular Component Folder Structure

Displays the file explorer in VS Code showing the new Angular component files inside src/app/projects/states/index/. Includes:
Confirms that the migration was scoped and structured correctly.
Screenshot 5 - Routing Configuration

Shows the route definition in app-routing.module.ts for projects/:projectId. Confirms that the new Angular component is registered with Angular Router and replaces the legacy AngularJS state.
How the behaviour can be tested / replicated by others
Anyone reviewing this PR can follow the steps below to confirm the migrated Projects Index state works as expected:
Start the frontend application
Launch the Angular development server using the project’s standard startup command.
Sign in using any valid development credentials
Once the app loads, log in with the default test credentials provided in the project documentation.
Navigate to the Projects Index route
In the browser, go to:
Replace
<validProjectId>with a project ID that exists in your local database.Confirm the page renders correctly
The Projects Index page should load without errors.
If the user isn’t enrolled in a unit, the “not enrolled” message will appear - this is expected behaviour.
Check the browser console
Open DevTools → Console and refresh the page.
There should be no red errors.
Any AngularJS optimisation warnings are normal and unrelated to this migration.
Check the Network tab
Open DevTools → Network and refresh the page.
API calls such as
getProjectandgetUnitshould return successful responses (e.g.,200 OK).Verify routing behaviour
/home.Confirm the old AngularJS controller is no longer used
The legacy
ProjectsIndexStateCtrlshould not appear in logs or be triggered during navigation.