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-810 Allergy can be deleted in online/offline mode #793

Merged
merged 4 commits into from
Aug 3, 2020

Conversation

rishabh-997
Copy link
Collaborator

Description of what I changed

Issue I worked on

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

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-commenter
Copy link

codecov-commenter commented Aug 1, 2020

Codecov Report

Merging #793 into master will decrease coverage by 0.10%.
The diff coverage is 1.19%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #793      +/-   ##
==========================================
- Coverage   13.87%   13.76%   -0.11%     
==========================================
  Files         227      228       +1     
  Lines        9234     9312      +78     
  Branches      890      893       +3     
==========================================
+ Hits         1281     1282       +1     
- Misses       7854     7931      +77     
  Partials       99       99              
Impacted Files Coverage Δ
...ard/allergy/PatientAllergyRecyclerViewAdapter.java 0.00% <0.00%> (ø)
...oard/allergy/PatientDashboardAllergyPresenter.java 36.66% <0.00%> (-15.72%) ⬇️
...enmrs/mobile/api/repository/AllergyRepository.java 40.00% <0.00%> (-34.08%) ⬇️
...obile/api/workers/allergy/DeleteAllergyWorker.java 0.00% <0.00%> (ø)
.../org/openmrs/mobile/databases/AppDatabaseHelper.kt 0.00% <0.00%> (ø)
...openmrs/mobile/databases/entities/AllergyEntity.kt 0.00% <0.00%> (ø)
...g/openmrs/mobile/utilities/ApplicationConstants.kt 10.00% <ø> (ø)
...tientdashboard/allergy/PatientAllergyFragment.java 2.32% <5.88%> (-1.25%) ⬇️
...n/java/org/openmrs/mobile/application/OpenMRS.java 0.00% <0.00%> (ø)
...erdashboard/ProviderManagerDashboardPresenter.java 41.37% <0.00%> (ø)
... and 2 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...ea2aef6. Read the comment docs.

});
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@rishabh-997 this file needs formatting maybe !

}

@Override
public void onFailure(Call<ResponseBody> call, Throwable t) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@rishabh-997 instead of using customAPI callback use default response callback and try to use toast utils in the presenter by using these callbacks

Copy link
Collaborator Author

@rishabh-997 rishabh-997 Aug 2, 2020

Choose a reason for hiding this comment

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

Does that mean I need to create another Listener extending DefaultResponseCallback ??? Because I need to send ToastType also as three different Toasts are being used here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@rishabh-997 Maybe we can have an additional method other than OnSuccess and On failure just extend from default response callback and add the additional method for notify does it sounds good ?

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.

@rishabh-997 See my comments. Also the dialog's button text color should be white.

image

@rishabh-997
Copy link
Collaborator Author

rishabh-997 commented Aug 3, 2020

Also the dialog's button text color should be white

I think this needs a separate issue on JIRA as there are other instances in register patient activity where the same problem occurs in night mode, so I will create a separate issue regarding this and update all occurrence of this error.

@f4ww4z
Copy link
Collaborator

f4ww4z commented Aug 3, 2020

Also the dialog's button text color should be white

I think this needs a separate issue on JIRA as there are other instances in register patient activity where the same problem occurs in night mode, so I will create a separate issue regarding this and update all occurrence of this error.

I agree, this occurs on almost all alert dialogs in dark mode.

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.

@f4ww4z f4ww4z merged commit 8bc51d6 into openmrs:master Aug 3, 2020
@rishabh-997 rishabh-997 deleted the AC-810-delAllergy branch August 19, 2020 18:41
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.

4 participants