fix(admin): match the list windows the auth API enforces - #199
Merged
Conversation
The API validates the window on its admin list routes: limit is 1 to 100 and offset is 0 or more. The CLI checked both flags against one range with a floor of zero, which was right for --offset and wrong for --limit, so users list --limit 0 and users list --limit 500 were sent and came back as a 400 naming neither the flag nor the bound. Each flag is checked against its own range now, and the message says which one was wrong and what it accepts. --limit 0 is an error rather than a request for nothing. The old comment called zero a legitimate answer, but the floor was really there for --offset, which the same function validated. Asking the server for zero rows and printing "No users." said there were none when the CLI had not looked, which is worse than saying the flag is out of range. org list gains --limit, --offset and --search. It sent no window at all, so once the API started defaulting to 50 it printed the first 50 organizations followed by a count of every organization, claiming rows it had never shown. It reports where the page sits now, the way users list already did, and --search matches the name and slug server-side. The window parsing and the position line move to adminShared, since two commands need both and the next list command will need them too.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follows fells-code/seamless-auth-api#299, which gave
GET /admin/usersthe same validated window the other admin collections already had.The flag bounds
The API takes a
limitof 1 to 100 and anoffsetof 0 or more. The CLI checked both flags with one function against one range, floor zero. That is right for--offset, where zero is the first page, and wrong for--limit, sousers list --limit 0andusers list --limit 500were sent and came back as a 400 naming neither the flag nor the bound.Each flag is checked against its own range now, and the message says which one was wrong and what it accepts:
--limit 0is now an error rather than a request for nothing. The old comment called zero a legitimate answer, but the floor was really there for--offset, which the same function validated. The alternative was to keep it and skip the request, and I did not, because printing "No users." when the CLI never asked says there are none. That is worse than saying the flag is out of range.org listwas silently truncatingNot in the original ask, but caused by fells-code/seamless-auth-api#288, so I would rather not leave it:
org listsent no window at all. Once the API started defaulting to 50 it printed the first 50 organizations and then"140 organizations."— a count of every organization, including the 90 it had not shown, with no flag to reach them.It now takes
--limit,--offsetand--search, and reports position the wayusers listalready did:--searchmatches name and slug server-side, using the parameter the API gained in #288.Say the word if you would rather this were its own PR and I will split it out.
Refactor
The window parsing and the position line moved to
adminShared, since two commands need both and the next list command will need them too.users listbehaviour is unchanged apart from the bounds.Not affected
sessions listcalls/sessions, the caller's own sessions, which takes no query parameters. It is the only other list command, sousersandorgare the whole surface.Verification
npm run buildclean (tsctype-checks)npm test: 1001 passing, 4 skipped (up from 998)npm run coveragepasses: 99.39 lines overall, andadminShared.ts,org.tsandusers.tsall at 100