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

Add endpoint for getting disabled user list #39756

Merged
merged 5 commits into from
Oct 10, 2023

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented Aug 8, 2023

  • Resolves: #

Summary

Adds an endpoint to get the list of disabled users, with offset and limit.
This avoids fetching all the users when opening the disabled user list, fixing the disabled user list for LDAP backed installations which do not have a lot of disabled users. Also avoids spamming the LDAP server when opening disabled user list.

TODO

Checklist

@come-nc come-nc added the 2. developing Work in progress label Aug 8, 2023
@come-nc come-nc added this to the Nextcloud 28 milestone Aug 8, 2023
@come-nc come-nc self-assigned this Aug 8, 2023
@provokateurin
Copy link
Member

There is a new CI workflow that checks that the OpenAPI specs are up-to-date. Please rebase onto master and regenerate the specs so that the workflow can do it's job.

@come-nc come-nc force-pushed the enh/add-disabled-users-endpoint branch 2 times, most recently from 0833802 to fa1f37c Compare August 28, 2023 13:52
@come-nc come-nc requested a review from provokateurin August 28, 2023 14:15
Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

API LGTM

@Pytal Pytal force-pushed the enh/add-disabled-users-endpoint branch from 303c153 to 4d63760 Compare September 9, 2023 00:13
@come-nc come-nc force-pushed the enh/add-disabled-users-endpoint branch 2 times, most recently from bc0fcbe to 93722e7 Compare September 18, 2023 13:33
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc force-pushed the enh/add-disabled-users-endpoint branch from 93722e7 to 61da2b9 Compare October 9, 2023 10:04
@come-nc come-nc added enhancement 3. to review Waiting for reviews feature: users and groups and removed 2. developing Work in progress labels Oct 9, 2023
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc requested review from a team, icewind1991, nfebe, sorbaugh and Altahrim and removed request for a team October 9, 2023 12:52
Copy link
Contributor

@nfebe nfebe left a comment

Choose a reason for hiding this comment

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

A default value for limit seems reasonable of if null, we should check for valid values different from zero... Such as negative numbers and 0

    if ($limit !== null && $limit < 0) {
    // Whatever exception
        throw new InvalidArgumentException("Invalid limit value: $limit");
    }

    if ($offset < 0) {
        throw new InvalidArgumentException("Invalid offset value: $offset");
    }

Generally, this is a little slim or error handling.

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Copy link
Contributor Author

come-nc commented Oct 10, 2023

A default value for limit seems reasonable of if null, we should check for valid values different from zero... Such as negative numbers and 0

    if ($limit !== null && $limit < 0) {
    // Whatever exception
        throw new InvalidArgumentException("Invalid limit value: $limit");
    }

    if ($offset < 0) {
        throw new InvalidArgumentException("Invalid offset value: $offset");
    }

Generally, this is a little slim or error handling.

Nice input, I’ve added that to the controller method.
For the internal method I consider the caller knows what he’s doing, but for the controller it makes sense to block invalid values.

@come-nc come-nc enabled auto-merge October 10, 2023 09:04
@come-nc come-nc disabled auto-merge October 10, 2023 11:33
@come-nc come-nc merged commit 43971f6 into master Oct 10, 2023
38 checks passed
@come-nc come-nc deleted the enh/add-disabled-users-endpoint branch October 10, 2023 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants