-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
World taxon rake update code extracted to class #1900
World taxon rake update code extracted to class #1900
Conversation
78868e1
to
357f0bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work on this—I know I've added quite a few comments, but they are mostly pretty minor!
1724827
to
e1b3203
Compare
e1b3203
to
9063d6c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple more comments.
lib/world_taxon_update_helper.rb
Outdated
|
||
if suffix_index.nil? | ||
log_rake_progress("No change to title: #{title}") | ||
return "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we returned title
here, instead of ""
, we could simplify the caller from next if new_title.empty? || title == new_title
to next if title == new_title
.
expect { described_class.new(log_file).add_country_names }.to output(/An error occurred while processing taxon/).to_stderr | ||
end | ||
|
||
it "logs an error to stderr but doesn't continue processing if BulkPublishTaxon fails" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The call to BulkPublishTaxon is the last thing that happens, so I don't think "but doesn't continue" (and the associated expect(Taxonomy::BulkPublishTaxon).to receive(:call).once
check) make sense for this test.
To test the processing continues when GdsApi::HTTPConflict or StandardError occur, we should mock one of the calls in the loop to throw those exceptions (Taxonomy::BuildTaxon.call or Taxonomy::UpdateTaxon.call—assuming that these can actually throw these exceptions?).
expect { described_class.new(log_file).remove_country_names }.to output(/An error occurred while processing taxon/).to_stderr | ||
end | ||
|
||
it "logs an error to stderr but doesn't continue processing if BulkPublishTaxon fails" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The call to BulkPublishTaxon is the last thing that happens, so I don't think "but doesn't continue" (and the associated expect(Taxonomy::BulkPublishTaxon).to receive(:call).once check) make sense for this test.
To test the processing continues when GdsApi::HTTPConflict
or StandardError
occur, we should mock one of the calls in the loop to throw those exceptions (Taxonomy::BuildTaxon.call
or Taxonomy::UpdateTaxon.call
—assuming that these can actually throw these exceptions?).
…be more reliable in terms of avoiding mocking cross-contamination than trying to test the rake task directly. All methods moved to single renamed class and tests completed
9063d6c
to
31dc945
Compare
31dc945
to
29c83d4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Implements the testing for the Rake task which will check all of the [World taxonomy titles]
(https://docs.publishing.service.gov.uk/manual/world-taxonomy.html) adding the name of the country they relate to where necessary. Trello card. All of the world taxons are descendants of /world/all content item
For accessibility reasons, taxon titles need to be more unique. Having multiple versions of pages for different
countries all titled e.g. 'Trade and invest' goes against the following WCAG guidelines.
https://www.w3.org/WAI/WCAG22/Understanding/page-titled.html
https://www.w3.org/WAI/WCAG22/Techniques/failures/F25
Essentially, world location pages such as: gov.uk/world/japan include links to sections such as 'Coming to the UK' or 'Birth, death and marriage abroad'. These sections can however also appear as pages in their own right, for example, they can appear as a link under 'Explore the topic' where the title makes the content sound generic but actually links to specific information relating to that country. E.g. the 'Coming to the UK' link at the bottom of this page: https://www.gov.uk/government/publications/tuberculosis-test-for-a-uk-visa-clinics-in-cameroon actually links to the dedicated page - https://www.gov.uk/world/coming-to-the-uk-cameroon
For titles which need to be updated, this task takes the name of the country from the internal name and then combines it with a relevant word or symbol to create a full title:
E.g.
All template versions of pages, the root and any taxon that already includes the country name in the title, are skipped and not processed.