Add paging to GET /reporting-orgs, and two small fixes. #58
Conversation
Even if there are no results, there is a single page of results, which is just an empty list. The 'first' link reflected this but when there were zero results, the 'last' link was listed as 'page=0', which was incorrect.
This adds paging to /reporting-orgs. Previously, the list of user<->reporting org associations was drawn from the FGA db, but that DB contained no info on the name of the org so to sort the list (to ensure deterministic paging) would have meant reading it all, and then performing a SuiteCRM lookup for each. Instead, we fetch the list of user<->org relationships from the CRM. Resolves #15
| raise ValueError("page_size must be greater than 0") | ||
|
|
||
| total_pages = -(-total_records // page_size) | ||
| total_pages = max(1, -(-total_records // page_size)) |
There was a problem hiding this comment.
I'm curious why there are the negative signs here. Not suggesting a change, just curious.
There was a problem hiding this comment.
It's a shortcut for ceiling division: the // is floor division, so by putting the negative we always get the total number of pages needed, then the 2nd minus sign flips it back to a positive number. (And because that pattern still returns 0 pages if there are no results, but we always want at least 1 page, there is that call to max).
|
|
||
| crm: SuiteCRM = context.suitecrm_client_factory.get_client() | ||
| if role_for_org is None: | ||
| context.app_logger.warning( |
There was a problem hiding this comment.
I don't think this should be a warning as it'll be part of normal operation of the CRM where there are people related to an organisation in the CRM but whom have no permissions to make changes. Perhaps this should be informational in the logs?
There was a problem hiding this comment.
Yes, that's a good point. I was not treating that case as central since it doesn't exist yet, but perhaps that is a mistake. I wonder if we could think of same way to identify the two different cases, so that we could tell whether there should be a corresponding entry in the FGA db. Because if there isn't a preexisting CRM relationship then the lack of an entry does indicate a problem with the state of the system, which one day it would be good to be alerted about. I'll change it to an info message and we can think about it.
chrisarridge
left a comment
chrisarridge
left a comment
There was a problem hiding this comment.
There is only one thing that I think could be changed - but perhaps it's fine to leave it for now. Otherwise all okay.
This PR:
page=0when there were no resultsResolves #15