Add API to void entries in the passed locations

[mahalakshme]

Motivation:

Frequently implementors are in need to void entries in the database using SQLs. This is prone to errors. Hence we can develop APIs that will do the task so that they need not run SQLs.

Related tickets:

https://avni.freshdesk.com/a/tickets/3343
https://avni.freshdesk.com/a/tickets/3345
https://app.zenhub.com/workspaces/avni-impl--support-5cf8e458bf08585333fd64ac/issues/gh/avniproject/apfodishanutrition/118

Reason:

In the ground, because of collaboration issues, sometimes locations get created in the wrong catchments. In this particular case, users of the right catchment with the actual location have already registered the individuals. And users(who have left the job) with the catchment with incorrect location have also registered individuals. So we need to delete the locations, remove the locations from the catchments and delete all the entries in the location. Its better to develop APIs at unit level and hence we can just do voiding all entries in the location and others can be achieved via UI.

AC:

• Add an API called /api/voidEntries with parameter that accepts an array of location UUIDs
• When the API is called void all the entries(subjects, encounters, enrolments, approval status etc.,) in that location
• Add the above and this: /subjectMigration/bulk to the API documentation
• Feel free to change the above technical implementation details without changing the overall intention.

[himeshr]

@mahalakshme Let me know your thoughts regarding the following..

Clarification needed:

• What privilege(s) should the user have to be able to invoke this operation.?
• Shouldn't this be run as a background job, as this could take a long time to complete and the connection could close before that.?
• What are the limits regarding the scale of entities that could be voided as part of single request.? In terms of addressLevels
• Should we return summary information regarding entities that were voided as part of the response.?
• Should the void of all entries happen as part of single transaction or separate transaction for each entity type.?

[mahalakshme]

@himeshr

• the user should have orgAdmin permission
• currently for the requirement for each address level the no of individuals and encounter combined is less than 500. I think we can keep it simple for now - no need to make it bg job. So may be to avoid such problems we can have the API accept just single location
• As mentioned above, for the requirement we ve now, currently the scale is low
• No need summary information - just status information should suffice since the users are going to be our implementors they can check from prod read db. If its easy, may be we can just return the count of each entity type(individual/encounter, etc.,) deleted.
• Whatever is simple - its okay if last_modified_date_time is gonna be same.

[mahalakshme]

@himeshr Moved it to 7.0

[mahalakshme]

@himeshr just reminder that we need to complete this for 7.0 release. U can move it to Ready as well, if u think there is no dependency on u.

[1t5j0y]

@petmongrels https://editor.swagger.io/?url=https://raw.githubusercontent.com/avniproject/avni-server/master/avni-server-api/src/main/resources/api/external-api.yaml shows a few errors in the swagger yaml.

Semantic error at paths./api/subjectTree.delete.requestBody
DELETE operations cannot have a requestBody.
Jump to line 1161
Semantic error at components.schemas.BulkSubjectMigrationBody.properties.subjectTypeIds
Schemas with 'type: array', require a sibling 'items: ' field
Jump to line 1672
Semantic error at components.schemas.VoidSubjectCriteriaBody.properties.addressIds
Schemas with 'type: array', require a sibling 'items: ' field
Jump to line 1678

Guess we can't do much about the first error but we can probably address the others.

[petmongrels]

fixed.

…

On Wed, Jan 24, 2024 at 5:44 PM Joy A ***@***.***> wrote: @petmongrels <https://github.com/petmongrels> https://editor.swagger.io/?url=https://raw.githubusercontent.com/avniproject/avni-server/master/avni-server-api/src/main/resources/api/external-api.yaml shows a few errors in the swagger yaml. Guess we can't do much about the first error but we can probably address the others. — Reply to this email directly, view it on GitHub <#677 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAZ3C2JR76EYSZFRNXFBNDYQD3I3AVCNFSM6AAAAABBJZGJL2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMBYGAYDSMZUHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Vivek Singh | +91 98452 32929 | @petmongrels <https://twitter.com/petmongrels>

[himeshr]

@petmongrels https://editor.swagger.io/?url=https://raw.githubusercontent.com/avniproject/avni-server/master/avni-server-api/src/main/resources/api/external-api.yaml shows a few errors in the swagger yaml.

Semantic error at paths./api/subjectTree.delete.requestBody
DELETE operations cannot have a requestBody.
Jump to line 1161
Semantic error at components.schemas.BulkSubjectMigrationBody.properties.subjectTypeIds
Schemas with 'type: array', require a sibling 'items: ' field
Jump to line 1672
Semantic error at components.schemas.VoidSubjectCriteriaBody.properties.addressIds
Schemas with 'type: array', require a sibling 'items: ' field
Jump to line 1678

Guess we can't do much about the first error but we can probably address the others.

