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

feat(location modal): "add a location" modal #136

Merged
merged 8 commits into from
Feb 26, 2016

Conversation

jniles
Copy link
Collaborator

@jniles jniles commented Feb 24, 2016

This PR adds a modal for creating locations on the fly. The modal can create countries, provinces, sectors, or villages. It also improves the translation and code of the bhLocationSelect component slightly.

UI/UX

Default Interface
modalcountrypane

Selecting a Country
modalcountrydropdown

Create a Village Pane
modalvillagepane

Creating a Village
modalvillageentry

Error State
modalerrorstate

No Data State
provincedropdownnooptions

Overview

This feature allows users to quickly create new location without having to navigate away from the present page. This is particularly useful on the patient registration page. There are four panes, allowing the user to create a new country, province, sector, or village.

The current view is stored in AppCache so that it is recalled whenever the user opens the modal again. In order to create a village, you must choose a country, province, and sector. If those do not exist, the user is alerted and he/she may choose to create the necessary dependencies on their respective panes.

Usage

If you would like to use this modal in your controller, simply import the LocationService and attach the modal() method to an ng-click directive.

Tests

A suite of end-to-end tests are found in the file test/e2e/locations/modal.spec.js. The patient registration page is used as the test bed for this feature.

Closes #100.

@jniles jniles force-pushed the modal-add-location branch 3 times, most recently from a8fc3aa to ccf72bd Compare February 25, 2016 10:06
noData;
});
function loadCountries() {
Locations.countries()
Copy link
Contributor

Choose a reason for hiding this comment

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

countries() does not express an http action, you should choose a better name as `loadCountries()orgetConuntries()``

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This API was introduced in a previous pull request that created the LocationService. I was using the same syntax as the PatientService, which allows the developer to do Patients.groups() to get the groups associated with a patient. See here.

I'm happy to update particular query if it seems better. Curious about what @sfount thinks.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we discussed this before when discussing previous APIs - in my opinion the angular service abstracts away from the actual HTTP action and does need to directly reflect this in the method name - location.countries makes it clear what will be returned by this method.

That being said I can see nothing truly wrong with either suggestion - we should choose one and try to use a similar naming strategy throughout the application.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the feedback.

I'm happy to change the LocationService API. However, since this is an API change, it will break the bhLocationSelect component and any other modules/services that use the LocationService (I'm not aware of any).

Furthermore, this change is not for code introduced in this PR (the API was developed and introduced in this commit two weeks ago), I propose that a separate pull request change the service API, update the bhLocationSelect component and any tests that are needing to be updated should be addressed then. If we try to include all these changes in this PR, the original purpose will be lost.

@jniles jniles mentioned this pull request Feb 25, 2016
13 tasks
This commit implements the scaffolding for an add location modal.  The
goal is to be able to add any type of location easily and quickly to the
bhima database using a modal interface.  There is a link between the
location select component and the modal for easy opening/closing.

TODO - make the link optional in the component options.
This commit implements INSERT/POST routes for the /locations interface
and includes (failing) integration tests for them.  The tests will fail
until #125 is addressed.
This commit changes the country schema and updates the database schema
to store only the required information about locations.

Closes #125.
This removes legacy exchange rate code.
This commit improve the location modal by allowing the current view to
be stored in appcache.  It also cleans up the HTML considerably by
employing suggestions made by @sfount.
This commit finishes the complete feature set of the "add location"
modal controller and behavior.  There should only be bug fixes for a
little while.

Functionality added:
 1. AppCache dynamically caches the view so that everytime the modal is
 opened, it opens to the previous view.

 2. The view clears previous selections when it changes to ensure that
 we get nicely rendered inputs.

 3. English translations have been added.  In a further improvement,
 both bhLocationSelect and this modal use the same keys for their
 default <select> options, stored in LocationService.messages.  This
 includes messages for no data.

 4. The modal now returns the appropriate promise to the controller.

Users can now easily add locations instead of navigating away from
previous pages and losing their work.
This commit implements e2e tests for the "add location" modal component.
These tests can be (and should be) improved over time.
This commit adds a generic error message to the form for better UX.
@jniles
Copy link
Collaborator Author

jniles commented Feb 26, 2016

This PR has been updated with feedback given from @DedrickEnc.

DedrickEnc added a commit that referenced this pull request Feb 26, 2016
feat(location modal): "add a location" modal
@DedrickEnc DedrickEnc merged commit c7aa8c6 into IMA-WorldHealth:master Feb 26, 2016
@jniles jniles deleted the modal-add-location branch February 26, 2016 11:34
jniles referenced this pull request in jniles/bhima Jan 12, 2017
This commit fixes the voucher identifiers that were broken in the update
from the previous updates.

Fixes #136.
jniles referenced this pull request in jniles/bhima Jan 14, 2017
This commit fixes the voucher identifiers that were broken in the update
from the previous updates.

Fixes #136.
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.

None yet

3 participants