Use hashes as options for govspeak extensions #89
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
This makes us resilient to hashes being passed in
Attachments and links are provided with hashes rather than links. This means we don't have to depend on classes with a matching interfaces to be created in each application that uses govspeak and instead they just need to have a hash that matches the pattern.
danielroseman
approved these changes
Sep 28, 2016
kevindew
force-pushed
the
options-as-hashes
branch
from
3755124 to
d54a058
Compare
| attr_reader :contact | ||
|
|
||
| def initialize(contact) | ||
| @contact = contact |
There was a problem hiding this comment.
to avoid all these methods wrapping around the contact hash, maybe store contact as an openstruct (forgive me, mama) and use delegate to access them?
kevindew
force-pushed
the
options-as-hashes
branch
2 times, most recently
from
d6eae51 to
31bfa8a
Compare
|
|
File paths are relative to current working directory so need context so they can work within a installed environment.
These cause confusion in markdown as this is within an inline HTML element.
This avoids empty span elements if the attributes are missing
This tests a number of scenarios that were not previously covered with unit tests and fixes issues that were caught as a result of the more thorough testing.
Previously email links were coming out in a bit of a muddle because of the new lines and other characters in the output. This was causing markdown to not render the links correctly.
This applies a more thorough a test suite for attachment and caught a variety of bugs in the process.
This resolves the issue we have where we have templates in two different places.
Outputting <span class="attachment-inline"> <a href="">test</a> </span> will output as ` test ` which is often not what an author wants. This also swaps new lines for spaces in an attachment title
kevindew
force-pushed
the
options-as-hashes
branch
from
31bfa8a to
21fa421
Compare
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.
This changes the expected input for our govspeak extensions from objects to hashes.
This makes sense as it reduces the need for applications that use govspeak to create classes that conform to interfaces defined here and since hash is the format we receive options in the publishing API we don't need to convert that to a class before passing to govspeak. It also maintains our ability to render govspeak with JSON.
I was able to change the options for links, attachments, inline attachments and contacts as we know these aren't used. However we still have inline attachment images which require a class I'm not sure if anyone uses that and if we can remove or deprecate it?
Along the way through this I found there were a number of issues with our rendering of attachments where they were getting malformed so I've put together more thorough tests for them and fixed the issues I could fix.
There are still a few issues that should be looked into when the attachments code is started to be rolled out:
/images/*.pngand I assume they need to be pointed at some hostI’ve left FIXME notes in the code for them, I don't see them as high priority as we'll probably have to resolve them as well as others once apps that use attachments start being migrated.