Render error to user if base path reserved [WHIT-2884] #10914
Conversation
patrickpatrickpatrick
force-pushed
the
avoid-accidentally-overwriting-base-path
branch
from
3611a8f to
9de162c
Compare
patrickpatrickpatrick
force-pushed
the
catch-base-path-exception-and-show-user
branch
2 times, most recently
from
f02d8a6 to
4383c4b
Compare
patrickpatrickpatrick
force-pushed
the
catch-base-path-exception-and-show-user
branch
10 times, most recently
from
a396a17 to
025310d
Compare
patrickpatrickpatrick
changed the title
patrickpatrickpatrick
force-pushed
the
catch-base-path-exception-and-show-user
branch
3 times, most recently
from
5ddabe7 to
7e59046
Compare
patrickpatrickpatrick
changed the title
patrickpatrickpatrick
force-pushed
the
catch-base-path-exception-and-show-user
branch
9 times, most recently
from
519163d to
ae69cf1
Compare
patrickpatrickpatrick
force-pushed
the
catch-base-path-exception-and-show-user
branch
from
ae69cf1 to
8b7de83
Compare
ChrisBAshton
left a comment
ChrisBAshton
left a comment
There was a problem hiding this comment.
Thanks for nudging this towards the finish line, @patrickpatrickpatrick ! Some comments below.
features/news-article.feature
Outdated
| @@ -0,0 +1,9 @@ | |||
| Feature: News article | |||
There was a problem hiding this comment.
This behaviour applies to StandardEdition documents as well as legacy ones, doesn't it?
If so, please move this test to the standard_edition.feature file (and amend it as necessary). We don't want any concrete references to news articles going forward.
| "/government/generic-editions/#{url_slug}" | ||
| end | ||
|
|
||
| def base_path_without_sequence |
There was a problem hiding this comment.
I'm not entirely sure we need this additional method? Reading between the lines, I think it's because you're wanting to catch cases where we send both foo-bar-baz and foo--bar-baz and we want to treat them as the same rather than as distinct slugs. Is that right?
If so, do you have examples of that, or is it a belt and braces thing? I'd suggest we avoid adding this unless absolutely necessary.
If it is necessary, can we add a unit test for this new method?
There was a problem hiding this comment.
If we don't have this method then base_path will return (e.g.) /government/news/news-title--2 for a news article with the same title within Whitehall. As a result the check against PublishingAPI will return negative for a match if there is published edition with path /government/news/news-title.
An alternative approach could be that we use the similar_slug_exists? method from Document before this check with the API even occurs. We could add it as a validation in Edition and then we could assume that the Edition won't have the --NUMBER suffix on the base path.
There was a problem hiding this comment.
Ah, I'd misunderstood - that makes sense then, thanks.
Good reminder about the similar_slug_exists? method - which will require some rethinking soon anyway, because it's currently scoped by document_type, and ultimately everything will eventually be a StandardEdition so the method would start raising false positives. Let's revisit that later.
Your current solution is good - perhaps it would benefit from an inline comment where it's used in publishing_api.rb.
| @edition.destroy! | ||
| build_edition | ||
| build_edition_dependencies | ||
| render :new |
There was a problem hiding this comment.
Just to be clear - would the submitted values be retained when encountering this error, or would the user have to fill it all out again?
There was a problem hiding this comment.
The submitted values would be retained since build_edition and build_edition_dependencies use the parameters that the user has submitted to make a new Edition.
| test "destroys saved new draft edition if base path conflict with published edition" do | ||
| login_as create(:gds_admin) | ||
|
|
||
| published_edition = create(:published_news_article, title: "Test title") |
There was a problem hiding this comment.
Oh and this needs to not refer to legacy news article please.
patrickpatrickpatrick
force-pushed
the
catch-base-path-exception-and-show-user
branch
from
8b7de83 to
f754c36
Compare
The user attempts to create a document with the same base path as a published edition then destroy the newly saved edition. This is required as the edition is saved before the conflict check. If the edition is not destroyed then the user can still edit and attempt to save and/or force publish the conflicting new edition.
patrickpatrickpatrick
force-pushed
the
catch-base-path-exception-and-show-user
branch
from
f754c36 to
f2f3246
Compare
Instead of presenting a server error to the user if they try and create an edition that has the base path of another published edition, the error is now presented to the user.
patrickpatrickpatrick
force-pushed
the
catch-base-path-exception-and-show-user
branch
from
f2f3246 to
5088778
Compare
What
When trying to create a document that has an already reserved base path, catch the error and render an error with the title field on the new document form.
Further details
Why
Continues the work of #10912, now presenting an error to the user on the form instead of rendering a server error and removing the contents of the form.
Visual Differences
On attempting to create a new draft with the same base path:
This application is owned by the Whitehall Experience team. Please let us know in #govuk-whitehall-experience-tech when you raise any PRs.
Follow these steps if you are doing a Rails upgrade.