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-400: add concepts data providers, add visit form #585

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
4 participants
@Gelassen
Copy link
Contributor

commented May 24, 2019

Description of what I changed

The task was to make a data provider for concepts, that can operate both REST and cache sources. Feature for concepts that was cached already implemented few years ago and the same API was used for web request (SIC! apparently that concepts feature wasn't fully correct despite it passed code review, see details in ticket comments).

I make data provider for REST call and combine it with cache. I make a visit form as PoC of concepts autopopulate feature. I extend business logic of visit form to populate with all necessary data. Altought I faced with issue to find out way to submit entered data. It should be done to make visit note business valuable.

Also I rename existing data providers from API postfix to Repository postfix as it more relevant to their current status and common naming convention.
I introduced several business layers according to clean architecture principles in addition to MVP as current state of project needs it.

Issue I worked on

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

Checklist: I completed these to help reviewers :)

  • My pull request only contains ONE single commit

  • I have added tests to cover my changes. (If you refactored
    existing code that was well tested you do not have to add tests)
    (I noticed previous contributors stops cover changes with tests. Does it link to some extra work should be done to mock some classes?)

  • All new and existing tests passed.
    (Half of the tests fails. By looking through errors like this "Reason: cannot find android.support.v4.util.LruCache" seems they reffred to work of previous contributor. I think it is a good manners to fix them anyway, but let me do PR first to submit existing work to community as I contribute spare time to this project which I might lack of it at any time)

  • My pull request is based on the latest changes of the master branch.

@codecov-io

This comment has been minimized.

Copy link

commented May 24, 2019

Codecov Report

Merging #585 into master will decrease coverage by 0.45%.
The diff coverage is 1.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #585      +/-   ##
==========================================
- Coverage   14.16%   13.71%   -0.46%     
==========================================
  Files         184      192       +8     
  Lines        8557     8838     +281     
  Branches      750      761      +11     
==========================================
  Hits         1212     1212              
- Misses       7270     7551     +281     
  Partials       75       75
Impacted Files Coverage Δ
...openmrs/mobile/utilities/ApplicationConstants.java 0% <ø> (ø) ⬆️
...ivities/visitdashboard/VisitDashboardFragment.java 0% <0%> (ø) ⬆️
...penmrs/mobile/services/ConceptDownloadService.java 0% <0%> (ø) ⬆️
...in/java/org/openmrs/mobile/api/PatientService.java 0% <0%> (ø) ⬆️
.../java/org/openmrs/mobile/api/EncounterService.java 0% <0%> (ø) ⬆️
...ies/visitdashboard/visitnote/DiagnosisAdapter.java 0% <0%> (ø)
...es/visitdashboard/visitnote/VisitNoteActivity.java 0% <0%> (ø)
...ava/org/openmrs/mobile/api/RestServiceBuilder.java 0% <0%> (ø) ⬆️
...ava/org/openmrs/mobile/usecase/PatientUseCase.java 0% <0%> (ø)
...ivities/visitdashboard/VisitDashboardActivity.java 0% <0%> (ø) ⬆️
... and 26 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 dd72f26...9a66584. Read the comment docs.

@f4ww4z
Copy link
Collaborator

left a comment

@Gelassen Thank you for the PR! Left some comments for you to address. Also, did you pull the latest changes from the master branch? The openmrs-client/release folder should be ignored.

@Gelassen actually, I want the renaming of the Repository classes in a different issue, so that this PR is specifically for the autocompletion. I've filed this issue at AC-608, please work on that one first.

Show resolved Hide resolved gradle/wrapper/gradle-wrapper.properties Outdated

@Gelassen Gelassen force-pushed the Gelassen:feature/AC-400 branch from 83c7c01 to ed55024 May 28, 2019

@Gelassen

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

@f4ww4z, please confirm PR is ready for merge. Thank you.

@f4ww4z

This comment has been minimized.

Copy link
Collaborator

commented May 28, 2019

Yes @Gelassen , I will need @deepak140596 's response for #587 .

@Gelassen

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

@f4ww4z , it seems all PRs are ready for merge, Could you please merge them?

@f4ww4z f4ww4z force-pushed the openmrs:master branch from 5222962 to 61319ec Jun 10, 2019

@dkayiwa

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

@Gelassen did you see the merge conflicts?

@f4ww4z

This comment has been minimized.

Copy link
Collaborator

commented Jun 26, 2019

@Gelassen Ping

@Gelassen

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

Was out of work for several weeks, I will be back to work next week

@f4ww4z

This comment has been minimized.

Copy link
Collaborator

commented Jul 8, 2019

@Gelassen work on #589 first before this PR. Ping me to reopen this when it's finished.

@f4ww4z f4ww4z closed this Jul 8, 2019

@Gelassen Gelassen referenced this pull request Jul 8, 2019

Merged

AC-608: rename data source to repositories #614

2 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.