Skip to content
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

The number of people and groups on access control page are now configurable #1956

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

arvoConsultores
Copy link
Contributor

References

Description

The size of the groups and epeople list on access control page can be configured.

The new configs are under a new accesscontrol property in config.yml

  • accesscontrol > epeople > pageSize
  • accesscontrol > groups > pageSize

The default value is 5 for both.

Instructions for Reviewers

List of changes in this PR:

  • New configuration property have been created: accesscontrol
  • New configuration properties have been created: epeople and groups under "accesscontrol" property
  • New configuration property created under both: pageSize
  • The hard-coded number 5 have been replaced by the value of pageSize

The value of pageSize must match the pagination pages sizes of the browseby like #1771 (review)

Checklist

  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
  • My PR passes TSLint validation using yarn run lint
  • My PR doesn't introduce circular dependencies
  • My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
  • My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
  • If my PR includes new, third-party dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.

@tdonohue tdonohue added improvement 1 APPROVAL pull request only requires a single approval to merge labels Nov 7, 2022
@tdonohue tdonohue added this to the 7.5 milestone Dec 8, 2022
@tdonohue tdonohue self-requested a review December 8, 2022 16:29
@jtimal
Copy link
Contributor

jtimal commented Dec 11, 2022

Hi @arvoConsultores

I took the liberty to test this pr, when we set 5 it works fine but when I set 8 for example it shows 10
image
when i put for example 15 or 20 it shows 10 in the same way. is this expected behavior?
image

@arvoConsultores
Copy link
Contributor Author

Hi @jtimal

Yes, that is a known behavior, it's explained in the comment.

You must enter a value that exist in the property pageSizeOptions in pagination-component-options.model.ts. If you use another value it will be rounded to the nearest.

pageSizeOptions: number[] = [1, 5, 10, 20, 40, 60, 80, 100];

It's the same as #1771 (review)

Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arvoConsultores : I tested this today, and the basics work. However, I'm finding that both pages load very slowly if you set the pageSize > 5. When I set them both to pageSize: 10, each page takes a while to load (nearly 30 seconds for me). When I set both to pageSize: 20, I cannot load either page (it's almost like it hits an infinite loop because my Chrome window just starts using more and more memory)

I don't think this performance issue is caused by this PR. Rather, I'm suspecting the code on these pages has some sort of bug where larger page sizes result in major performance issues.

Basically, I think your PR is working properly...but the unfortunate result is that these pages do not perform properly when the pageSize configuration is changed. At least, that's what I'm seeing on my end (and I'd be interested to hear if this same behavior is seen by others or not -- as it's always possible I'm hitting a problem specific to my setup).

@jtimal
Copy link
Contributor

jtimal commented Dec 14, 2022

@arvoConsultores : I tested this today, and the basics work. However, I'm finding that both pages load very slowly if you set the pageSize > 5. When I set them both to pageSize: 10, each page takes a while to load (nearly 30 seconds for me). When I set both to pageSize: 20, I cannot load either page (it's almost like it hits an infinite loop because my Chrome window just starts using more and more memory)

I don't think this performance issue is caused by this PR. Rather, I'm suspecting the code on these pages has some sort of bug where larger page sizes result in major performance issues.

Basically, I think your PR is working properly...but the unfortunate result is that these pages do not perform properly when the pageSize configuration is changed. At least, that's what I'm seeing on my end (and I'd be interested to hear if this same behavior is seen by others or not -- as it's always possible I'm hitting a problem specific to my setup).

Hi, yes I confirm that it is cycling even when loading people to be more than 20 but I thought it was just my computer, I have tried in chrome and firefox developer.
image

@github-actions
Copy link

Hi @arvoConsultores,
Conflicts have been detected against the base branch.
Please resolve these conflicts as soon as you can. Thanks!


This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there.

@tdonohue tdonohue modified the milestones: 7.5, 7.6 Feb 21, 2023
@tdonohue
Copy link
Member

Unfortunately, while I'd like to be able to move this forward, merging this will likely result in major performance issues for any site that choses to use this feature (as setting pageSize > 5 will result in performance problems on the people/group pages, as described above).

Therefore, this is "on hold" until some solution can be found for the performance issues. Moving this to the 8.0 board, simply so that we are reminded of this issue later on.

@tdonohue tdonohue removed this from the 7.6 milestone Mar 23, 2023
@tdonohue tdonohue added work in progress needs discussion and removed 1 APPROVAL pull request only requires a single approval to merge labels Mar 23, 2023
@github-actions
Copy link

Hi @arvoConsultores,
Conflicts have been detected against the base branch.
Please resolve these conflicts as soon as you can. Thanks!

@tdonohue
Copy link
Member

Hi @arvoConsultores . If you could find time to update this PR, it's possible this will work much better now that DSpace/DSpace#9052 has been fixed. I suspect the prior issues we encountered with this PR were all related to that performance bug.

@tdonohue tdonohue self-requested a review November 16, 2023 16:19
@tdonohue tdonohue added 1 APPROVAL pull request only requires a single approval to merge and removed work in progress needs discussion labels Nov 16, 2023
@tdonohue
Copy link
Member

Strangely, after testing this again today, I've discovered that there are still performance issues when the pageSize is increased greater than 5 on these pages. It appears the slowness is somehow related to how performance checks are done (in order to display edit/delete buttons). So, it was not fixed by DSpace/DSpace#9052 as it seems to be a different issue.

To reproduce:

  • Install this PR
  • Set a pageSize of 10 for either the "Access Control" -> "People" or "Groups" page (it's reproducible on either page).
  • Open your browser DevTools on the Network tab
  • Login as an Admin
  • Visit that page where pagesize is set to 10.
  • On the Network tab (in DevTools) you'll notice that 10 requests are sent to /server/api/authz/authorizations/search/objects?feature=canDelete (one for each object displayed). The first 5-6 will return quickly. The last few may take a long time (several seconds) to respond. Until all these requests finish, nothing on the page will load.
    • This behavior is even worse when the pageSize is set to 20.
    • However, it's not noticeable when the pageSize is set to 5.

So, I'm still uncertain how to move this forward. The page loading flaw doesn't appear to have been caused by this PR. However, these pages only load in a reasonable time if the pageSize remains at 5. So, I'm still worried about merging this since it is unusable until the page can be sped up.

Copy link

Hi @arvoConsultores,
Conflicts have been detected against the base branch.
Please resolve these conflicts as soon as you can. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 APPROVAL pull request only requires a single approval to merge improvement merge conflict
Projects
Status: ❓ Stalled/On Hold
Development

Successfully merging this pull request may close these issues.

Display more than 5 e-people or groups per page
4 participants