Skip to content

BAH-3847: Appointments: Add REST support for 'fulfilling encounters' - #168

Draft
jnsereko wants to merge 1 commit into
Bahmni:masterfrom
jnsereko:LIME2-400
Draft

BAH-3847: Appointments: Add REST support for 'fulfilling encounters'#168
jnsereko wants to merge 1 commit into
Bahmni:masterfrom
jnsereko:LIME2-400

Conversation

@jnsereko

@jnsereko jnsereko commented Feb 27, 2025

Copy link
Copy Markdown

Description

This logic adds the ability to get and set fulfilling encounters for a specified appointment
cc @mogoodrich

Screenshot

Screenshot 2025-02-17 at 16 52 39

Ticket

https://bahmni.atlassian.net/browse/BAH-3847

Other resources

See how the form engine uses this endpoint to add appointments to their associated encounters at openmrs/openmrs-esm-form-engine-lib#471

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@pirupius pirupius left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks @jnsereko for this. Some clean up to do with indenting

private HashMap extensions;
private String teleconsultationLink;
private String priority;
private String[] fulfillingEncounters = new String[0];

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This indentation seems to be off

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think our general standard is to use a List instead of an Array (ie List), is there a reason you use an Array here?

Comment on lines +149 to +155
public String[] getFulfillingEncounters() {
return fulfillingEncounters;
}

public void setFulfillingEncounters(String[] fulfillingEncounters) {
this.fulfillingEncounters = fulfillingEncounters;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same applies to this block

@pirupius

Copy link
Copy Markdown

cc: @angshu @dkayiwa

@dkayiwa

dkayiwa commented Feb 27, 2025

Copy link
Copy Markdown

Do you wanna add some tests for this?

@mogoodrich mogoodrich left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A few comments but generally looks good to me.

private HashMap extensions;
private String teleconsultationLink;
private String priority;
private String[] fulfillingEncounters = new String[0];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think our general standard is to use a List instead of an Array (ie List), is there a reason you use an Array here?

if (appointmentRequest.getFulfillingEncounters().length > 0){
Set<Encounter> fulfillingEncounters = Arrays.stream(appointmentRequest.getFulfillingEncounters())
.map(encounterService::getEncounterByUuid)
.filter(Objects::nonNull)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there any way we can throw an error (or at least a warning) if an encounter is not found for uuid? This seems like something that shouldn't just fail silently.

@mogoodrich

Copy link
Copy Markdown
Contributor

Oh, also, I meant to add, can we add a test for this? (Hopefully there's an existing AppointmentMapper test we can just add this to)

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.

5 participants