Refactor body_does_not_contain_lead_image validator #8448
Conversation
ollietreend
force-pushed
the
refactor-lead-image-validator
branch
from
18021bc to
a3d5bb3
Compare
This commit refactors the `body_does_not_contain_lead_image` custom validation method to reduce its coupling to Govspeak. Right now it uses regular expressions to check if the lead image has been embedded into the document body. This relies upon the validator knowing the specific Govspeak syntax that would be used to embed the image (of which there are two different variants). This refactor takes a more 'black box' approach by rendering the Govspeak as HTML, and then inspecting the output to see if it contains the lead image. This way the validator doesn't need to parse the Govspeak syntax directly – making it more robust against future changes to the syntax. I imagine this new approach is slightly less performant. It now renders and inspects the resultant HTML, rather than simply matching a regex against the raw Govspeak. However I expect this will have negligible overall impact on performance, so would be a net benefit given the improvements to code readability and cleanliness.
ollietreend
force-pushed
the
refactor-lead-image-validator
branch
from
a3d5bb3 to
1a7a425
Compare
| def stubbed_image(name) | ||
| build(:image).tap do |image| | ||
| image.stubs(:filename).returns(name) | ||
| image.stubs(:url).returns("http://example.com/#{name}") |
There was a problem hiding this comment.
Do we need to stub this out? Can we not just create an image with the filename we want?
There was a problem hiding this comment.
I tried avoiding a stub in the first place, but it looks like the Image and ImageData factories have been changed with the recent work done by the Assets team.
From what I can tell, the factory can only create an SVG image or a JPEG image. And the filename isn't configurable – it seems like the JPEG is hardcoded as minister-of-funk.960x640.jpg and the SVG is test-svg.svg.
Really I want to create two 'normal' images (e.g. not SVGs, because we know they sometimes get treated differently) that have different filenames.
So I figured this stub was an easy way to avoid changing how that factory works. (Perhaps that's a bit lazy of me!?)
There was a problem hiding this comment.
Yea i've seen quite a lot of stuff that's not used the factories which probably isn't ideal (if you search for build(:image_data) you'll see what i mean. This seems fine to me though.
This commit refactors the
body_does_not_contain_lead_imagecustom validation method to reduce its coupling to Govspeak.Right now it uses regular expressions to check if the lead image has been embedded into the document body. This relies upon the validator knowing the specific Govspeak syntax that would be used to embed the image (of which there are two different variants).
This refactor takes a more 'black box' approach by rendering the Govspeak as HTML, and then inspecting the output to see if it contains the lead image. This way the validator doesn't need to parse the Govspeak syntax directly – making it more robust against future changes to the syntax.
I imagine this new approach is slightly less performant. It now renders and inspects the resultant HTML, rather than simply matching a regex against the raw Govspeak. However I expect this will have negligible overall impact on performance, so would be a net benefit given the improvements to code readability and cleanliness.
Follow these steps if you are doing a Rails upgrade.