Workflow to comment with code review checklists on PRs.

[stephaniehobson]

One-line summary

Workflow to comment with code review checklists on PRs.

Significant changes and points to review

The idea is to create a GitHub workflow which will post appropriate checklists in a comment when a pull request is opened if some file types have been added. So, these are a little general in places. But, I'm hoping they can help keep our code base consistent-ish as we have more new developers contributing.

The conditions where the checklists should post are:

• new files with type html, css, js, or ftl
• new files with the word experiment in the name
• html edits to lines with <a or <button
• js edits to lines with addEventListener and simmilar
• js edits to line that include experiment_view
• py edits which include variations = [ and the array is not empty
• if there are no checklists to add it will not post a comment

=> each condition should only trigger relevant checks

Add checklists for:

• HTML
• CSS
• JS
• Analytics
• Experiments
• Localization
• General (not in use by workflow)
• Tests (not in use by workflow)
• Media (not in use by workflow)

Did a bit of markdown cleanup too.

Issue / Bugzilla link

n/a

Testing

You can see an earlier version of the script at work on the pulls here: https://github.com/stephaniehobson/bedrock/pulls

I'll re-run with the most recent version of the script tomorrow.

[github-actions]

Thank you for your pull request. Based on the contents you may need to double-check these things:

---

📊 Analytics Checklist

Links <a href="">

• Has a data-cta-text OR data-link-text
  • If it has class mzp-c-button it is a CTA
  • If it has class mzp-c-cta-link it is a CTA
  • If there are two of the same data-cta-text include data-cta-position
  • Does not have both data-cta-text and data-link-text
• If linking to another Mozilla property include utm params
  • utm_source is 'www.firefox.com or www.mozilla.org
  • utm_medium is referral
  • if the query string is in a variable it is called params and the string includes the ?
• Download button:
  • Use the appropriate helper, don't hard code these. (download_firefox_thanks, google_play_button, apple_app_store_button)
  • include a download_location if there are multiple buttons on the page

Buttons <button type="button">

• Download button:
  • Download buttons are links, see above
• Not a download button:
  • widget_action reporting in the dataLayer

QR Codes

• Use the qr_code helper
• Use the apps store redirects if applicable - include a product and campaign

Other

• New custom events configured in GTM.
• If any of the following are used check that their custom events will be triggered:
  • Download
  • Mozilla Accounts form
  • Newsletter subscribe
  • Self-hosted videos
  • Send to Device
  • Social Share
  • VPN subscribe button
  • Widget Action

---

🧪 Experiment Checklist

If you want to suggest an improvement to this script please comment in mozmeao/springfield#396

[stevejalim]

py edits which include variations = [ and the array is not empty

I don't see this behaviour in there any more - was it dropped?

Also, I guess it'd be good to put the docs in mozmeao/platform-docs now instead

[maureenlholland]

This is all looking great! 🥇 One best practice CSS suggestion (non-blocking)

Suggested change

	- [ ] No commented out code
	- [ ] No commented out code
	- [ ] Prefer top-level classes (avoid unnecessary nesting or specificity)

[stephaniehobson]

Hm. I guess I need to break up the docs changes and put them in platform-docs but keep the workflow here and in springfield.

[janbrasna]

Would it be feasible to catch inline style attributes in HTML and JS and remind the author to check the CSP limits do not break that for them?

[janbrasna]

For some reason the example comment above shows an empty "🧪 Experiment Checklist" (the idea is for sections without any content to stay hidden right?)

[janbrasna]

Idea: if any TODO or FIXME or TKTK is present in the additions, add a checkmark to make sure it's either deliberate, or resolved?

[maureenlholland]

Good tip from Steve:
run ./manage.py l10n_update or make preflight -- --retain-db after the work is done but before you PR to ensure you're not still working with now-stale Fluent files