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

Hca initial controller #588

Merged
merged 10 commits into from
Dec 2, 2016
Merged

Hca initial controller #588

merged 10 commits into from
Dec 2, 2016

Conversation

akainic
Copy link
Contributor

@akainic akainic commented Dec 1, 2016

Going to keep this on a feature branch for now, called hca-port.

Super basic controller for HCA. Included get just for the purposes of testing. Can remove before merging this if it doesn't feel useful.

@akainic
Copy link
Contributor Author

akainic commented Dec 1, 2016

Don't understand the failure :/. Doing some more digging....

screen shot 2016-12-01 at 5 41 23 pm

@@ -27,6 +27,11 @@
constraints: ->(_) { FeatureFlipper.show_education_benefit_form? }
end

resource :health_care_application, only: [:create, :index] do
post :create
get :index
Copy link
Contributor

Choose a reason for hiding this comment

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

These two route definitions are redundant with L30, the resource declaration will create these routes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I included it just because without when I run rake routes it doesn't include a route for GET, and if I try to go to http://localhost:3000/v0/health_care_application I get a 404. Wasn't sure why that was.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just figured out that's because I need to specify resources instead of just resource, but now I'm getting a Rails error (uninitialized constant V0::HealthCareApplicationController) when trying to go to that url.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you declare it as a resource, you are saying the user can only access a single health care application, so index would not be used to show all the applications, and show would be action responsible for the GET request to /v0/health_care_application. Will a veteran have multiple health care applications, or just one? What operations can they take, just creation, or can they view their application as well?
If you change it to resources, the name should become plural
resources :health_care_applications

Copy link
Contributor

@saneshark saneshark Dec 2, 2016

Choose a reason for hiding this comment

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

Everything @austin-martinez said, but you can also limit CRUD operations using only: there should be examples already in the routes file on how to do that.

But I have to agree with @austin-martinez if it is a single resource, and a vet can only ever have one (for example a user can only have 1 profile). Then you should use resource. The name will be singular, but your controller will still be pluralized. More information on this and why controller names are always pluralized, even for singular resources, here

@saneshark
Copy link
Contributor

I'm not entirely sure your branch got out of sync. It is showing some files changed that are not relevant to this PR.

@akainic
Copy link
Contributor Author

akainic commented Dec 2, 2016

@saneshark I think the extra commit got in as a result of a rebase and force push gone awry: first rebased against master as a matter of habit, but should have rebased against hca-port, so I think another commit got in that wasn't part of hca-port. But I then updated hca-port with that commit and it's still showing up... hmmm.

@saneshark
Copy link
Contributor

I have had mixed success with rebasing, especially with as many contributors as we have. What I do is commit my changes in my PR branch, switch to master (which i always keep pristine with origin), pull the changes, then switch back to my PR branch and merge my local (updated master) into the PR branch. Then when it's time to merge, i do the squash and merge.

Alternately, I will click the little button that lets you update with master when there are no conflicts, but naturally then I have to remember to pull those when adding changes locally.

@akainic
Copy link
Contributor Author

akainic commented Dec 2, 2016

@saneshark Yep, that makes sense. I ended up just creating a new local branch from hca-port, cherry-picking the commits I actually wanted, and the force pushing and that seemed to do the trick :)

Copy link
Contributor

@markolson markolson left a comment

Choose a reason for hiding this comment

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

Nice! Having this in place will help a lot as everything else gets built out. I'd just removed the index action now that you know the routes are working, as we'll only be using the create for now.

@@ -27,6 +27,8 @@
constraints: ->(_) { FeatureFlipper.show_education_benefit_form? }
end

resources :health_care_application, only: [:create, :index]
Copy link
Contributor

@saneshark saneshark Dec 2, 2016

Choose a reason for hiding this comment

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

Just wanted to make sure that this is in fact desired behavior.

A user can have more than 1 health care application. Is that correct?

Otherwise, using resource and :show actions would be the way to go (when user can only ever have one).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For right now they can only have 1 application since there is no authentication, each application is anonymous.

