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

RA-1855, update and delete relationships #101

Merged
merged 1 commit into from Jan 26, 2021
Merged

Conversation

cioan
Copy link
Member

@cioan cioan commented Jan 25, 2021

Hi @jwamalwa , here is my attempt on fixing this bug with the relationships controller. The code probably needs a bit more cleanup, but I was able to delete and update existing relationships on my local environment.

@HerbertYiga
Copy link

@cioan well done on this you could also add the ticket link on your pr description ,go on to claim the ticket too

@jwnasambu
Copy link
Contributor

jwnasambu commented Jan 25, 2021

@HerbertYiga, The ticket link is Hyperlinked to RA-1855 and when you click or tap on it you will be directed to the issue worked on.

@jwnasambu
Copy link
Contributor

jwnasambu commented Jan 25, 2021

@cioan I have used your commit for testing since it has a single commits for both RA-1717, and 1855 and everything works as expected.

Copy link
Member

@sherrif10 sherrif10 left a comment

Choose a reason for hiding this comment

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

for (Relationship existingRelationship : existingRelationships) {
personService.voidRelationship(existingRelationship, "deleted via the Registration widget");
}
}
Copy link
Member

Choose a reason for hiding this comment

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

My apologies, as I'm stepping into something I haven't looked at in quite some time, but doesn't the relatioship widget allow you to customize the relationships you display to certain types? If so, we need to make sure to apply the same logic when deleting (ie, don't delete types that you can't enter/view via the widget)

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so, @mogoodrich , it shows all the patient relationships:
https://github.com/openmrs/openmrs-module-registrationapp/blob/master/omod/src/main/webapp/resources/scripts/field/personRelationship.js#L12

This is an old person relationships widget that PIH is not using.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me, thanks @cioan !

Copy link
Member

@mogoodrich mogoodrich left a comment

Choose a reason for hiding this comment

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

LGTM!

@cioan
Copy link
Member Author

cioan commented Jan 26, 2021

Thanks @mogoodrich !

@cioan cioan merged commit b8854b0 into openmrs:master Jan 26, 2021
@cioan cioan deleted the RA-1855 branch January 26, 2021 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants