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

AC-480 Patient model should extend Person #586

Merged
merged 1 commit into from Jun 6, 2019
Merged

AC-480 Patient model should extend Person #586

merged 1 commit into from Jun 6, 2019

Conversation

its-snorlax
Copy link
Contributor

@its-snorlax its-snorlax commented May 26, 2019

Description of what I changed

  • Changing the relationship between Patient and Person to is a relationship instead of has a relationship.
  • Details that can be updated in a patient now can be updated through a new PatientDetails object.
  • We had a hard dependency on the backend API while updating or registering patient, It still accepts certain detail as part of person key. So we have exposed a getPerson method while making any network request.

Issue I worked on

After this PR, Patient model is extending Person. From now onwards, we have to get patient details from the Patient itself instead of from Person.

Before

 patient.getPerson().getNames();

After

 patient.getNames();

I have already run the unit test at my local machine and all are passing successfully and I also run the application on the device and it also works the expected way.

JIRA Issue: https://issues.openmrs.org/browse/AC-480

Checklist: I completed these to help reviewers :)

  • My pull request only contains ONE single commit
    (the number above, next to the 'Commits' tab is 1).
  • I have added tests to cover my changes. (If you refactored
    existing code that was well tested you do not have to add tests)
  • All new and existing tests passed.
  • My pull request is based on the latest changes of the master branch.

@codecov-io
Copy link

codecov-io commented May 26, 2019

Codecov Report

Merging #586 into master will increase coverage by 0.2%.
The diff coverage is 19.04%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #586     +/-   ##
=========================================
+ Coverage   14.16%   14.36%   +0.2%     
=========================================
  Files         184      185      +1     
  Lines        8557     8575     +18     
  Branches      750      751      +1     
=========================================
+ Hits         1212     1232     +20     
+ Misses       7270     7266      -4     
- Partials       75       77      +2
Impacted Files Coverage Δ
...ivities/addeditpatient/AddEditPatientFragment.java 0% <0%> (ø) ⬆️
...ditpatient/SimilarPatientsRecyclerViewAdapter.java 0% <0%> (ø) ⬆️
...ies/matchingpatients/MatchingPatientsFragment.java 0% <0%> (ø) ⬆️
...tientdashboard/details/PatientDetailsFragment.java 0% <0%> (ø) ⬆️
...rg/openmrs/mobile/utilities/PatientComparator.java 0% <0%> (ø) ⬆️
...in/java/org/openmrs/mobile/api/PatientService.java 0% <0%> (ø) ⬆️
...cedpatients/SyncedPatientsRecyclerViewAdapter.java 0% <0%> (ø) ⬆️
...mentrypatientlist/FormEntryPatientListAdapter.java 0% <0%> (ø) ⬆️
...c/main/java/org/openmrs/mobile/dao/PatientDAO.java 0% <0%> (ø) ⬆️
.../activevisits/ActiveVisitsRecyclerViewAdapter.java 0% <0%> (ø) ⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d23c576...84edfd9. Read the comment docs.

Copy link
Collaborator

@f4ww4z f4ww4z left a comment

Choose a reason for hiding this comment

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

@juniorjainsahab Thanks for the PR. There's no need to create PatientDetails and PatientDto class, as the Patient class contains all the necessary setters and getters to be used in other classes. Creating new classes would just increase the complexity. Please change that, and also refer to my other comments.

@its-snorlax
Copy link
Contributor Author

Creating setters for multiple fields in a class can be dangerous sometimes, One may forget to set some fields which will result in the run time failure. These things can be easily resolved at the language level on the compile time by setting up the data contract which a class needs to be initialized with. PatientDetails is one such class.

Regarding PatientDto, this class is something which is defining the structure with backend APIs, If i remove it and use Patient instead. backend server will send 400 bad request since the structure of request body has changed.

Let me know if you think otherwise and have a suggestion on those two points. Meanwhile, I'll be fixing the unused imports issue.

@f4ww4z
Copy link
Collaborator

f4ww4z commented May 28, 2019

@juniorjainsahab What you can do is make a constructor in Patient, as currently the way to create a Patient object is create an empty Patient object and calling multiple setters. See an example in the openmrs-core repo. You can edit the constructor to take in the same variables as in the PatientDetails class, so creating a Patient object with arguments defined in the constructor is the same as creating an empty object and calling setters on it. You can also overload the constructor, if necessary.

I think that PatientDto looks good to me, just add a javadoc in the class and describe how the class is used.

@its-snorlax
Copy link
Contributor Author

@f4ww4z all the changes are done.

@its-snorlax
Copy link
Contributor Author

@f4ww4z
if you have some feedback do let me know otherwise please merge it as soon as possible because it will be hard to remove merge conflicts.

Copy link
Collaborator

@f4ww4z f4ww4z left a comment

Choose a reason for hiding this comment

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

@juniorjainsahab I left additional comments.

@its-snorlax
Copy link
Contributor Author

@f4ww4z , I think there has been some change regarding play store stuff and causing both of the build services to fail. As far as I know, this is not related to my changes. Could you please verify that?

Copy link
Collaborator

@f4ww4z f4ww4z left a comment

Choose a reason for hiding this comment

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

@juniorjainsahab Yes, it's an unexpected bug when encrypting files. It should be fixed today, afterwards I'll notify you to re-trigger the ci build.

Did you see our PR tips? Make sure to follow all guidelines.

@its-snorlax
Copy link
Contributor Author

@f4ww4z I have squashed all the commits into one. Please let me know if something is required from my side.

@f4ww4z
Copy link
Collaborator

f4ww4z commented Jun 4, 2019

@juniorjainsahab The external CI error is now fixed - please pull the latest changes from master.

removing unused imports

remove patient detail

add javadocs in patient DTO

trying to retrigger appveyor

Revert "trying to retrigger appveyor"

This reverts commit c9bf0cd.

updating java docs
@its-snorlax
Copy link
Contributor Author

@f4ww4z now both the ci builds are passing. So it is now ready for merge.

@f4ww4z f4ww4z merged commit b45b731 into openmrs:master Jun 6, 2019
@its-snorlax its-snorlax deleted the patient-extend-person branch June 15, 2019 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants