Add recruitment banners #14
Conversation
1e42344 to
54d56db
Compare
There was a problem hiding this comment.
I think this is a good start. I think the idea of validating the config, but how and where that happens seems to jump about a bit in the commit history.
It would be good if the validation also output what is missing.
I'm also unsure of the structure of the classes and the need for an ApplicationHelper.
I've added some other comments inline.
74c4a47 to
77338cc
Compare
a5f2e4c to
3a233fa
Compare
There was a problem hiding this comment.
This is looking cool! I've made a couple of comments but the thing I can't quite wrap my head around is the number of files included for the dummy application - do we need them all? eg spec/dummy/lib/assets/.keep, spec/dummy/config/cable.yml, favicons etc. Is all that needed?
| add_error(banner, "is missing any page_paths") unless banner.page_paths.present? | ||
|
|
||
| (banner.page_paths || []).each do |path| | ||
| add_error(banner, "page_path #{path} should start with a /") unless path.start_with?("/") |
There was a problem hiding this comment.
Wondering if we should check if it's a valid gov.uk URL?
There was a problem hiding this comment.
Hmm, maybe! But how could we check?
There was a problem hiding this comment.
Hmmm. I thought it was possible to check content-store for draft content, but that doesn't seem to be the case. Well, it isn't via gds-api-adaptors.
Does it matter if the page doesn't exist on GOV.UK? The banner won't be shown. It might be difficult to spot a typo, but that would true for an error message too.
I guess it would be annoying for people to have to manually check. So perhaps at the most it could be a warning message, but I'm not convinced that the effort is worth it.
There was a problem hiding this comment.
Why would we need to check content-store for draft content? I was thinking we'd just do Services.content_store.content_item("/base_path") and raise an error if it returns anything except a 200. Or am I missing something and it's problematic to do that for some reason?
If you both prefer not to, it's not a blocker from my pov. I'm just thinking it's a pain if we release the gem with a typo. Presume the workflow would be make a branch of the gem, and point frontend apps at it locally to confirm it looks ok, and then open a PR for the branch of the gem.
But if we have lots of simultaneous banner requests, that workflow becomes time consuming, and this one extra validation feels like it completes the set nicely, so that you could almost just merge the gem bump in confidence that the banners will show.
There was a problem hiding this comment.
Oh, checking for draft content pages is for pages that will have a banner but aren't live yet. For example if the banner and the page are going live at the same time.
But your point about releasing the gem is a good one. It is much more hassle to release a gem than it is to deploy an application to correct a typo.
| add_error(banner, "page_path #{path} should start with a /") unless path.start_with?("/") | ||
| end | ||
|
|
||
| add_error(banner, "expired over a week ago") unless (banner.end_date + 1.week) >= Time.now |
There was a problem hiding this comment.
I wonder about this. So if there are expired banners more than a week old listed in the config I can't add a new banner until I remove the cruft, because the config fails the validation.
Do we need to explain this in the readme - as it might not be clear to the uninitiated that deleting someone else's expired banner config has no consequence on the website.
Also why a week? Might be less brain melting if it was just
add_error(banner, "has expired. Remove this banner's config.") unless banner.active?
sort of a thing
There was a problem hiding this comment.
Hahaha - it was originally just "is it expired?" and Leena asked me to give some grace. It is in the readme, but perhaps not as strongly as it should be.
There was a problem hiding this comment.
It was based the conversation I heard in your standup @hannako about whether a banner should be extended and the department being slow to reply. I didn't want people to be blocked by an unrelated banner.
There was a problem hiding this comment.
Not sure if an expired banner should block adding another one (only if they are on the same page) since there is a lot of involvement from other people from other departments, organisations.
Monitoring the status of banners it's a task in itself and maybe it should be done outside these validations. Like having a slack bot that notifies the team what is live and when it should be removed?
There was a problem hiding this comment.
Yeah it's a good point. Maybe we should remove this check from the validation altogether.
It's nice that the validation helps to keep the config file neat and tidy, but being forced to sort out an unrelated banner to get an urgent one up is maybe more of a pain than having a config file with expired banners.
There was a problem hiding this comment.
So: I've resolved this by making this a warning - if the config contains expired banners you'll see it as a warning, but it won't stop the test suite from completing / CI from passing.
There was a problem hiding this comment.
(and since it's a warning, I've remove the 1 week grace period)
It might not be. Basically, that's the default dummy app for engines, but I can try to pare it down a bit. I think it's not included in the gem, so it doesn't affect final gem size, but we could probably reduce the number of files to deal with. |
a1f58b4 to
2e3ad7b
Compare
| allow(YAML).to receive(:load_file).with(original_path).and_return(replacement_file) | ||
| end | ||
|
|
||
| context "GET banner-ready-page" do |
There was a problem hiding this comment.
What is "banner-ready-page"? Should this be "GET show"?
There was a problem hiding this comment.
To be honest, neither make sense, because we're not testing a specific action. I'll make it just "getting a page with the banner partial"
a0988e5 to
6602fb5
Compare
- A basic dummy app, but we give it /page-with-banners and /page-without-banners endpoints that point to the same action, the view will later be added with the various banner snippets for request testing.
- create module properly - version file is already present - include the components gem (it will probably already be included, but doesn't hurt to make sure).
- We'll be a bit aggressive and demand 100% test coverage, given we want the gem to be trusted by the calling apps.
- these test fixtures represent various valid and invalid configurations that will be picked up by the test suites introduced in the next two commits.
6602fb5 to
e74385d
Compare
3ad42ae to
d4a0e0d
Compare
README.md
Outdated
| * the same page_path is not present on two banners that are active at the same time | ||
| * paths must start with a forward-slash (/) | ||
|
|
||
| It will also display warnings (but not ) |
There was a problem hiding this comment.
It feels like the end of this statement is missing. but not what?
| @@ -1,4 +1,4 @@ | |||
| require "govuk_web_banners/test_validators/recruitment_banner" | |||
| require "govuk_web_banners/validators/recruitment_banner" | |||
There was a problem hiding this comment.
Minor: it's odd that this needed to be updated rather than just added. I'm guessing something went awry with the rebase.
There was a problem hiding this comment.
Actually, is this statement needed?
There was a problem hiding this comment.
It's needed, but it's in the wrong place. Have fixed
There was a problem hiding this comment.
(It's needed because we don't include the validators when we include the rest of the gem, because they're intended to only be present during dev/CI)
- Banners loaded from config/govuk_web_banners/recruitment_banners.yml - The partial contains its own guard call which checks for a banner on the path the request is from. This allows the partial to be inserted in the calling app without any additional config. - Code is lightweight with few checks for config validity because it will be checked as part of CI in later commits, so can be trusted to be valid at runtime. FIX (remove from rb spec)
- Update the dummy app's show view to include the recruitment banner. - Add a new fixture relevant to the dummy app. - Test for banner presence / non presence on a particular path (note that the same controller/view is called for both paths, but the request path triggers the banner or not)
- To be used in tests in the following commits
- The test strategy for this gem is to include configuration checking in during CI so that misconfigured banners won't pass, and therefore can't be released/deployed (which gives us two benefits - first, it prevents a bunch of mistakes, and secondly it allows us to simplify our code by removing guard clauses that would be needed to protect us from misconfiguration. - The test validator class includes the code used by CI validation, so here we're testing hypothetical failing configurations from the fixtures to ensure that it can detect all of them. - In a following commit, we'll actually use this class to check the config as part of the default set of rake tasks. - Exclude the test files from the released gem. - There are two classes of problem we'll check for - firstly errors that will prevent CI, then warnings which will not cause a CI failure but will notify the dev that something might not be quite right: either there's banner config for expired banners that needs to be tidied up, or a banner points to a page that isn't visible on gov.uk yet (which might be a typo, or might just be that the banner is to go live later when a page is published). - OpenURI calls are being made to the live content API rather than using the gds-api-adapters helper method. Gds-api-adapters uses Plek to check the environment that code is running in and queries content store in the same environment. For this validator we always want to be calling the live content store, and Plek doesn't allow us to specify that.
- Now that we have the class, and tests for it, and the validator, and tests for that, we use the validator to validate the live config. - If the config is missing, broken, or invalid, the check_config task will cause CI to fail, making it impossible to release the gem. - It will show a list of warnings/errors in the live config to give the developer a few clues as to what's gone wrong. - add Rainbow gem for prettier output.
d4a0e0d to
03385a3
Compare
Adds the first supported set of banners into the gem: recruitment banners. These are currently set in 4 places - once in each app that supports them - so this brings their configuration and logic into a single place. Here we're also setting up the scaffolding for testing this gem, which will be used here and in the other two banner types.
Note that some of the logic in the apps that support banners has more guard code in it than we have here. The philosophy for this gem is that where configuration is in the gem (as it should be for recruitment and global banners, the two banners currently hardcoded), that configuration should be checked during the CI stage - that is to say, at runtime we don't have to guard against an invalid configuration because an invalid configuration can't pass the test suite and be deployed as a new version of the gem.
https://trello.com/c/aYXKcrCo/375-create-govukwebbanner-gem-with-support-for-recruitment-banners
Related PR: alphagov/government-frontend#3405 - a branch of government-frontend using this branch (government-frontend is rendering the currently active banners)