if health_care_application
render(json: { success: true })
else
render(json: { success: false })
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the JSON here is just for testing purposes, but I wanted to make mention that if a resource is created that does not have a real payload, then the HTTP status code 204 "No Content" with no JSON body is ideal.

for anything that is not a success, we would raise an error that application_controller.rb uses to render JSON according to how JSONAPI v1.0 spec suggests to do so.

This is just a comment for posterity sake. I realize this is just for testing purposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, once we're submitting real data that'll be good to keep in mind!

@akainic
Copy link
Contributor Author

akainic commented Dec 2, 2016

@markolson removed the index action! if you want to review again

@akainic akainic merged commit 79a27b1 into hca-port Dec 2, 2016
@akainic akainic deleted the hca-initial-controller branch December 2, 2016 21:28
patrickvinograd pushed a commit that referenced this pull request Jan 4, 2017
* Hca initial controller (#588)

* Initial controller for HCA

* Fix things, need to specify post

* Should be working now

* fix errors

* Add a little more logic

* Maybe fixed?

* Fix routes

* Change syntax of render function

* Write a few tests

* Remove index route and rename controller to be plural

* Initial HCA files/settings

* Prep the Cert store for HCA usage

* hca port Enrollment system (#599)

* add enrollment system module

* add enrollment system file

* add form template

* add has_financial_flag

* format_zipcode method

* format address spec

* format address with non american address

* use strings for form template keys

* use strings for keys

* lint

* add marital_status_to_sds_code

* lint

* use test_method helper

* spec for spanish_hispanic_to_sds_code

* phone_number_from_veteran method

* email from veteran method

* veteran_to_races method

* add validations dob

* validated string method

* validate string spec

* validate name method

* validate ssn method

* test veteran_to_spouse_info

* resource_to_income_collection method

* resource_to_expense_collection method

* child_relationship_to_sds_code method

* lint

* use constant for veteran_to_races

* lint validate_ssn

* disable module length rubocop

* child_to_dependent_info method

* child_to_dependent_financials_info method

* lint

* veteran_to_dependent_financials_collection method

* lint

* veteran_to_spouse_financials method

* provider_to_insurance_info method

* lint

* veteran_to_person_info method

* lint

* service_branch_to_sds_code method

* lint

* discharge_type_to_sds_code method

* lint

* veteran_to_military_service_info method

* lint

* veteran_to_insurance_collection method

* veteran_to_enrollment_determination_info method

* lint

* veteran_to_financials_info method

* reuse spouse financials

* lint

* lint

* relationship_to_contact_type method

* lint

* child_to_association method

* spouse method

* lint

* spouse_to_association method

* lint

* veteran_to_association_collection test

* more tests for veteran_to_association_collection

* lint

* use spec helper in enrollment spec

* use spec helper in validations spec

* veteran_to_demographics_info method

* lint

* veteran_to_summary method

* lint

* convert_hash_values method

* add hca fixtures

* fix phone_number_from_veteran

* put test veteran and result into variables

* update vets json schema

* move api_schema_matcher to spec helper

* update vets json schema

* lint

* fix tests

* Add HCA Submission service

Uses Savon to build the request from the wsdl and xml files, though
we still use Faraday for the actual submission so that we can add the
ssl certs and keys we need. TBD if including Savon is worth it, or if
we can 'just' use Gyoku for the transform.

This also refactors some existing middleware and errors that MVI was
using and turn them into what are hopefully more generic SOAP tools
we can reuse in the future.

* lint

* Add some tests for HCA healthcheck

* Merge hca-port branch and rename HCA namespace

* Update config env variables to match devops

* Add healthcheck route; fix controller naming

* SSL config devops alignment

* Avoid uninitialized constant

* Add ES to travis.yml

* Add ES to Jenkinsfile

* Rubocop

* Fix controller name/route

* Add privacyAgreementAccepted to HCA Sample

* Add a reuqest spec for ES Healthcheck

* Initial HCA conformance port

* Lint
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.

5 participants