Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Clean up completed data when status is not completed [DHIS2-16122] #18539

Merged
merged 4 commits into from
Sep 6, 2024

Conversation

enricocolasante
Copy link
Contributor

@enricocolasante enricocolasante commented Sep 5, 2024

completedAt and completedBy fields were correctly populated when an event transitioned to COMPLETED status but they were not cleaned when an event transitioned from COMPLETED to ACTIVE. (An event can only transition from COMPLETE to another status accepting data values as implemented in StatusUpdateValidator)

@enricocolasante enricocolasante marked this pull request as ready for review September 6, 2024 06:30
Copy link
Contributor

@teleivo teleivo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we not yet have a test class where these can go? I am just surprised.

Looking at EventTrackerConverterService it is mapping Events. To me the permutations you test here should and can be unit tested. This logic does not benefit from these time consuming tests 😅 Having one/two that go all the way is ok but we should already have those 😅


assertNoErrors(importReport);

Event event = eventService.getEvent(UID.of("D9PbzJY8bJO"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you get the id from the trackerObjects.getEvents().get(0) to make sure/show that the event that was completed is the one you assert on?

}

@Test
void shouldDeleteCompletedDataWhenUpdatingAnEventWithStatusActive()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should merge this with the first test as you should assert that completed fields are set before than setting status to active and asserting they are not.

@enricocolasante
Copy link
Contributor Author

Do we not yet have a test class where these can go? I am just surprised.

Looking at EventTrackerConverterService it is mapping Events. To me the permutations you test here should and can be unit tested. This logic does not benefit from these time consuming tests 😅 Having one/two that go all the way is ok but we should already have those 😅

I added the unit tests for this change.
Our integration tests are not well organized, I couldn't find a test just for importing events.
As you were saying, I think that we should have a couple of import tests that go all the way for all the entity types and then move most of the integration tests to unit tests https://dhis2.atlassian.net/browse/DHIS2-16554

Copy link

sonarqubecloud bot commented Sep 6, 2024

@enricocolasante enricocolasante merged commit 05e9c80 into master Sep 6, 2024
15 checks passed
@enricocolasante enricocolasante deleted the DHIS2-16122 branch September 6, 2024 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants