AC-822 added repository layer for lastViewedPatient package by dino-saurabh · Pull Request #791 · openmrs/openmrs-contrib-android-client

dino-saurabh · 2020-07-31T21:59:21Z

Description of what I changed

added repository layer for lastViewedPatient package
added necessary changes to tests
Added custom callback

Issue I worked on

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

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.

1. added repository layer for lastViewedPatient package 2. added necessary changes to tests 3. Added custom callback

codecov-commenter · 2020-07-31T22:12:10Z

Codecov Report

Merging #791 into master will increase coverage by 0.14%.
The diff coverage is 37.50%.

@@            Coverage Diff             @@
##           master     #791      +/-   ##
==========================================
+ Coverage   13.87%   14.01%   +0.14%     
==========================================
  Files         227      227              
  Lines        9234     9254      +20     
  Branches      890      893       +3     
==========================================
+ Hits         1281     1297      +16     
- Misses       7854     7855       +1     
- Partials       99      102       +3

Impacted Files	Coverage Δ
...lastviewedpatients/LastViewedPatientsActivity.java	`0.00% <0.00%> (ø)`
.../providerdashboard/ProviderDashboardPresenter.java	`37.03% <ø> (ø)`
...erdashboard/ProviderManagerDashboardPresenter.java	`41.37% <ø> (ø)`
...nmrs/mobile/api/repository/ProviderRepository.java	`33.59% <ø> (+0.26%)`	⬆️
...mobile/api/workers/provider/AddProviderWorker.java	`0.00% <ø> (ø)`
...ile/api/workers/provider/DeleteProviderWorker.java	`0.00% <ø> (ø)`
...ile/api/workers/provider/UpdateProviderWorker.java	`0.00% <ø> (ø)`
...g/openmrs/mobile/utilities/ApplicationConstants.kt	`10.00% <ø> (ø)`
...enmrs/mobile/api/repository/PatientRepository.java	`20.53% <29.72%> (+7.64%)`	⬆️
...astviewedpatients/LastViewedPatientsPresenter.java	`57.75% <60.71%> (-0.65%)`	⬇️
... and 3 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 9ae672a...a7fe9d0. Read the comment docs.

dino-saurabh · 2020-07-31T22:14:30Z

@f4ww4z @rishabh-997 please review !!

f4ww4z

@LuGO0 See my comments.

openmrs-client/src/main/java/org/openmrs/mobile/api/repository/PatientRepository.java

f4ww4z · 2020-08-01T15:31:50Z

openmrs-client/src/main/java/org/openmrs/mobile/api/repository/PatientRepository.java

+                                    callbackListener.onResponse();
                                }
+                            } else {
+                                ToastUtil.error("Patient[" + patient.getId() + "] cannot be synced due to server error" + response.message());


Any error should be caught by the presenter and logged there instead of here. Then use strings.xml for this (since you need Context)

@f4ww4zSir these violations are from matching patients presenter should we keep them to a different PR (for issue AC-826 ) since this one was for lastViewedPatient package?
these came in for review since I just reformatted the code and these were out of formatting standards .

@f4ww4z Sir, you want these changes in this PR i will push them!! 👍 .

@LuGO0 I see, nevermind, do it for AC-826

f4ww4z · 2020-08-01T15:32:25Z

openmrs-client/src/main/java/org/openmrs/mobile/api/repository/PatientRepository.java

+                        @Override
+                        public void onFailure(@NonNull Call<PatientDto> call, @NonNull Throwable t) {
+                            //string resource added "patient_cannot_be_synced_due_to_request_error_message"
+                            ToastUtil.notify("Patient[ " + patient.getId() + "] cannot be synced due to request error " + t.toString());


See previous comment. You should call ToastUtil in the presenter.

f4ww4z · 2020-08-01T15:32:43Z

openmrs-client/src/main/java/org/openmrs/mobile/api/repository/PatientRepository.java

                        //added string resource "patient_update_unsuccessful_server_error"
                        ToastUtil.error(
-                                "Patient " + patient.getPerson().getName().getNameString() + " cannot be updated due to server error will retry sync " + response.message());
+                            "Patient " + patient.getPerson().getName().getNameString() + " cannot be updated due to server error will retry sync " + response.message());


See above comment.

f4ww4z

@LuGO0 Sorry I didn't realize they were only formatting changes. Address my comments for AC-826. Also push your changes for the

Use the patientDao class variable instead of creating a new one

comment.

openmrs-client/src/main/java/org/openmrs/mobile/api/repository/PatientRepository.java

dino-saurabh · 2020-08-02T15:54:34Z

@f4ww4z I have those changes locally I forgot to push . I will push them tonight 👍

dino-saurabh · 2020-08-02T19:27:20Z

@f4ww4z Sir pushed changes please review !!

f4ww4z

LGTM @LuGO0 !

dino-saurabh added 2 commits August 1, 2020 03:26

AC-822 added repository layer for lastViewedPatient package

25dae7b

1. added repository layer for lastViewedPatient package 2. added necessary changes to tests 3. Added custom callback

AC-822 fixed tests for the presenter

cab8d83

f4ww4z reviewed Aug 1, 2020

View reviewed changes

f4ww4z reviewed Aug 2, 2020

View reviewed changes

openmrs-client/src/main/java/org/openmrs/mobile/api/repository/PatientRepository.java Outdated Show resolved Hide resolved

AC-822 added requested changes

a7fe9d0

dino-saurabh requested a review from f4ww4z August 3, 2020 13:42

f4ww4z approved these changes Aug 3, 2020

View reviewed changes

f4ww4z merged commit 378b807 into openmrs:master Aug 3, 2020

Conversation

dino-saurabh commented Jul 31, 2020

Description of what I changed

Issue I worked on

Checklist: I completed these to help reviewers :)

Uh oh!

codecov-commenter commented Jul 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dino-saurabh commented Jul 31, 2020

Uh oh!

f4ww4z left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

f4ww4z Aug 1, 2020

Choose a reason for hiding this comment

Uh oh!

dino-saurabh Aug 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dino-saurabh Aug 1, 2020

Choose a reason for hiding this comment

Uh oh!

f4ww4z Aug 2, 2020

Choose a reason for hiding this comment

Uh oh!

f4ww4z Aug 1, 2020

Choose a reason for hiding this comment

Uh oh!

f4ww4z Aug 1, 2020

Choose a reason for hiding this comment

Uh oh!

f4ww4z left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dino-saurabh commented Aug 2, 2020

Uh oh!

dino-saurabh commented Aug 2, 2020

Uh oh!

f4ww4z left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented Jul 31, 2020 •

edited

Loading

dino-saurabh Aug 1, 2020 •

edited

Loading