Remove specialist topic code from Publishing API #2669

unoduetre · 2024-03-12T13:03:07Z

⚠️ This repo is Continuously Deployed: make sure you follow the guidance ⚠️

This application is owned by the publishing platform team. Please let us know in #govuk-publishing-platform when you raise any PRs.

Follow these steps if you are doing a Rails upgrade.

What

When all specialist topic pages have been archived and redirected, we need to remove the specialist topic related code from Publishing API. This application is owned by Publishing Platform.

Why

To remove technical debt

Trello ticket

hannako

Appreciate this is only draft, but wanted to point out a few things before you got too far!

content_schemas/dist/formats/answer/frontend/schema.json

content_schemas/dist/formats/email_alert_signup/frontend/schema.json

hannako · 2024-03-13T16:25:30Z

content_schemas/formats/shared/base_links.jsonnet

@@ -1,22 +1,21 @@
 {


d85bfbf is a big commit and hard to review. I've always been slightly confused by the difference between base_links and whitehall_edition_links. What do you think about splitting this commit:

Remove topic from base_links, and run rake build_schemas as per the docs. What I don't think that will do is update the examples so you'd have to add those to the commit manually. Perhaps by running rake and seeing which fail?

Remove topic from whitehall_edition_links and same as above.

I will split it later before putting this PR as ready for review.

I've separated dist files (they are autogenerated) from others, which should make it easier to review.

hannako · 2024-03-13T16:26:56Z

content_schemas/dist/formats/call_for_evidence/frontend/schema.json

@@ -1020,12 +1020,6 @@
      "type": "object",
      "additionalProperties": false,
      "properties": {
-        "additional_topics": {


I'll come back to this, or have you already checked? Ie do you have anything that confirms these fields (additional_topics and primary_topics) are specialist topics?

I've grepped through email apps and found nothing. If you know that it is used somewhere else, please, let me know.

Also, we have contract tests, but if they cannot be relied on, please, let me know.

I'm a bit confused by ddaf127 and 513f78f

It looks like we need to remove topics, additional_topics and primary topic from the tags attribute of the email alert schema. And the relevant examples need to be updated. But I'm struggling to figure out if the examples you've added are including this schema. If they are, then it might be that there's some additional code changes needed which would form part of this change.

So let's make this it's own PR. As yet I've not removed any code from email alert service or email alert api that might be using these fields. So hopefully the contract tests will fail in a helpful and informative way so we can link all the relevant work.

hannako · 2024-03-14T21:16:49Z

@unoduetre I know this is still a draft, and that you've previously said you want the schemas to reflect the state of the database and therefore not split the PR. But at 378 files and 11 commits-ish I really think this PR is too big to review or safely merge:

if something breaks as a result of shipping this PR it will be very hard to figure out which change was responsible.
I think it's better practice to have a single pr for the database migration alone so that if we need to roll anything back we don't add complexity.

My suggestion: We could remove topics from whitehall_edition links in one PR, remove it from base_links in a second PR, remove additional_topics etc in a third. Is there something I'm missing here that makes that impossible?

…shing-api-l

unoduetre · 2024-03-15T11:37:48Z

As discussed with @hannako, this PR need to be split into a number of separate PRs.

unoduetre · 2024-03-15T11:53:56Z

I've added additional commits as I've found during review one more thing I missed earlier.

hannako

Thanks for such a thorough piece of investigation @unoduetre really amazing attention to detail!

I have obviously suggested a rework, but hopefully it will make things easier rather than more complicated. Am very happy to pair on any aspect.

hannako · 2024-03-15T09:40:22Z

lib/schema_generator/expanded_links.rb

-      # Taxons that have been created by merging old 'legacy' taxons will have
-      # a reverse link to determine where the replacement Topic Taxonomy taxon
-      # now resides
-      "topic_taxonomy_taxons" => "frontend_links_with_base_path",


It sounds like topic_taxonomy_taxons were to do with mapping specialist topics to taxonomy topics, so I think it's fair to assume that this field is no longer required. It also looks like this attribute isn't being used in the publishing apps or in the frontend apps anyhow 👍

So let's cherry pick bf0fa5f and b155257 into a fresh PR. Linking to that grep in the commit message/PR description, so that it's easy to review and merge with confidence that nothing will break.

hannako · 2024-03-15T09:58:56Z

content_schemas/examples/call_for_evidence/frontend/closed_call_for_evidence.json

@@ -69,17 +69,6 @@
        "document_type": "organisation",
        "analytics_identifier": "D1198"
      }
-    ],


Let's cherry pick 234c0ee and 00f784c into its own work - "Remove specialist topics from whitehall_edition_links and base_link schemas, and update relevant schema examples."

I think the CI will fail until we've updated any code that looks for topics in links. Eg breadcrumbs in the gem. In fact having this PR as its own piece of work will help us identify dependencies and give us a to do list of code to update.

hannako · 2024-03-15T13:10:48Z

content_schemas/formats/email_alert_signup.jsonnet

@@ -37,7 +37,6 @@
        email_alert_type: {
          type: "string",
          enum: [
-            "topics",


Let's cherry pick ad572f9 and 2a8893b into its own work: "Remove topics from email_alert_signup schema and associated examples". I'm not sure we've removed this field from the publishing applications as per the documentation for removing-a-field-from-a-content-schema so it would be nice to have this as a standalone PR, so we can link to relevant changes in other apps.

hannako · 2024-03-15T13:21:29Z

content_schemas/dist/formats/call_for_evidence/frontend/schema.json

@@ -1020,12 +1020,6 @@
      "type": "object",
      "additionalProperties": false,
      "properties": {
-        "additional_topics": {


I'm a bit confused by ddaf127 and 513f78f

It looks like we need to remove topics, additional_topics and primary topic from the tags attribute of the email alert schema. And the relevant examples need to be updated. But I'm struggling to figure out if the examples you've added are including this schema. If they are, then it might be that there's some additional code changes needed which would form part of this change.

So let's make this it's own PR. As yet I've not removed any code from email alert service or email alert api that might be using these fields. So hopefully the contract tests will fail in a helpful and informative way so we can link all the relevant work.

hannako · 2024-03-15T13:38:41Z

db/migrate/20240314140958_delete_specialist_topics_from_database.rb

@@ -0,0 +1,43 @@
+class DeleteSpecialistTopicsFromDatabase < ActiveRecord::Migration[7.1]


As mentioned previously - I think it's safer to have a standalone PR for the database migration. And I'd definitely want to chat this PR through with someone from Publishing Platform first.

unoduetre · 2024-03-18T15:43:45Z

This PR has been split into the following PRs:

unoduetre added the do not merge label Mar 12, 2024

unoduetre force-pushed the 2417-remove-specialist-topic-code-from-publishing-api-l branch from 6358da3 to 8e71410 Compare March 13, 2024 14:36

hannako reviewed Mar 13, 2024

View reviewed changes

unoduetre mentioned this pull request Mar 14, 2024

Remove specialist topics related code from publishing-api Pact specs alphagov/gds-api-adapters#1240

Merged

unoduetre added 10 commits March 14, 2024 16:44

Remove topic taxonomy taxons

bf0fa5f

Remove topic taxonomy taxons (dist)

b155257

Remove links to topics

234c0ee

Remove links to topics (dist)

00f784c

Remove topic content schema

a624df0

Remove topic content schema (dist)

2f69bb1

Remove topics from email alert types

ad572f9

Remove topics from email alert types (dist)

2a8893b

Remove topic tags

ddaf127

Remove topic tags (dist)

513f78f

unoduetre added 5 commits March 15, 2024 08:47

Fix specs other than Pact specs

e20c58c

Revert removing topic taxonomy taxons

ac09a5a

Revert removing topic taxonomy taxons (dist)

09d159a

Fix Pact specs

f7ba0ff

Remove specialist topics from database

1ec47ae

unoduetre force-pushed the 2417-remove-specialist-topic-code-from-publishing-api-l branch from a73c7c6 to 1ec47ae Compare March 15, 2024 09:15

unoduetre marked this pull request as ready for review March 15, 2024 09:29

Merge branch 'main' into 2417-remove-specialist-topic-code-from-publi…

aab4de5

…shing-api-l

unoduetre added 3 commits March 15, 2024 11:41

Remove topic from allowed_document_types.yml

97884ee

Fix after merging main

524c8ba

Remove topic from allowed_document_types.yml (dist)

e684811

hannako reviewed Mar 15, 2024

View reviewed changes

unoduetre marked this pull request as draft March 18, 2024 13:56

unoduetre closed this Mar 18, 2024

unoduetre mentioned this pull request Mar 27, 2024

Release 95.0.1 alphagov/gds-api-adapters#1245

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove specialist topic code from Publishing API #2669

Remove specialist topic code from Publishing API #2669

unoduetre commented Mar 12, 2024

hannako left a comment

hannako Mar 13, 2024

unoduetre Mar 13, 2024

unoduetre Mar 15, 2024

hannako Mar 13, 2024

unoduetre Mar 13, 2024

unoduetre Mar 13, 2024

hannako Mar 15, 2024

hannako commented Mar 14, 2024

unoduetre commented Mar 15, 2024

unoduetre commented Mar 15, 2024

hannako left a comment

hannako Mar 15, 2024

hannako Mar 15, 2024

hannako Mar 15, 2024

hannako Mar 15, 2024

hannako Mar 15, 2024

unoduetre commented Mar 18, 2024

		@@ -0,0 +1,43 @@
		class DeleteSpecialistTopicsFromDatabase < ActiveRecord::Migration[7.1]

Remove specialist topic code from Publishing API #2669

Remove specialist topic code from Publishing API #2669

Conversation

unoduetre commented Mar 12, 2024

What

Why

hannako left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hannako commented Mar 14, 2024

unoduetre commented Mar 15, 2024

unoduetre commented Mar 15, 2024

hannako left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unoduetre commented Mar 18, 2024