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-622: Add, Edit,Delete Providers #611

Merged
merged 1 commit into from
Jul 3, 2019
Merged

Conversation

deepak140596
Copy link
Contributor

@deepak140596 deepak140596 commented Jun 23, 2019

Description of what I changed

Added Swipe Option menu to delete and edit providers.
Added FAB to add providers.
Created ProviderRepository with Callback.
Created Activity to add and edit providers.

Issue I worked on

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

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 Jun 23, 2019

Codecov Report

Merging #611 into master will decrease coverage by 0.07%.
The diff coverage is 8.94%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #611      +/-   ##
==========================================
- Coverage   14.39%   14.31%   -0.08%     
==========================================
  Files         190      194       +4     
  Lines        8823     9025     +202     
  Branches      767      784      +17     
==========================================
+ Hits         1270     1292      +22     
- Misses       7474     7654     +180     
  Partials       79       79
Impacted Files Coverage Δ
...openmrs/mobile/utilities/ApplicationConstants.java 0% <0%> (ø) ⬆️
...mobile/activities/dashboard/DashboardFragment.java 0% <0%> (ø) ⬆️
...gerdashboard/ProviderManagerDashboardActivity.java 0% <0%> (ø)
...agerdashboard/addprovider/AddProviderActivity.java 0% <0%> (ø)
...agerdashboard/addprovider/AddProviderFragment.java 0% <0%> (ø)
...agerdashboard/addprovider/AddProviderContract.java 0% <0%> (ø)
...d/ProviderManagerDashboardRecyclerViewAdapter.java 0% <0%> (ø)
...gerdashboard/ProviderManagerDashboardFragment.java 0% <0%> (ø)
...penmrs/mobile/api/retrofit/ProviderRepository.java 18.75% <2.08%> (-49.68%) ⬇️
...erdashboard/ProviderManagerDashboardPresenter.java 38.7% <7.14%> (ø)
... and 7 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 2808b87...4e30368. Read the comment docs.

@deepak140596
Copy link
Contributor Author

@f4ww4z, made the PR. Please review it.

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.

Thanks @deepak140596 , please address my comments.

image

Also, what is meant by the toast message at the above image? Can you change it to a phrase?

@deepak140596
Copy link
Contributor Author

deepak140596 commented Jun 30, 2019

@f4ww4z , now Edit , Delete and Adding Providers are working.

@deepak140596 deepak140596 changed the title [WIP] AC-622: Delete Providers AC-622: Add, Edit,Delete Providers Jun 30, 2019
@deepak140596 deepak140596 force-pushed the AC-622 branch 2 times, most recently from 099cb3e to 0789a2a Compare July 1, 2019 07:14
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.

The UI for all of this looks great! @deepak140596 do address all my comments:

  • For AddProvider screen, it needs to be following the MVP pattern still, don't put all the code into 1 big class.
  • To sort these screens out (the 'provider manager dashboard' and the 'add provider' screens), I suggest you rename the providermanager to providermanagerdashboard and all the classes inside it to follow the ProviderManagerDashboard name. Then, create the addprovider package and work on the 'add provider' screen inside it.
  • Change provider_manager in strings.xml to Provider Manager.

identifierTIL = findViewById(R.id.add_provider_identifier_til);

if (editProvider != null) {
String displayName = editProvider.getPerson().getDisplay();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you try accessing the given, middle and family names using getNames ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It always shows the size as 0. I think there is some problem in the backend where the names are store in Display and are lost in the List variable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@deepak140596 Can you investigate this and file a new issue?

String displayName = editProvider.getPerson().getDisplay();

firstNameEt.setText(displayName.substring(0, displayName.indexOf(' ')));
lastNameEt.setText(displayName.substring(displayName.lastIndexOf(' ') + 1));
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is being done here? This complexity is too high.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It extracts the first and last names of the Provider. I have broken this line into two parts.

}


Person createPerson() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you choose to create this method? You can possibly rename this to createPersonFromUserInput.

@f4ww4z
Copy link
Collaborator

f4ww4z commented Jul 2, 2019

1 more thing: make sure to add tests to the Presenter of 'add providers' screen, at least.

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.

Looks good @deepak140596 !

@f4ww4z f4ww4z merged commit 544c0c4 into openmrs:master Jul 3, 2019
@deepak140596 deepak140596 deleted the AC-622 branch July 4, 2019 13:55
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.

3 participants