Allow registering multiple InvoicedItemsProcessingService impls#1102
Allow registering multiple InvoicedItemsProcessingService impls#1102labkey-adam wants to merge 5 commits intodevelopfrom
Conversation
labkey-martyp
left a comment
labkey-martyp
left a comment
There was a problem hiding this comment.
Not sure if you were ready for code review on this. We'll need register calls then in the client repos?
| protected BillingTask(Factory factory, PipelineJob job) | ||
| { | ||
| super(factory, job); | ||
| _processingService = InvoicedItemsProcessingService.get(job.getContainer()); |
There was a problem hiding this comment.
I think we'll want to use EHR_BillingManager.get().getBillingContainer(getJob().getContainer());
There was a problem hiding this comment.
I think we'll want to use EHR_BillingManager.get().getBillingContainer(getJob().getContainer());
Okay, I'll make that change. Not needed in EHR_BillingController.java?
Does relying on the registration module being active in the billing container make sense to you? IMO, this is more reliable than using folder type (which the old comment had suggested).
There was a problem hiding this comment.
Yeah probably should use the billing container as described above in the controller too. I don't think it will matter on a production server, but will probably matter on our test servers.
I think using the active modules in that container makes sense for production and test. Production will probably set that module property site wide, but tests will need to set it at the container level.
There was a problem hiding this comment.
Then I should probably just move the EHR_BillingManager.get().getBillingContainer() call into get()
There was a problem hiding this comment.
...except that the interface can't see the manager. So never mind.
Yes, this PR references PRs in WNPRC and TNPRC that adjust registration. I've assigned you as reviewer to those. |
| -- ehr.Project.Created and ehr.Project.Modified are NULL on SQL Server but NOT NULL on PostgreSQL. Set the NULL values | ||
| -- and switch the columns to NOT NULL to match PostgreSQL. | ||
| UPDATE ehr.Project SET Created = diCreated WHERE Created IS NULL; | ||
| UPDATE ehr.Project SET Modified = diModified WHERE Modified IS NULL; |
There was a problem hiding this comment.
We could fall back on GETDATE() if the di columns are null. Unless we want to hard-fail the upgrade on a server that might not have values as a way of assessing real-world data.
There was a problem hiding this comment.
Good idea. I added use of your favorite function, COALESCE().
| { | ||
| return ServiceRegistry.get().getService(InvoicedItemsProcessingService.class); | ||
| // Return the service implementation based on the registering module being active in the provided container | ||
| return REGISTRATION_LIST.stream() |
There was a problem hiding this comment.
In some other EHR contexts, we use a module-dependency ordering to choose between something provided by the core EHR module and a center-specific module.
Here, I believe that the core EHR module would startup first, and therefore have register things first. Then center-specific modules would register. findFirst() means we'd resolve the core EHR version, if present. Consider switching to reverse the list or otherwise choose the other version.
There was a problem hiding this comment.
I assumed we were just registering this in the center specific modules (not in any core EHR modules) so there would be just one service registered in the billing container for each center's project. I believe that would work for the test server with multiple EHR projects.
There was a problem hiding this comment.
I assumed we were just registering this in the center specific modules (not in any core EHR modules) so there would be just one service registered in the billing container for each center's project. I believe that would work for the test server with multiple EHR projects.
I believe that's the current state in terms of what needs to be registered, but I'm advocating that we consider being consistent so we don't have to remember which one will win in scenarios like this.
There was a problem hiding this comment.
Got it. Makes sense.
There was a problem hiding this comment.
Instead of reversing the list or stream every time, registration now uses addFirst()
3 participants
Rationale
https://github.com/LabKey/kanban/issues/1577
We need to allow multiple modules to register a
InvoicedItemsProcessingServiceimpl. Provide a module at registration time and a container at retrieval time. Determine the impl to return based on active modules in the container.Related Pull Requests