dspace-angular icon indicating copy to clipboard operation
dspace-angular copied to clipboard

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

Open arvoConsultores opened this issue 3 years ago • 9 comments

References

  • Fixes #1764

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 https://github.com/DSpace/dspace-angular/pull/1771#pullrequestreview-1093428318

Checklist

  • [x] 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.
  • [x] My PR passes TSLint validation using yarn run lint
  • [x] My PR doesn't introduce circular dependencies
  • [x] My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
  • [x] My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
  • [x] 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.

arvoConsultores avatar Nov 07 '22 12:11 arvoConsultores

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

jtimal avatar Dec 11 '22 00:12 jtimal

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.

https://github.com/DSpace/dspace-angular/blob/a5bc6c1bf7cc0b9edc5da4736bb3694ae99b5edd/src/app/shared/pagination/pagination-component-options.model.ts#L25

It's the same as https://github.com/DSpace/dspace-angular/pull/1771#pullrequestreview-1093428318

arvoConsultores avatar Dec 12 '22 08:12 arvoConsultores

@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

jtimal avatar Dec 14 '22 21:12 jtimal

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.

github-actions[bot] avatar Jan 18 '23 17:01 github-actions[bot]

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 avatar Mar 23 '23 19:03 tdonohue

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

github-actions[bot] avatar May 18 '23 13:05 github-actions[bot]

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

tdonohue avatar Nov 15 '23 22:11 tdonohue

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 https://github.com/DSpace/DSpace/issues/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.

tdonohue avatar Jan 22 '24 22:01 tdonohue

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

github-actions[bot] avatar Feb 15 '24 16:02 github-actions[bot]