Make Contacts feature work and be more user friendly #130
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kevindew
force-pushed
the
extract-contacts
branch
2 times, most recently
from
f88d496 to
77c036f
Compare
kevindew
changed the title
kevindew
force-pushed
the
extract-contacts
branch
from
77c036f to
8163cc1
Compare
kevindew
changed the title
kevindew
added a commit
to alphagov/content-publisher
that referenced
this pull request
Nov 6, 2018
This should be replaced when alphagov/govspeak#130 is released
kevindew
added a commit
to alphagov/content-publisher
that referenced
this pull request
Nov 6, 2018
This should be replaced when alphagov/govspeak#130 is released
kevindew
added a commit
to alphagov/content-publisher
that referenced
this pull request
Nov 6, 2018
This should be replaced when alphagov/govspeak#130 is released
This was referenced Nov 6, 2018
benthorner
reviewed
Nov 7, 2018
| # Not showing United Kingdom country is a "feature" lifted and shifted | ||
| # from Whitehall: | ||
| # https://github.com/alphagov/whitehall/blob/c67d53d80f9856549c2da1941a10dbb9170be494/lib/address_formatter/formatter.rb#L17 | ||
| value = "" if key == "world_location" && value.strip == "United Kingdom" |
There was a problem hiding this comment.
Could this be written as Hash[address]["world_location"].strip == "United Kingdom"? The current code with the assignment and reject is a bit tricky to follow.
There was a problem hiding this comment.
I've reworked this now. Hopefully it's easier now - didn't quite understand how your suggestion could work I'm afraid.
kevindew
force-pushed
the
extract-contacts
branch
from
8163cc1 to
8e2ed29
Compare
This allows us to determine which contacts have been linked to in a document so that we can work out what data to insert when rendering govspeak. As there is legacy markdown for contacts ([Contact: 123]) this determines that the contacts are only ones that match a UUID format - this also saves client apps having to work out whether an id is valid before sending it to the Publishing API.
Prior to this PR the data for contacts inside was based on the needs of the HTML that would be rendered. This makes it rather incompatible with the contact format that is used [1]. This changes the expected data for contacts to be based on the content schema - so that a Content Item returned from Publishing API get contact (or from Content Store) can be accepted. It also drops a worldwide office link which is a tricky to implement feature that Whitehall does not seem to actually use as part of govspeak rendering: Whitehall uses this code to render Contacts: https://github.com/alphagov/whitehall/blob/58374b3ba0de07be9dfef269028e1663f4fda0f8/app/helpers/govspeak_helper.rb#L144-L148 The link is only rendered if the Contact is a WorldwideOffice: https://github.com/alphagov/whitehall/blob/235a26c29c18b081a23a075d76f0f2ef34c4e360/app/views/contacts/_contact.html.erb#L47-L49 Worldwide office can pretend to be a Contact: https://github.com/alphagov/whitehall/blob/235a26c29c18b081a23a075d76f0f2ef34c4e360/app/models/worldwide_office.rb#L18-L22 but will never be loaded from a Contact.find(x) method as per the first point. [1]: https://github.com/alphagov/govuk-content-schemas/blob/4e976c696ddd6ebfd1c4e1da52d6d5af54c79ba4/formats/contact.jsonnet
The previous method did not work when this gem was used in a ruby app as the load method was relative to the current directory. This changes this to load the files relative to the file. This also loads the locale files in when the gem starts rather than at app runtime. This is so the gem does not surprise apps by adding locale information later that can not be overriden by apps.
This is to stop them getting mixed into the translations of an application that uses locales
This creates a method on the Govspeak module that will return the root path of the Gem. This is to make it simpler to include files this gem requires. The relative loading of files prior to this meant files were only loaded when running the gems tests and not when the gem is used in an application.
This re-applies a quirk of Whitehall where United Kingdom as a country
name is not shown in any contact addresses that are shown to users as
it's considered implicit. It feels a bit weird to do this here and
potentially frustrating in some contexts but it stops us introducing a
change.
This also adds code to deal with a common problem that contacts have an
array of empty data for postal_addresses in the Publishing API
As an example (from Publishing API):
Document.find_by(content_id: "fb6777d8-1e19-427f-8dae-0ef50f0ff359").live
```
details:
{"description"=>"",
"contact_type"=>"General contact",
"title"=>"Visa Enquiries",
"contact_form_links"=>[{"title"=>"Visa Enquiries", "link"=>"", "description"=>""}],
"post_addresses"=>
[{"title"=>"",
"street_address"=>"",
"locality"=>"",
"region"=>"",
"postal_code"=>"",
"world_location"=>""}],
"email_addresses"=>
[{"title"=>"",
"email"=>"All enquiries via the UK Visas website at www.gov.uk/visas-immigration"}],
"phone_numbers"=>[{"title"=>"Visa Enquiries", "number"=>"00 44 203 481 1736"}]},
```
(No comment on that email address)
This feature didn't work when I came to use this in an application. It used the following methods: h - Rails html_escape helper, not available in a Gem format_with_html_line_breaks - Whitehall helper method, now lifted and shifted here, already does a html_escape so that is no longer needed auto_link - An old Rails helper removed in 3.1, now provided by a Gem. I chose rinku as per Whitehall
This seems to be covered with the contact is not present in options test defined above.
This wasn't being tested before
This is to stop this gem being subtly broken.
This is a bit of sad commit as I didn't really want to add this stuff in. This adds more protection against empty data from the Publishing API where it returns fields with empty strings. I'm hoping to fix most of the data Whitehall has currently but I don't think I'll fix it all so this protection will probably be needed ongoing.
kevindew
force-pushed
the
extract-contacts
branch
from
8e2ed29 to
3351b97
Compare
|
@benthorner All comments addressed now, hopefully this is good to go. |
benthorner
approved these changes
Nov 8, 2018
|
Thanks @benthorner - sure if we have a mo tomorrow lets chat. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Trello: https://trello.com/c/gEAMmD0g/386-ability-to-embed-a-contact-via-markdown
This PR is to fix issues encountered by trying to use the Contacts feature added to Govspeak in 2016. Prior to this this feature was not used in any apps (evidenced by this feature not actually working properly) thus I don't consider any changes to this to be backwards compatibility breaks.