-
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 review - all high‑priority items have been addressed:
Please let me know if you'd like any further adjustments. |
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.