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

feat: Create note endpoints in tracker [DHIS2-17579] #19430

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

enricocolasante
Copy link
Contributor

@enricocolasante enricocolasante commented Dec 10, 2024

Added endpoint to create a note for enrollments and events:

  • POST tracker/enrollments/{uid}/note
  • POST tracker/events/{uid}/note

A new service and a new store using JDBC are created.
The validation applied are:

  • Verify that event/enrollment exists
  • Verify that user has read access to event/enrollment
  • Verify that field value for the note is not empty
  • Verify that a note with the same uid does not already exist

Inconsistency between the 2 endpoints
To check that the event/enrollment exists and that the user has all the right permissions we are using services (EnrollmentService.getEnrollment(uid) and EventService.getEvent(uid)) but they do behave differently when an entity without the right permission is accessed:

  • EnrollmentService throws a 403
  • EventService throws a 404

This is part of a biggest discussion and it will not be fixed here

@enricocolasante enricocolasante force-pushed the DHIS2-17579 branch 2 times, most recently from 4b15329 to ef136c7 Compare December 10, 2024 18:19
@larshelge larshelge changed the title fix: Implement create note endpoints in tracker [DHIS2-17579] feat: Create note endpoints in tracker [DHIS2-17579] Dec 10, 2024
@enricocolasante enricocolasante force-pushed the DHIS2-17579 branch 2 times, most recently from c5f710d to 8906a9f Compare December 11, 2024 09:25
@enricocolasante enricocolasante marked this pull request as ready for review December 11, 2024 13:01
@enricocolasante enricocolasante requested a review from a team as a code owner December 11, 2024 13:01
@muilpp
Copy link
Contributor

muilpp commented Dec 11, 2024

@enricocolasante, just for me to understand, why don't we use the importer to create notes as we do for other tracker objects?

@enricocolasante
Copy link
Contributor Author

@enricocolasante, just for me to understand, why don't we use the importer to create notes as we do for other tracker objects?

We agreed with frontend that would make sense to have an easier way for them to create a single note.
Using the importer for that adds a lot of unnecessary data that they should pass together with the note as we do not allow patch, so they would need to send a payload with the whole enrollment/event just to add a note.
And as well, it is not so good for performance, because we need to validate all that unnecessary data anyway

@@ -132,7 +132,6 @@ class JdbcEventStore {
n.created as note_created,\
n.creator as note_creator,\
n.uid as note_uid,\
n.lastupdated as note_lastupdated,\
Copy link
Contributor

Choose a reason for hiding this comment

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

Are notes immutable? Is that why this is done?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, they are immutable

@enricocolasante enricocolasante enabled auto-merge (squash) December 11, 2024 20:45
Copy link

sonarcloud bot commented Dec 11, 2024

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.

3 participants