Remove the publishing step when trying to republish HTML attachments for unpublished documents #8780
Conversation
minhngocd
force-pushed
the
remove-unpublishing-step-when-republshing-unpublished-or-withdraw
branch
4 times, most recently
from
94af7ed to
bc60706
Compare
…for unpublished documents The publishing of the HTML attachment could leave the document in a bad state and especially dangerous around sensitive documents where they should be unpublished, but are then inadvertently published during the republishing process. We should be able to reapply the redirects for unpublishing without the need to publish the document first. Withdrawn document process has been left as is for the moment.
minhngocd
force-pushed
the
remove-unpublishing-step-when-republshing-unpublished-or-withdraw
branch
from
bc60706 to
630ab47
Compare
minhngocd
changed the title
There was a problem hiding this comment.
A few comments for your consideration - sorry we didn't get a chance to pair on this as there is definitely enough complexity in this small PR to warrant it. Maybe let's jump on a call to discuss. Pretty sure everything is correct anyway but just want to confirm my understanding.
| unpublish_if_required | ||
| if Edition::PRE_PUBLICATION_STATES.include?(edition.state) | ||
| update_draft(update_type: "republish") | ||
| elsif edition.unpublishing && edition.withdrawn? |
There was a problem hiding this comment.
Would just edition.withdrawn? be sufficient? I don't think you can have a withdrawn edition without an unpublishing... 🤔
There was a problem hiding this comment.
yup I can apply this
| do_publish("republish") | ||
| discard_drafts(deleted_html_attachments) | ||
| withdraw | ||
| elsif edition.unpublishing |
There was a problem hiding this comment.
Similarly we could use edition.unpublished? here, I think
There was a problem hiding this comment.
yup I can apply this
| publication = create(:unpublished_publication_in_error_no_redirect) | ||
| PublishingApiWorker.any_instance.expects(:perform).never | ||
| call(publication) | ||
| end |
There was a problem hiding this comment.
I'm personally not a huge fan of tests that test something doesn't happen, or at least not beyond an initial TDD cycle. Theoretically the set of things we could test don't happen is infinite so I always feel a bit dubious about it.
There was a problem hiding this comment.
In a documentation sense that we explicitly want to call out that publishing "should not" happen, then I think it should be tested for things that we definitely do not want to happen otherwise bad things happen. To prevent in the future if people do add that functionality back in, they can see the test and thing "hmm do i really want to do this"
There was a problem hiding this comment.
Fair enough, I don't feel too strongly about it.
|
From further discussion in chats with @ryanb-gds, I think it would be good to merge this as is without the conditionals refactor to check state of edition instead of looking at the unpublishing association. We can have that done separately in another PR case it does have an impact |
minhngocd
deleted the
remove-unpublishing-step-when-republshing-unpublished-or-withdraw
branch
The publishing of the HTML attachment could leave the document in a bad state and especially dangerous around sensitive documents where they should be unpublished, but are then inadvertently published during the republishing process.
We should be able to reapply the redirects for unpublishing without the need to publish the document first.
Withdrawn document process has been left as is for the moment.
https://trello.com/c/e75Mp1Ap/2301-investigate-if-we-can-avoid-temporary-publishing-of-html-attachments-during-republishing-of-unpublished-edition
Follow these steps if you are doing a Rails upgrade.