Use v2 of the Publishing API everywhere #832
Conversation
The app is currently using gds-api-adapters v39.2.0, but these methods were removed in v38.0.0 [1], so this code cannot possibly work! All the tests still pass and I hope to remove the code which theoretically invokes these methods in later commits. [1]: https://github.com/alphagov/gds-api-adapters/blob/master/CHANGELOG.md#3800
Since the `#call` method always raises an exception, we can be sure that it isn't being executed in production and we're justified in simplifying it like this.
It's now obvious that this script would always fail due to the removal of the `GdsApi::PublishingApi#put_content_item` method from `gds-api-adapters`, so there's no point in keeping it around.
Since both `ManualPublishingAPIExporter#call` and `ManualSectionPublishingAPIExporter#call` will always raise an exception when their constructors are supplied with this `export_recipient`, we can be sure that it isn't being executed in production and we're justified in simplifying it like this.
Since an exception will always be raised within this method, we can be sure that it isn't being executed in production and we're justified in simplifying it like this.
Since an exception will always be raised within this method, we can be sure that it isn't being executed in production and we're justified in simplifying it like this.
This class is not referenced anywhere else in the code or in the specs and in any case it's only entry point (`#export_all`) no longer works and will always raise an exception. It looks as if it should've been removed as part of this commit [1]. [1]: a739e78
Going further on many of these would require rails 4 or mongoid 5 so we're just going as far as we can for now. For govuk_frontend_toolkit we go as far as the same version that manuals-frontend has; it's good to keep these in sync as we want the previews of markdown that manuals-publisher does to be as close as possible to the "live" display we'd get from manuals-frontend.
|
I think you could replace the The I think it's probably 50:50 on whether we drop that script as you've done already, or fix the underlying thing and add tests. If we chose to fix it I suspect it would be worth looking at the I'm 👍 on the 2nd half of removing the bulk exporter as I don't recall that being used recently. |
|
Thanks for the feedback - that's really helpful. I'll come back to this shortly to address your comments. A more general question: When you have a moment, would you be able to look through the scripts in the bin directory and give us some indication of which ones are definitely needed or which ones have been run relatively recently? Alternatively could you point us at anyone else who would know? |
|
I think most of the scripts in bin are there to help fix the result of bugs or missing functionality in the app. For example:
A good thing to do would be to turn these into rake tasks as then they can be run through jenkins and we'd get history of how often they're used. (See: https://github.com/alphagov/govuk-puppet/blob/f1c27d74dd0f361159bb5531473343730c692ea2/modules/govuk_jenkins/templates/jobs/run_rake_task.yaml.erb) |
|
@h-lame: Thanks for taking the time to explain the scripts! 👍 |
This script relies on `ManualsPublisher#document_services` which is been broken in a couple of different ways: * The `AbstractDocumentServiceRegistry` class was removed in this commit [1]. * The `ManualsPublisher#observer_registry` method was removed in this commit [2]. In this PR comment [3], @h-lame said: > Suspect this could be got rid of and we re-write as/when requested > via a support ticket. So I think it's safe to remove. [1]: 43e2e75 [2]: 9bbf2d3 [3]: #832 (comment)
This script relies on `ManualsPublisher#document_services` which is broken in a couple of different ways: * The `AbstractDocumentServiceRegistry` class was removed in this commit [1]. * The `ManualsPublisher#observer_registry` method was removed in this commit [2]. In this PR comment [3], @h-lame said: > Suspect this could be got rid of and we re-write as/when requested > via a support ticket. So I think it's safe to remove. [1]: 43e2e75 [2]: 9bbf2d3 [3]: #832 (comment)
|
Closing in favour of #847. |
|
Note: I've created a new issue to capture the problem with the |
The two places which are using v1 of the Publishing API have been broken since this commit, because they rely on methods which were removed from gds-api-adapters in v38.0.0; the application is currently using v39.2.0. This PR removes a bunch of code around the invocations of these non-existent methods on the basis that the code is currently useless.
I'm very confident that
ManualPublishingApiBulkDraftExporterand its associated code can be removed, because the rake task that was using it was removed in this commit.I'm slightly less confident that the
bin/reslug_manual_sectionscript and its associated code can be removed, but given that it is currently broken, if someone wants to keep it, it will need to be fixed. I would also argue that it should only be kept if we write a test for it to ensure that it stays in a working state. /cc @h-lameI did also investigate removing the use of
GdsApi::TestHelpers::PublishingApiin the tests, but it appears thatGdsApi::TestHelpers::PublishingApiV2#request_json_includesis not a direct replacement forGdsApi::TestHelpers::PublishingApi#request_json_includingand I can't see an easy way around this.