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

Fix bug with downtime messages not being published when no draft present #615

Merged
merged 2 commits into from Apr 7, 2017

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Apr 7, 2017

%w(published, archived) produces:
["published, ", "archived"] rather than the expected:
["published", "archived"]

  • Use %w(published archived)
    We should have some rubocop protection against using , within %w

This fix prevents unnecessary calls to discard_draft which were throwing an error because no draft existed. This error stopped the downtime message from being published:
https://github.com/alphagov/publisher/blob/fix-downtime/app/services/publishing_api_workflow_bypass_publisher.rb#L14

Error that was being thrown:

Response body: {"error":{"code":422,"message":"There is not a draft
edition of this document to discard"}} Request body: {}

This error was also masked by the way discard_draft was being stubbed on an instance of publishing_api. Removing the stub revealed the failing test.

I'm still not entirely sure why the stubbed method in the test was masking that the method was being called: https://github.com/alphagov/publisher/blob/fix-downtime/test/unit/services/publishing_api_workflow_bypass_publisher_test.rb#L33

Zen: https://govuk.zendesk.com/agent/tickets/1963886

cc @whoojemaflip @cbaines

fofr added 2 commits Apr 7, 2017
By stubbing `:discard_draft` for all tests, the following test was
giving a false pass:

`Services.publishing_api.expects(:discard_draft).never`

When the stub is removed, we see that `discard_draft` is being called.
`%w(published, archived)` produces: [“published, “, “archived”] rather
than the expected [“published”, “archived”]

Fix prevents calls to `discard_draft` which were throwing an error
because no draft existed.

See: https://govuk.zendesk.com/agent/tickets/1963886

`Response body: {"error":{"code":422,"message":"There is not a draft
edition of this document to discard"}} Request body: {}`
Copy link
Contributor

@whoojemaflip whoojemaflip left a comment

How this ever worked is beyond me

@cbaines
cbaines approved these changes Apr 7, 2017
@fofr fofr merged commit 9c1b47a into master Apr 7, 2017
2 checks passed
2 checks passed
continuous-integration/jenkins/branch This commit looks good
Details
security/snyk No new vulnerabilities
Details
@fofr fofr deleted the fix-downtime branch Apr 7, 2017
fofr added a commit to alphagov/govuk-lint that referenced this pull request Apr 7, 2017
A comma in a word array caused a subtle bug in Publisher:
alphagov/publisher#615

We encourage use of %w, we should protect against the risk of misuse.
fofr added a commit to alphagov/govuk-lint that referenced this pull request Apr 7, 2017
A comma in a word array caused a subtle bug in Publisher:
alphagov/publisher#615

We encourage use of %w, we should protect against the risk of misuse.
@fofr
Copy link
Contributor Author

@fofr fofr commented Apr 7, 2017

Corresponding linting change to avoid this happening again: alphagov/govuk-lint#71

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.