add missing office where clause in Clob getAll query #937
Conversation
|
Since we (probably me) got this wrong at least once there should probably be tests that go along with these changes. |
| .join(v_office).on(v_clob.OFFICE_CODE.eq(v_office.OFFICE_CODE)) | ||
| .where(JooqDao.caseInsensitiveLikeRegex(v_clob.ID,idRegex)) | ||
| .and(DSL.upper(v_clob.ID).greaterThan(clobCursor)) | ||
| .and(officeLike == null ? noCondition() : DSL.upper(v_office.OFFICE_ID).like(officeLike.toUpperCase())) |
There was a problem hiding this comment.
This should be caseInsensitiveRegexLike -right?
Paging should also order by office and clobId.
Probably need test
Is there an approved issue for these changes?
There was a problem hiding this comment.
No issue, I'll create one, good call on the regex.
There was a problem hiding this comment.
added tests, made office id support regex, ordered the page by office id and clob id.
| public Clobs getClobs(String cursor, int pageSize, String officeLike, | ||
| boolean includeValues, String idRegex) { | ||
| int total = 0; | ||
| String clobCursor = "*"; | ||
| AV_CLOB v_clob = AV_CLOB.AV_CLOB; | ||
| AV_OFFICE v_office = AV_OFFICE.AV_OFFICE; | ||
|
|
||
| Condition whereClause = JooqDao.caseInsensitiveLikeRegex(v_clob.ID, idRegex) | ||
| .and(officeLike == null ? noCondition() : JooqDao.caseInsensitiveLikeRegex(v_office.OFFICE_ID, officeLike)); |
There was a problem hiding this comment.
Didn't you add a caseInsensitiveLikeRegexNullTrue that does exactly this?
There was a problem hiding this comment.
wow, I have no recollection of writing that, but I guess I did!
| .from(v_clob) | ||
| .join(v_office).on(v_clob.OFFICE_CODE.eq(v_office.OFFICE_CODE)) | ||
| .where(whereClause) | ||
| .and(DSL.upper(v_clob.ID).greaterThan(clobCursor)) |
There was a problem hiding this comment.
This still isn't quite right. At least in my opinion. If you are ordering by office and clobId then I think the paging has to do the sameOffice/nextOffice thing. See buildPagingCondition in TimeSeriesDaoImpl for what I'm talking about
There was a problem hiding this comment.
I'm fairly certain Ryan is correct. especially if you have more than one office possibly coming through, but even just limiting to a single you want that sorting to be consistent.
There was a problem hiding this comment.
yup, thanks for catching this. Updated to match how you handled it in the projects endpoint
There was a problem hiding this comment.
Is there another commit? I don't see it in these changes
No description provided.