-
Notifications
You must be signed in to change notification settings - Fork 438
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
base: main
Are you sure you want to change the base?
Conversation
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.
It's the same as #1771 (review) |
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.
@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. |
Hi @arvoConsultores, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
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 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. |
Hi @arvoConsultores, |
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. |
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:
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. |
Hi @arvoConsultores, |
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
The default value is 5 for both.
Instructions for Reviewers
List of changes in this PR:
The value of pageSize must match the pagination pages sizes of the browseby like #1771 (review)
Checklist
yarn run lint
package.json
), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.