Stop 404 when withdrawing a section on published manual #2263
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
davidgisbey
force-pushed
the
only-withdraw-draft-sections-if-draft-present
branch
5 times, most recently
from
586e2f5 to
02b778b
Compare
davidgisbey
force-pushed
the
only-withdraw-draft-sections-if-draft-present
branch
2 times, most recently
from
08fb8c4 to
88bc7c2
Compare
When a manual is published all sections are published as well. Post publication sections retain a published state expect in 2 circumstances. 1. they are edited 2. they are withdrawn If they are edited a draft section is pushed downstream to Publishing API and the Draft Content Store. When the section is withdrawn, we need to discard the draft from Publishing API. There's some logic to handle this in the remove_service which checks to see if the section is a draft (i.e. has a draft edition in Publishing API) and if so it attempts to discard the draft. However, there's an issue with this. Within the remove service assign_attributes is called on the section. If you go and have a look at the method defined in the Section model, you can see that it actually creates a new draft section when called if the latest edition is published. It then passes change note params to the new draft edition of the section. This means that when #draft? is called on the section it will always return true. This causes an issue when the following is called: ``` if section.draft? Adapters.publishing.discard_section(section) end ``` There blows up when a section was in a published state prior to the remove_service being called as there is no draft section in the Publishing API database. There are feature specs which explicity cover that the section is removed when Published sections are withdrawn. The proposed fix of capturing the state prior so assign_attributes being called breaks these tests. I'm pretty sure that the tests are actually testing the incorrect behaviour which is causing the bug though. At first glance I thought it might be possible to not bother creating a new draft edition prior to removing the section from the manual, but it's actually required for capturing the change note the user inputs so that users can see why the section was removed. When an edition is published downstream, any change notes are pulled from removed sections added as PublicationLogs These are then rendered on the frontend.
davidgisbey
force-pushed
the
only-withdraw-draft-sections-if-draft-present
branch
from
88bc7c2 to
073cb41
Compare
ryanb-gds
approved these changes
Jan 2, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
When a manual is published all it's sections are published as well. Published sections retain a published state except in 2 circumstances.
If they are edited a draft section is pushed downstream to Publishing API and the Draft Content Store.
When this occurs and the section is withdrawn, we need to discard the draft Section from Publishing API.
There's some logic to handle this in the remove_service which checks to see if the section is a draft (i.e. has a draft edition in Publishing API) and if so it attempts to discard the draft.
However, there's an issue with this. Within the remove service
assign_attributesis called on the section. If you go and have a look at the method defined in the Section model, you can see that it actually creates a new draftsection when called on a published section. It then passes change note params to the new draft edition of the section.
This means that when
#draft?is called on the section it will always return true. This causes an issue when the following is called:Publiishing API blows up as there is no draft section in the Publishing API database.
There are feature specs which explicitly cover that the section is removed when Published sections are withdrawn. The proposed fix of capturing the state prior so
assign_attributesbeing called breaks these tests.I'm pretty sure that the tests are actually testing the incorrect behaviour which is causing the bug though.
At first glance I thought it might be possible to not bother creating a new draft edition prior to removing the section from the manual, but it's actually required for capturing the change note the user inputs so that users can see why the section was removed.
When an edition is published downstream, any change notes are pulled from removed sections and added as PublicationLogs
These are then rendered on the frontend.
Trello card
https://trello.com/c/GMy5HBPv/1830-fix-manual-section-withdrawal-error-caused-by-missing-draft-in-publishing-api
Follow these steps if you are doing a Rails upgrade.