Skip to content
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

Stop validating change notes for draft editions #9806

Closed
wants to merge 1 commit into from

Conversation

ryanb-gds
Copy link
Contributor

We only want to make sure that editions have a change note at the point that they are submitted for publication (or force published/scheduled).

Validating change notes for draft editions could cause bugs. For example, if the user replaced an attachment before adding a change note to the edition, the replacement silently failed, leaving the old attachment live.

We therefore change the validation logic to only validate non-draft editions.

Unfortunately this required us to temporarily update the state of the editions in the edition publisher and scheduler services. Those services trigger the edition validation before the state is updated, so the validation occurs as if the editions are still drafts, which isn't logical behaviour.

Trello: https://trello.com/c/31CtEP8h - note that this is how we fix the bug, even if it looks unrelated

@@ -11,7 +11,6 @@ Feature: Save edition content with visual editor
When I fill in the required fields for publication "Publication with visual editor" in organisation "Visual ministry"
And I save and go to document summary
Then I should see the visual editor on subsequent edits of the publication
And I force publish the publication "Publication with visual editor"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This failed because the test was set up invalidly before (the publication is invalid due to missing attachment) but the change note stuff was obscuring the failure. In any case it doesn't seem necessary for the test scenario to prove what it was intended to prove, so I've removed it


assert_select ".govuk-link", text: "Preview on website (opens in new tab)", href: draft_edition.public_url(draft: true), count: 0
assert_select ".govuk-inset-text", text: "To see the changes and share a document preview link, add a change note or mark the change type to minor."
end
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no reason not to render the preview link in this scenario, so removing this test

@ryanb-gds ryanb-gds force-pushed the validate-change-note-on-submit-or-publish branch 2 times, most recently from a484a9d to cd1324a Compare January 13, 2025 13:40
We only want to make sure that editions have a change note at the point that they are submitted for publication (or force published/scheduled).

Validating change notes for draft editions could cause bugs. For example, if the user replaced an attachment before adding a change note to the edition, the replacement silently failed, leaving the old attachment live.

We therefore change the validation logic to only validate non-draft editions.

Unfortunately this required us to temporarily update the state of the editions in the edition publisher and scheduler services. Those services trigger the edition validation before the state is updated, so the validation occurs as if the editions are still drafts, which isn't logical behaviour.
@ryanb-gds ryanb-gds force-pushed the validate-change-note-on-submit-or-publish branch from cd1324a to 5a5062c Compare January 13, 2025 13:52
@ryanb-gds
Copy link
Contributor Author

Closing this because it requires us to manipulate the state of the edition directly, ignoring the API provided by the transitions gem. This can cause weird side effects, such as not triggering validation callbacks depending on the transition state machine state. Unfortunately this happens in quite a few places in Whitehall 😞

@ryanb-gds ryanb-gds closed this Jan 13, 2025
@ryanb-gds ryanb-gds deleted the validate-change-note-on-submit-or-publish branch January 13, 2025 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant