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

667 new service fields #676

Merged
merged 1 commit into from
Nov 11, 2014
Merged

667 new service fields #676

merged 1 commit into from
Nov 11, 2014

Conversation

anselmbradford
Copy link
Member

@monfresh Ready for review and merge. Thanks! I moved the status visibility part to #675 so that the new fields can be added independent of that feature.

@anselmbradford
Copy link
Member Author

Fixes #667

@monfresh
Copy link
Member

monfresh commented Nov 7, 2014

Will review this later, but wanted to ask if the following additional service fields would be part of this PR:

  • regular_schedules
  • holiday_schedules
  • contacts (including their phones)
  • phones (coming soon)

The example location has all of these service attributes for the "Passport Photos" service. Remember to test against values returned by the API:
https://ohana-api-demo.herokuapp.com/api/locations/22

@anselmbradford
Copy link
Member Author

I'd say wrap this one up and put those additional fields in another two PRs. Hours for instance can go into #671.

@anselmbradford
Copy link
Member Author

@monfresh How's this one look?

it 'includes defunct status notice' do
visit('/locations/fake-org/location-with-no-phone')
element = '.services-box .status'
expect(find(element)).to have_content('service is no longer operating')
Copy link
Member

Choose a reason for hiding this comment

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

It's better to point to the localization instead of hardcoding the string.

expect(find(element)).to have_content(I18n.t('service_fields.status_defunct'))

Also, I would add another test for the inactive scenario.

@anselmbradford
Copy link
Member Author

@monfresh Edit added!

@monfresh
Copy link
Member

Thanks. I think it's good to rebase and merge.

@anselmbradford anselmbradford force-pushed the 667-new-service-fields branch 12 times, most recently from 4051366 to c1a1d17 Compare November 11, 2014 03:35
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling c1a1d17 on 667-new-service-fields into b3f44d9 on master.

@anselmbradford anselmbradford force-pushed the 667-new-service-fields branch 2 times, most recently from 6275427 to e9b00b4 Compare November 11, 2014 03:39
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 6275427 on 667-new-service-fields into b3f44d9 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 7c6d55b on 667-new-service-fields into b3f44d9 on master.

@anselmbradford anselmbradford force-pushed the 667-new-service-fields branch 2 times, most recently from 2bac7bb to d657ad7 Compare November 11, 2014 03:46
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling d657ad7 on 667-new-service-fields into b3f44d9 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 4c43e7d on 667-new-service-fields into b3f44d9 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 4c43e7d on 667-new-service-fields into b3f44d9 on master.

- In order to organize the location details spec better, this commit
moves the location services tests to their own spec file.
- Orders service tests to fit the order shown in the detail view (with
the exception of the hours, which will be handled separately in
#671)
- Updates service_areas service partial name to be in the format field
name prefixed by `service_`, which follows the format of the other
partials. The file was named `service_areas`, which implies the field
is `areas`, when in fact, it is `service_areas`.
- Removes optional requirement from `how_to_apply` service field.
- Edits `name` and adds `alternate_name` service field and tests.
Removes optional check for the required `name` field.
- Adds test for service description field
and updates specificity of description test.
- Makes location description test check the description selector contents
as opposed to the whole page, since the checked content “Lorem Ipsum”
appears in the service description fields as well.
- Adds `email` service field and test.
- Renames `urls` service field to `website`, since a service now only has
one website. Also adds a test.
- Adds a container and styles for the parent element around `website` and
`email` service fields.
-Adds `accepted_payments` service field, locale, and test.
-Adds `required_documents` service field, locale, and test.
- Adds `status` service field and test.
- Renames name partial container and CSS from `title` to `name-box`.
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 52df0b7 on 667-new-service-fields into b3f44d9 on master.

anselmbradford added a commit that referenced this pull request Nov 11, 2014
@anselmbradford anselmbradford merged commit d83fe58 into master Nov 11, 2014
@anselmbradford anselmbradford deleted the 667-new-service-fields branch November 11, 2014 04:02
@anselmbradford
Copy link
Member Author

@monfresh In https://ohana-api-test.herokuapp.com/api/locations/example-location the required_documents array is empty, but it isn't in sample.json in the Ohana API repo. Is this intentional? The tests pass because they're working off a cassette.

@monfresh
Copy link
Member

That wasn't intentional. It's been fixed, but note that sample.txt is no longer the canonical source of the data since it will be removed once the CSV-based script is written.

https://github.com/codeforamerica/ohana-api/blob/master/CONTRIBUTING.md#updating-the-sample-data

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