@petmongrels would it be better to make this a POST API, with a "voidAll" keyword in the PATH.
"POST => /api/subjectTree/voidAll" instead of "DELETE => /api/subjectTree"

[mahalakshme]

moving this back since I am not doing the QA

[vinayvenu]

If this should be part of the external API, then

1. Addresses in other APIs are in the form of a hashmap. Shouldn't we use the hashmap, or the uuid/externalId?
2. Do we need to have the reason as part of the API?

[vinayvenu]

Test cases

1. Permissions
   • Should not work without permissions
   • Should not void when there are no permissions ( Failed )
   • Should not void when admin
2. Tree
   • Should void everything (individual, individual_relationship, encounter, program_enrolment, program_encounter, checklist, checklist_item, group_subject, comment, comment_thread, subject_migration, subject_program_eligibility, entity_approval_status) ( Failed )
   • Should not fail if there are no subjects
   • Should not fail when there is one bad address line ( Failed )
3. API
   • Should fail when API structure is incorrect
4. API response
   • Should provide details in response
5. Large transactions
   • Test against 1000 subjects and see what happens

Issues

Issue 1

Missing address id throws an error

Error 500
User: hasiru_ka_uat
Environment: prerelease
Request

{
    "addressIds": [2453]
}

Error: https://pastebin.com/raw/ywU5gJ31
Note: The address id exists in a different organisation. The issue seems to happen whenever the address id does not exist

Issue 2

In the above, the user did not have access to this API, but was still able to access the API. It looks like our privilege checker does not look for voided user_group entries.

Issue 3

The last_modified_date_time is not spread over time. It is the same value across all rows on the same table (see address level 11520 on staging)

Issue 4

comment_thread is not voided while its parent is

Notes

1.7 seconds on staging for 1 subject. For 145 subjects, it took 2.85 seconds. 1.45 seconds again for 1203 subjects. The response improves at log pace. Did not verify for all tables

[mahalakshme]

@petmongrels I just realised now is calculated from the clients mobile time, which might not be inline with the server's time. Hence I think using random() to update last_modified_date_time may cause issues.

[petmongrels]

I think these issues are separate from each other. We always set the timestamp to now when we update any data. For data fixes we have been updating this into the future but trying to restrict it so that it doesn't go too far into future (ideally restricting it to be within 1 second into future). This hasn't changed with random. Random ensures that too many records don't have the same timestamp - which we were not able to ensure with using id because ids can range from very small number to very large number making it difficult to keep one hardcoded denominator.

…

On Fri, Feb 23, 2024 at 9:27 AM mahalakshme ***@***.***> wrote: @petmongrels <https://github.com/petmongrels> I just realised now is calculated from the clients mobile time, which might not be inline with the server's time. Hence I think using random() to update last_modified_date_time may cause issues. — Reply to this email directly, view it on GitHub <#677 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAZ3C37MP66Y7VSPARPO7LYVAHRXAVCNFSM6AAAAABBJZGJL2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRQGY4TSNRVGY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Vivek Singh | +91 98452 32929 | @petmongrels <https://twitter.com/petmongrels>

[himeshr]

Standard use-case to void all entries corresponding to specified address_level is working as expected.
But we still have issue with Privilege Check.

Issue 1: Missing address id should throw a validation error
Status: Resolved
User: hasiru_ka_uat
Environment: prerelease
Request:

{
    "addressIds": [2453]
}

Response:

{
    "locations": {},
    "notFoundAddresses": [
        2453
    ]
}

---

Issue 2: In the above, the user did not have access to this API, but was still able to access the API. It looks like our privilege checker does not look for voided user_group entries.
Status: Unresolved

Below Sql query returns true, due to voided flags being ignored:

select bool_or(groups.has_all_privileges) from users
    left outer join user_group ug on users.id = ug.user_id
    left outer join groups on ug.group_id = groups.id
where users.id = 10260;

Its used in AccessControlService.userExistsAndHasAllPrivileges(contextUser) -> userRepository.hasAllPrivileges(contextUser.getId());

Whereas the sql query returns false, once we include voided checks:

select bool_or(groups.has_all_privileges) from users
    left outer join user_group ug on users.id = ug.user_id and not ug.is_voided
    left outer join groups on ug.group_id = groups.id and not groups.is_voided
where users.id = 10260 and not ug.is_voided;

---

Issue 3: The last_modified_date_time is not spread over time.
Status: Resolved

---

Issue 4: comment_thread is not voided while its parent is
Status: Resolved

---

[himeshr]

Issue 2: In the above, the user did not have access to this API, but was still able to access the API. It looks like our privilege checker does not look for voided user_group entries.
Status: Unresolved
Request:
{
"addressIds": [412317]
}
Response:

User doesn't have privilege of type: MultiTxEntityTypeUpdate