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

Add contact details to provider #23

Merged
merged 2 commits into from Jan 9, 2019
Merged

Conversation

dankmitchell
Copy link
Contributor

Context

Providers endpoint

Changes proposed in this pull request

Add contact details i.e. address1, address2, address3, address4, postcode

Guidance to review

/api/v1/providers

Copy link
Contributor

@benilovj benilovj left a comment

Choose a reason for hiding this comment

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

This looks good for the case where the provider hasn't got any enriched contact details. When we have a contact details enrichment, are you planning to add that to this PR or in another one?

Copy link
Contributor

@benilovj benilovj left a comment

Choose a reason for hiding this comment

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

It'd be good to get another spec for the case when there's a contact enrichment.

@misaka
Copy link
Contributor

misaka commented Jan 8, 2019

Also, I noticed this isn't being used: https://github.com/devmynd/jsonb_accessor

Might be useful here, although still need to see if it'd handle the existing data (e.g. I don't think it supports nested data, do we have any?)

misaka
misaka previously requested changes Jan 8, 2019
app/serializers/provider_serializer.rb Outdated Show resolved Hide resolved
@dankmitchell dankmitchell force-pushed the provider-contact-details branch 4 times, most recently from 3996aa7 to 1a283ef Compare January 8, 2019 16:56
@dankmitchell dankmitchell dismissed stale reviews from misaka and benilovj January 9, 2019 18:41

Updated code

@dankmitchell dankmitchell merged commit 0547413 into master Jan 9, 2019
@dankmitchell dankmitchell deleted the provider-contact-details branch January 9, 2019 18:41
"postcode" => attrs["Postcode"]
}
else
attributes
Copy link
Contributor

Choose a reason for hiding this comment

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

This returns more than just the address, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

It does. It's not the tidiest, true, but the whole ordeal is a bit messy anyway. I'd be up for tidying it up in the future, I reckon we'll be revisiting this bit of code anyway.

@dankmitchell
Copy link
Contributor Author

It didn’t help that the provider attributes are a different “case” to the enrichment attributes I.e. address1 vs Address1. The enrichments definitely needs refactoring.

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

4 participants