-
Notifications
You must be signed in to change notification settings - Fork 505
Difficult to find users to delete from group in large groups #3286
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: main
Are you sure you want to change the base?
Conversation
|
Hi @iam-vir-harshit, |
7f2d045 to
dcea3ba
Compare
|
Rebased with dspace-angular main branch |
tdonohue
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.
@iam-vir-harshit : Thanks also for this PR! Apologies for the delay in reviewing this, but we gave this a code review as a team in today's Developer Meeting. Overall, the code looks good, but again we are noticing that automated tests are missing:
- The changes to
eperson-form.component.tsshould all be tested ineperson-form.component.spec.ts - Similarly, the changes to
members-list.component.tsshould all have automated tests inmembers-list.component.spec.ts.
Once those automated tests are added, the Codecov warnings on this PR (in the "Files changed" tab) should go away (or mostly go away). We can then work to get this tested and merged quickly. Thanks again!
|
Hi @iam-vir-harshit, |
|
@iam-vir-harshit : Just a friendly reminder, that if you want this PR to be considered for the 9.0 release, we will need to have the feedback above resolved soon (and any merge conflicts resolved). Our 9.0 feature merger deadline is March 28. Anything not approved & merged by that date will unfortunately need to be delayed for the next major release. |
|
Hi @iam-vir-harshit, |
|
Hi @iam-vir-harshit, |
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.
@iam-vir-harshit and @PitbaranK : We reviewed this as a team in today's DSpace Developers Meeting. I've summarized the code review feedback in the comments below. Let us know once you've addressed these comments, and hopefully we can find someone to quickly re-review or test it.
|
|
||
| @if (activeEPerson$ | async) { | ||
| <h1 class="border-bottom pb-2">{{messagePrefix + '.edit' | translate}}</h1> | ||
| <h1 class="border-bottom pb-2">{{messagePrefix + '.edit' | translate}}</h1> |
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.
All these re-alignment changes should be removed, as it makes the code more difficult to read. It's possible these occurred automatically from your IDE, but please remove any unnecessary changes to code alignment from this file.
| </tr> | ||
| </thead> | ||
| <tbody> | ||
| @for (group of (groups$ | async)?.payload?.page; track group) { |
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.
All the changes in this file appear to be code realignment. We don't see where you've added the delete button to this page? Is there code missing?
You should remove all the alignment changes (i.e. undo all current changes to this file), and double check that the delete button has been added into this HTML.
| * Deletes a given group from the Group list of the eperson currently being edited present in | ||
| * @param group group we want to delete as of which the current eperson being edited is member of | ||
| */ | ||
| deleteGroupFromMember(group: Group) { |
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.
This method is only being called in your spec file. I suspect it's supposed to be called by an actual delete button.
| searchDone: boolean; | ||
|
|
||
| // Whether or not user has done a EPeople Member search yet | ||
| searchCurrentMembersDone: boolean; |
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.
This variable is never used. We believe it can be removed.
|
|
||
| selectedReviewers: EPerson[] = []; | ||
|
|
||
| searchCurrentMembersForm: UntypedFormGroup; |
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.
Why did you add this form to the reviewers-list.component.ts? It doesn't appear to be used at all? Either it should be used or it should be removed.
|
|
||
| "admin.access-control.epeople.form.table.remove": "Remove", | ||
|
|
||
| "admin.access-control.epeople.form.table.edit.buttons.removegroup": "Remove member from group \"{{name}}\"", |
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.
These two i18n keys are not used at this time. But, I suspect it is because there's code missing from your eperson-form.component.html file.
|
Hi @tdonohue, @MarieVerdonck, |
|
Hi @iam-vir-harshit, |
|
@iam-vir-harshit : This PR has a merge conflict because it looks like you've updated every Instead, we recommend only updating the |
… Translation Files Changes except en.json5
|
Reopening this PR with the recommended *.json5 translation file changes rollback except the en.json5 changes. |
…g Rollback Changes
- Missing Changes
- Fix Indentation in en.json5
|
@tdonohue, |
|
Hi @iam-vir-harshit, |
References
Description -
group page.
List of changes in this PR:
For searching the current editing group members (includes frontend and backend task) :-
a) - In the Frontend, we have added a search form;
b) - In the Backend, we have added a search method("findIsMemberOf") that receives current editing group id and query parameter which
searches in the current member's list of the editing group;
For deleting the eperson from groups (includes only frontend task) :-
a) - We have only added an extra column in the edit eperson page for giving remove button;
b) - We are utilizing the same api created for unlinking an eperson from the group in the edit group page;
Steps to reproduce:-
Task 1- Search option in the group edit page (in current epeople members section)
Task 2- Remove button in the particular EPeople edit page