Chongsun Ahn

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.

Shouldn't we have an option to build with a setting of false? That is what I would expect the setting to be if a facility is created/updated from the FHIR service. Same for the geo zone builder.

Shouldn't we have an option to build with a setting of false? That is what I would expect the setting to be if a facility is created/updated from the FHIR service. Same for the geo zone builder.

I don't think this is right; for user requests, we shouldn't be checking to see if the flags are the same, but if it's set in the request at all. This will give an error if I don't pass in "isFhirL...

I don't think this is right; for user requests, we shouldn't be checking to see if the flags are the same, but if it's set in the request at all. This will give an error if I don't pass in "isFhirLocationOwner" during an update by admin user, and that setting is set to true already.

Not sure this is correct. If the existing isFhirLocationOwner is true, then pretty much everything (except perhaps id) is editable, but if false, then some fields are not editable.

Not sure this is correct. If the existing isFhirLocationOwner is true, then pretty much everything (except perhaps id) is editable, but if false, then some fields are not editable.

We should rename currentFlag to something more descriptive about its purpose. Same thing with existingFlag below.

We should rename currentFlag to something more descriptive about its purpose. Same thing with existingFlag below.

I'm not sure this pattern (hasSameFields) is more readable than before. The name doesn't quite seem accurate; we are looking for the same fields but also the same values. And it's not immediately c...

I'm not sure this pattern (hasSameFields) is more readable than before. The name doesn't quite seem accurate; we are looking for the same fields but also the same values. And it's not immediately clear that we are ignoring the field specified in the second argument.

These changes are virtually impossible to review in-depth.

These changes are virtually impossible to review in-depth.

Since this returns a boolean, we should name it accordingly; i.e. isServiceToken. Or is it, isServiceOrApiKeyToken?

Since this returns a boolean, we should name it accordingly; i.e. isServiceToken. Or is it, isServiceOrApiKeyToken?