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

Remove mapped_specialist_topic_content_id #2710

Conversation

unoduetre
Copy link
Contributor

⚠️ 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.

Warning

The following PR in the whitehall repository must be merged first.

What

As part of work to retire specialist topics, we temporarily stored a mapping between the archived specialist topic and its document collection conversion. We should remove this once we’re done.

Why

To keep the system clean and avoid unnecessary data.

Trello card

Copy link
Contributor

@beccapearce beccapearce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one looks good too. Again it would be great to make the commit messages distinct. (I know one is just generated), I usually just use Run rake build_schemas for the second commit so people know they can not pay it too much attention.

@unoduetre
Copy link
Contributor Author

This one looks good too. Again it would be great to make the commit messages distinct. (I know one is just generated), I usually just use Run rake build_schemas for the second commit so people know they can not pay it too much attention.

They are distinct, because of the second line, which is not visible by default here (you'd need to click on ...), but it is visible in the commit list.

@unoduetre unoduetre marked this pull request as ready for review April 18, 2024 15:25
@beccapearce
Copy link
Contributor

yep yep, I see the rest of the commit message. it would be nice to have it in the title of the commit I think. Just thinking about scrolling down the commits in the github GUI where the extended commit isn't visible by default.

@unoduetre unoduetre changed the title Remove mapped_specialist_topic_content_id from document collections Remove mapped_specialist_topic_content_id Apr 22, 2024
@unoduetre unoduetre force-pushed the 2503-remove-mappedspecialisttopiccontentid-from-document-collections-m-l branch from 08aaa9f to 504accc Compare April 22, 2024 15:17
@unoduetre
Copy link
Contributor Author

unoduetre commented Apr 22, 2024

@beccapearce I've updated commit messages and PR name.

@hannako
Copy link
Contributor

hannako commented Apr 23, 2024

Thanks @unoduetre it's great to see this almost gone!

You can reference that the work reverts #2385

And whilst it seems like a nitpick, I do agree with Becca's comment about simple scannable commit titles. You can see the titles for the original implementation as a guide.

@unoduetre unoduetre force-pushed the 2503-remove-mappedspecialisttopiccontentid-from-document-collections-m-l branch from 504accc to a7f0243 Compare April 24, 2024 12:35
Remove mapped_specialist_topic_content_id from
document_collection.jsonnet.
Reverts d2ff3a5
The files have been regenerated by running bundle exec rake build_schemas.
Reverts 56937d0
@unoduetre unoduetre force-pushed the 2503-remove-mappedspecialisttopiccontentid-from-document-collections-m-l branch from a7f0243 to 2909bd4 Compare April 24, 2024 12:40
Copy link
Contributor

@beccapearce beccapearce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me! 💪

@unoduetre unoduetre requested review from brucebolt and JonathanHallam and removed request for brucebolt April 25, 2024 08:42
Copy link
Contributor

@JonathanHallam JonathanHallam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thank you :)

@unoduetre unoduetre merged commit 7264d70 into main Apr 25, 2024
44 checks passed
@unoduetre unoduetre deleted the 2503-remove-mappedspecialisttopiccontentid-from-document-collections-m-l branch April 25, 2024 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants