Migrate/group member list

[Pasindufdo98]

Migration : Group Member List

Description

This component displays the list of members in a selected group, allowing users to add new members or remove existing ones. Also manages related actions such as showing member details and updating the list when group data changes.

Before:

After:

Fixes # (issue)

How Has This Been Tested?

Tested by adding and removing members across different groups to confirm the list updates correctly and toolbar actions respond based on user role.

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
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have requested a review on the Pull Request

[returnMarcco]

Hi @Pasindufdo98,

This is a peer review (junior OR senior) for these changes.

All in all, the component is functioning as expected. I am able to add and remove members to and from a group, respectively. Good work. Below are some things that will need changing/investigating:

1. Please remove the reference to the old .js component-file in doubtfire-angularjs.module.ts. This is causing the app to not load.

2. I've noticed what seems to be potentially infinite triggering of the members() getter method highlighted in the below screenshot. I'm not sure if this behavior was present in the old component, but you can reproduce it by creating a group, and by putting a breakpoint on the method name shown in the below screenshot, this method should trigger on its own. If you step through the code in the debugger, you should notice that this method is constantly called in succession:

3. The - sign on the Remove button seems like it needs a space between itself and the Remove text.

Once these requested changes have been made/investigated, please leave your response in this PR and re-request a review from me from the PR Reviewers list.

Cheers

[returnMarcco]

Hi @Pasindufdo98,

Can you please update the Migration Progress List section of README.MD.

Cheers

[Pasindufdo98]

Hi @Pasindufdo98,

This is a peer review (junior OR senior) for these changes.

All in all, the component is functioning as expected. I am able to add and remove members to and from a group, respectively. Good work. Below are some things that will need changing/investigating:

1. Please remove the reference to the old .js component-file in doubtfire-angularjs.module.ts. This is causing the app to not load.
2. I've noticed what seems to be potentially infinite triggering of the members() getter method highlighted in the below screenshot. I'm not sure if this behavior was present in the old component, but you can reproduce it by creating a group, and by putting a breakpoint on the method name shown in the below screenshot, this method should trigger on its own. If you step through the code in the debugger, you should notice that this method is constantly called in succession:

3. The `-` sign on the `Remove` button seems like it needs a space between itself and the `Remove` text. Once these requested changes have been made/investigated, please leave your response in this PR and re-request a review from me from the PR `Reviewers` list.

Cheers

Hey @returnMarcco ,
Thanks for the catch Jason, the repeated calls were from the template getter being re-evaluated every change-detection pass. I did the fixes as follows and added some more improvements too.

Fixes implemented

• Replaced get members()/sortedMembers() with fields, recalculated via resort() only when data or sort order changes.
• Enabled ChangeDetectionStrategy.OnPush and added markForCheck() after async loads to trigger view updates when needed.
• Updated removeMember to refresh local arrays so rows disappear right away, no page refresh needed.
• Verified (members-loaded) output correctly toggles the toolbar.
• The component now updates only when expected and avoids unnecessary cycles too.

[disururathnayake]

Hi @Pasindufdo98 ,

I have successfully pulled and reviewed your code. Nice work on the migration. The component is working as expected

[returnMarcco]

Hi @Pasindufdo98,

When I attempt to add a student to a group, that student doesn't seem to show in the list for me. Can you please check to see if you're experiencing the same?

Also, when I add a group to a group set and reload the page, the group no longer shows in the list (it exists in the database though). Can you confirm if this is the case for you? This probably isn't a result of your changes though.

Cheers

[Pasindufdo98]

Hi @Pasindufdo98,

When I attempt to add a student to a group, that student doesn't seem to show in the list for me. Can you please check to see if you're experiencing the same?

Also, when I add a group to a group set and reload the page, the group no longer shows in the list (it exists in the database though). Can you confirm if this is the case for you? This probably isn't a result of your changes though.

Cheers

Hi @returnMarcco ,
Thanks for reviewing again.

• I fixed the issue with not updating member list in the frontend. Now it should be working when adding and removing a member without reloading.
• When adding a group to a group set, it shows immediately without any error for me.

[Pasindufdo98]

Hi @Pasindufdo98,

When I attempt to add a student to a group, that student doesn't seem to show in the list for me. Can you please check to see if you're experiencing the same?

Also, when I add a group to a group set and reload the page, the group no longer shows in the list (it exists in the database though). Can you confirm if this is the case for you? This probably isn't a result of your changes though.

Cheers

Hey @returnMarcco,
I noticed this issue only occurs when signing in as an admin. I also tested it on the old AngularJS version and the same problem exists there, so it’s not introduced by my migration changes to group-member-list. When logged in as a convenor, the functionality seems to work as expected.

[returnMarcco]

Hey Pasindu,

I've left a few comments as feedback. Reach out if you have any questions.

Cheers

[returnMarcco]

Pre-approving this PR. Please see last recommended change. After making this change, please contact Brian as soon as possible so he can review this before opening an upstream PR. Good work.

[Pasindufdo98]

Latest Appearance

[chelaz1234]

Hi @Pasindufdo98,

Nice, thorough migration of group-member-list with clear commit structure. All functions are work as expected. Overall good job!

[disururathnayake]

Hi @Pasindufdo98 ,

I have successfully pulled and reviewed your code. Nice work on the migration. The component is working as expected