Chongsun Ahn

Thanks for this. No need to put details about the existing domain models (program, supervisory node, etc.). Shouldn't the association model have one-to-one or one-to-many types of links in them? Th...

Thanks for this. No need to put details about the existing domain models (program, supervisory node, etc.). Shouldn't the association model have one-to-one or one-to-many types of links in them? These are relatively minor changes.

Thanks for doing this. However, I don't think it's necessary to do class diagrams for all of our "infrastructure" classes (importers, DTOs, repositories, etc.). The important ones are the domain cl...

Thanks for doing this. However, I don't think it's necessary to do class diagrams for all of our "infrastructure" classes (importers, DTOs, repositories, etc.). The important ones are the domain classes (SupplyPartner and SupplyPartnerEntry) and how they relate to our existing reference data domain classes (program, supervisory node, etc.).

I didn't want to use the term "subscription" because I didn't want us to do it the way eLMIS does; duplicating a program and copying values over. They don't really seem like subscriptions to me. If...

I didn't want to use the term "subscription" because I didn't want us to do it the way eLMIS does; duplicating a program and copying values over. They don't really seem like subscriptions to me. If you feel entry is too generic, I am open to other suggestions. Perhaps mapping? But that seems about the same to me.

Ah yes, you're correct.

Ah yes, you're correct.

Agreed.

Agreed.

I don't quite understand what needs to be reviewed here?

I don't quite understand what needs to be reviewed here?

Same for the PUT below.

Same for the PUT below.

Or the token might be invalid. Same for others below.

Or the token might be invalid. Same for others below.

Technically a 400 is a bad request, so it might not simply be the query parameters that has an issue. Same for others below.

Technically a 400 is a bad request, so it might not simply be the query parameters that has an issue. Same for others below.

Why not a page?

Why not a page?

I think we should support that there may be a supply partner, but it may at any point not have any entries. All entries might be removed while configuring, or when first created, etc.

I think we should support that there may be a supply partner, but it may at any point not have any entries. All entries might be removed while configuring, or when first created, etc.

Is there a test for update where the user sent a request, the FHIR flag is not set in the DTO, and the resource is managed externally? That seems like it could happen.

Is there a test for update where the user sent a request, the FHIR flag is not set in the DTO, and the resource is managed externally? That seems like it could happen.

From what I can tell, it is possible to create a Location resource in FHIR using a specified UUID. If there is a corresponding OpenLMIS ID from reference data, we can use that. Or if it is from an ...

From what I can tell, it is possible to create a Location resource in FHIR using a specified UUID. If there is a corresponding OpenLMIS ID from reference data, we can use that. Or if it is from an external system, the system can generate a UUID and use that to save to FHIR.

For the others, it does seem like we may need those fields as identifiers in order to save the corresponding resource in reference data.

You mean if something outside of OpenLMIS POSTs to /hapifhir/Location, which is a geo zone and needs to be created in reference data?

You mean if something outside of OpenLMIS POSTs to /hapifhir/Location, which is a geo zone and needs to be created in reference data?

I'm not sure the first check should be checking for the same value in the new and existing. I'm also not sure we should be checking for invariants based on info from the request, but rather the inf...

I'm not sure the first check should be checking for the same value in the new and existing. I'm also not sure we should be checking for invariants based on info from the request, but rather the information from the datastore (the existing resource).

We may want to allow modification of the flag if it wasn't set properly during the create, for a non-user request.

We may want to allow modification of the flag if it wasn't set properly during the create, for a non-user request.

I'm not sure if you've taken this into account, but when updating a facility or geo zone, we'll need to be "smart" about it. Example: FHIR service updates a facility. We need to ensure properties t...

I'm not sure if you've taken this into account, but when updating a facility or geo zone, we'll need to be "smart" about it. Example: FHIR service updates a facility. We need to ensure properties that FHIR doesn't care about, like supported programs, facility type, go live date, etc. are not overwritten.

I think once we rename this flag, as I mentioned in another comment to something like "isManagedExternally", then we should be fine here.

I think once we rename this flag, as I mentioned in another comment to something like "isManagedExternally", then we should be fine here.

I think the check here is supposed to be similar to the case above. We are checking if it's a user request and if the flag is set in the "new resource" (DTO). User requests should not be able to se...

I think the check here is supposed to be similar to the case above. We are checking if it's a user request and if the flag is set in the "new resource" (DTO). User requests should not be able to set the flag on an update.

I think we should be checking if the flag is set in the existing resource. For user requests, we should be ignoring the setting in the DTO.

I think we should be checking if the flag is set in the existing resource. For user requests, we should be ignoring the setting in the DTO.

I understand the meaning of "isFhirLocationOwner" to be "is this OpenLMIS system the owner of the corresponding FHIR location?" Therefore if the flag does not exist or is true, then OpenLMIS owns t...

I understand the meaning of "isFhirLocationOwner" to be "is this OpenLMIS system the owner of the corresponding FHIR location?" Therefore if the flag does not exist or is true, then OpenLMIS owns the FHIR location and should be able to update the facility info. If the flag exists and is false, then the owner of the FHIR location is external to OpenLMIS, and this facility's main info is not managed, and therefore not editable, by OpenLMIS, and we can only update some facility info.

Sorry, this doesn't seem to match what is in the AC for the ticket. I think this flag name is too confusing; this flag needs to indicate the opposite; that it is managed/owned externally. Perhaps "isManagedExternally" or something similar? If it's set and true, then only some fields are editable. If not set or false, all fields are editable.

I'm curious as well.

I'm curious as well.

I looked at the validation logic and I'm not sure it's correct. We may need to discuss.

I looked at the validation logic and I'm not sure it's correct. We may need to discuss.