CDA-88 - Implements username regex filter for /users endpoint#1614
CDA-88 - Implements username regex filter for /users endpoint#1614rma-bryson merged 9 commits intodevelopfrom
Conversation
| @OpenApiParam(allowEmptyValue = true, name = OFFICE, | ||
| description = "Show only users with active privileges in a given office." | ||
| + Controllers.OFFICE_DESCRIPTION ), | ||
| @OpenApiParam(name = USERNAME, |
There was a problem hiding this comment.
If its a regex the parameter name should be username-regex or username-like or username-mask. username-regex seems the most explicit to me.
Also, look at any other regex parameter - the swagger desc should link to the regex doc page.
There was a problem hiding this comment.
don't we consistently use "-like" for the regex fields? Probably makes sense here to do both. username and username-like, backend can easily turn the username into a regex that means "I mean this exactly."
There was a problem hiding this comment.
+1 username-like. I'd rather not have two parameters that do the same thing. If it takes a regex you can always pass in an exact match. Unless there is/was an existing parameter that we need to maintain support for.
| .where(upper(vUserGroups.DB_OFFICE_ID).eq(upper(limitOffice))) | ||
| .and(vUserGroups.IS_MEMBER.eq("T")).asField().gt(1); | ||
|
|
||
| // Note: usernameRegex is not included in the cursor; apply it if present in current request |
There was a problem hiding this comment.
huh, if it's not in the cursor, how are we paginating through it? the value of next-page is supposed to be self contained.
There was a problem hiding this comment.
Many end-points, maybe even more than half, of the end-points do not include all the query parameters in the next-page cursor. Not saying that its right not to include them all but that is where we are at right now.
There was a problem hiding this comment.
Maybe we make a list of which end-points could be fixed with regard to the next-page cursor?
There was a problem hiding this comment.
upon closer inspection, Users does include the username query param in the next-page cursor. Removed those lines of code. Also added test coverage.
There was a problem hiding this comment.
Look again. If it was in the cursor somewhere you'd be pulling it out of parts[] and your Users object would be passing it into encodeCursor. If its not in the cursor and you removed it from the query params in the paging test and your IT is still passing that tells me the IT isn't right or there aren't enough IT.
There was a problem hiding this comment.
Okay, so it includes the last username, but that is not the regex query param.
If we want it self-contained, then we should include the regex in the cursor, otherwise the regex query param needs to be included in each subsequent paging web requests.
There was a problem hiding this comment.
Seems like it belongs in the cursor rather than a query param. Updated to include it.
rma-bryson
force-pushed
the
bugfix/CDA-88-add-username-filtering-for-users
branch
2 times, most recently
from
98664d5 to
9b3fec8
Compare
| try (final Timer.Context ignored = markAndTime(GET_ALL)) { | ||
| DSLContext dsl = getDslContext(ctx); | ||
| String office = ctx.queryParam(OFFICE); | ||
| String username = ctx.queryParam(USERNAME_LIKE); |
There was a problem hiding this comment.
call the variable usernameRegex or something similar to help track what it holds
There was a problem hiding this comment.
updated to usernameRegex
MikeNeilson
left a comment
MikeNeilson
left a comment
There was a problem hiding this comment.
Good change with the new interface. Have a little nitpick on the formatting and documentation for future devs.
|
|
||
| public interface PageCursor { | ||
| void decodeCursor(String cursor, String delimiter); | ||
| String encode(Base64.Encoder encoder, String delimiter); |
There was a problem hiding this comment.
Add a new line between things -- all kinda blurred into one method on first read. Maybe a short javadoc explaining expectations for each method. Might be obvious to us but not the new reader.
There was a problem hiding this comment.
added line breaks and javadocs
…rsor does contain username.
…n class with ability to decode into class rather than String array.
rma-bryson
force-pushed
the
bugfix/CDA-88-add-username-filtering-for-users
branch
from
87089f4 to
a7aeaba
Compare
3 participants
No description provided.