converted all erb templates to haml

[anthonygarvan]

Converted all erb templates to haml use the erb2haml gem. Tested as much of the app as a I am aware of:

• landing page
• proposals page
• new NCR request form, including BA80
• adding comments
• request approval page
• request approved page
• submitting help with / without bug
• dashboard

I did not find any issues.
Note I am not sure how to get to the 18f dogfooding forms, you will have to test those or let me know how to test them.

[phirefly]

I'd like to deploy this branch to dev for a bit of testing before merging this in, too. We've had some email template issues with roadie-rails previously so would like to verify a couple of things.

[cmc333333]

Thanks for taking a swing at this!

Right now, I see a lot of questionable \>s, pipes, and other idioms that don't look quite right. It also looks like the plain text emails were converted to haml, which isn't what we want, I don't think. We probably don't want the conversion tools in our stack, either.

Given that this was an automatic conversion, I'd feel a lot more comfortable with merging this in if we broke it into batches for review. What do you all think about breaking this into batches of 5-10 templates that are manually checked after conversion?

[anthonygarvan]

Sure, if you think breaking it up will make it easier to review and less risky I can definitely go down that road. I just don't want it to drag on for too long, hence my pulling-off-a-bandaid approach.

[phirefly]

I was able to deploy to dev and confirm no layout issues on the email sends.

[anthonygarvan]

So... if @phirefly agrees, next step will be to close this PR, start opening a series of ~6 PR with 5-10 manually validated files each (partitioned by folder), so that the risk is mitigated and the code reviews more manageable.

[phirefly]

Although I'm not opposed to separate PR's, separate commits in the same PR would be reasonable to me, making it easy to do a revert on each template if we run into issues.

A couple of other comments:
• I believe .text.haml files do work in rails
• Pipes are haml multi-lines, which I'm not opposed, but feel free to remove if you're inspired

@cmc333333 additional thoughts?

[cmc333333]

Separate commits would help, but I'm not seeing the urgency to get this in without reviewing the changes more carefully. This was an automated pass; it contains quirks (e.g. \>s, pipes, etc.) that wouldn't be present to this degree (perhaps at all) if the templates were reviewed manually.

• I believe .text.haml files do work in rails

Indeed, they do. But the plain text files shouldn't have markup in them; haml seems like the wrong choice, then, no?

• Pipes are haml multi-lines, which I'm not opposed, but feel free to remove if you're inspired

Right. I'm saying, if we had done the conversion by hand, they most likely wouldn't be present -- we don't have any in our existing code base. They're only here because this is machine generated with little manual clean-up. Again, why not spend the extra time to do this properly?

[cmc333333]

Here's an example; unless I misunderstand, this isn't quite right. We don't want the body of the tr to be the string class="comment-update"; we don't want the greater than symbol; we want the tds to be contained in this tr.

We want something like

%tr{class: "comment-update" if c.update_comment}
    %td
    ...

[phirefly]

Agreed. We should definitely be doing manual clean-up on things like this.

[phirefly]

Regarding the text only emails... I see what you're getting at. Yes, haml is pointless if there is no markup. We should leave erb as @cmc333333 suggested.

[anthonygarvan]

Yeah, nice catch. Looks like a known issue with html2haml which erb2haml uses under the hood.

[afeld]

Sounds like it makes sense to split this up into smaller closer-reviewed chunks – thanks for putting in all this effort though, @anthonygarvan!