[WHIT-2926] Allow offsite links to have many parents#11001
Conversation
e3eaeb5 to
3202fcd
Compare
5bac9ea to
06de1d7
Compare
fb20c65 to
5f6062f
Compare
| add_index :offsite_link_parents, | ||
| %i[parent_type parent_id offsite_link_id], | ||
| unique: true | ||
| end |
There was a problem hiding this comment.
Things we considered here. When we delete an offsite link, the row in the join table is not automatically deleted, and we could delete it as there is no need for it.
On the OffsiteLink model, we tried:
has_many :offsite_link_parents, dependent: :destroy
-> but that removed the relationship to the topical_event, breaking the call to republish the topical event
whitehall/app/models/offsite_link.rb
Line 114 in 5cfda0d
We could try
add_foreign_key :offsite_link_parents,
:offsite_links,
on_delete: :nullify
-> but that means we need to remove the not null constraint on the offsite_link_id. And it might still break as per the above.
Given this, we considered it generally fine to have stale data in the join table, but happy to hear a different take.
| # We only expect one type of parent per offsite link (either organisations, or topical events, or world location news). | ||
| # Once topical events become editionable, the parents will return all editions associated with the offsite link. | ||
| def parents | ||
| organisations + topical_events + world_location_news |
There was a problem hiding this comment.
All in all, this carries some level of risk. It is practically impossible to create a multi parent link off different types.
We considered rewriting it such that we introduce the join table and still keep a 1:1 for the non-editionable models. But that didn't look very straightforward.
There was a problem hiding this comment.
Good point! In reality, we can only have one parent type. So this will mostly likely only return an array of 1 element at any given time. Definitely will need a revisit after this scope.
5f6062f to
2463604
Compare
ryanb-gds
left a comment
There was a problem hiding this comment.
I think this is good to go.
| end | ||
|
|
||
| def republish_parent_to_publishing_api | ||
| def republish_parents_to_publishing_api |
There was a problem hiding this comment.
Not sure this method name should have changed?
In order to enable standard editions to be associated with offsite links, we are introducing a join table. This will allow an offsite link to be associated with multiple editions. This should not introduce any behavioural changes to organisations, topical events or world location news. We opted for releasing the data migration alongside the code so it runs immediately as pods come up, eliminating the gap where new offsite links could be created in the old shape. Running both together keeps the window for inconsistent data essentially zero.
2463604 to
9d031e9
Compare
…oves the association, not the offsite link itself In the existing implementation, deleting an offsite link from a standard edition would delete the offsite link record itself, which would affect other editions that are associated with the same offsite link. I've made sure only the association between the edition and the offsite link is removed, ß while retaining the offsite link record or any other editions that may be using it. This follows the established design from #11001
…oves the association, not the offsite link itself In the existing implementation, deleting an offsite link from a standard edition would delete the offsite link record itself, which would affect other editions that are associated with the same offsite link. I've made sure only the association between the edition and the offsite link is removed, ß while retaining the offsite link record or any other editions that may be using it. This follows the established design from #11001
…oves the association, not the offsite link itself In the existing implementation, deleting an offsite link from a standard edition would delete the offsite link record itself, which would affect other editions that are associated with the same offsite link. I've made sure only the association between the edition and the offsite link is removed, ß while retaining the offsite link record or any other editions that may be using it. This follows the established design from #11001
…oves the association, not the offsite link itself In the existing implementation, deleting an offsite link from a standard edition would delete the offsite link record itself, which would affect other editions that are associated with the same offsite link. I've made sure only the association between the edition and the offsite link is removed, ß while retaining the offsite link record or any other editions that may be using it. This follows the established design from #11001
…oves the association, not the offsite link itself In the existing implementation, deleting an offsite link from a standard edition would delete the offsite link record itself, which would affect other editions that are associated with the same offsite link. I've made sure only the association between the edition and the offsite link is removed, ß while retaining the offsite link record or any other editions that may be using it. This follows the established design from #11001
…oves the association, not the offsite link itself In the existing implementation, deleting an offsite link from a standard edition would delete the offsite link record itself, which would affect other editions that are associated with the same offsite link. I've made sure only the association between the edition and the offsite link is removed, while retaining the offsite link record or any other editions that may be using it. This follows the established design from #11001
…oves the association, not the offsite link itself In the existing implementation, deleting an offsite link from a standard edition would delete the offsite link record itself, which would affect other editions that are associated with the same offsite link. I've made sure only the association between the edition and the offsite link is removed, while retaining the offsite link record or any other editions that may be using it. This follows the established design from #11001
…oves the association, not the offsite link itself In the existing implementation, deleting an offsite link from a standard edition would delete the offsite link record itself, which would affect other editions that are associated with the same offsite link. I've made sure only the association between the edition and the offsite link is removed, while retaining the offsite link record or any other editions that may be using it. This follows the established design from #11001
…oves the association, not the offsite link itself In the existing implementation, deleting an offsite link from a standard edition would delete the offsite link record itself, which would affect other editions that are associated with the same offsite link. I've made sure only the association between the edition and the offsite link is removed, while retaining the offsite link record or any other editions that may be using it. This follows the established design from #11001
…oves the association, not the offsite link itself In the existing implementation, deleting an offsite link from a standard edition would delete the offsite link record itself, which would affect other editions that are associated with the same offsite link. I've made sure only the association between the edition and the offsite link is removed, while retaining the offsite link record or any other editions that may be using it. This follows the established design from #11001
What
In order to enable standard editions to be associated with offsite links, we are introducing a join table. This will allow an offsite link to be associated with multiple editions. This should not introduce any behavioural changes to organisations, topical events or world location news.
We opted for releasing the data migration alongside the code, as to mitigate against new data entering the system between the time the pods are created (code becomes available) and the migration runs. This helps us to run both together and pretty quick - to avoid having data in bad state.
Why
We are making topical events config-driven and thus editionable (using the
StandardEditionmodel).The implementation - partly outlined in the spike at #10984 - suggests that we will be using the
FeatureListmodel withFeature.The problem we're trying to solve arises when creating a new draft. Scenario with tables outlines:
We create two links:
We feature the first link only:
We make a new edition. The tables become:
Our features are now pointing to an offsite link that doesn't exist. We also need to link them up to the new feature list we've created (we can probably do that by
feature_list.features.for_eachand generate new features).We can try to copy over the offsite links too:
⛓️💥 Problem:
offsite_link_id1 in the features table with.✨ Solution
We persist the same offsite link across editions. We introduce a join table between editions (featurable) and their offsite links, as to allow an offsite link to have multiple editions.
We start off in the same way, with an edition with two offsite links, one of them featured. The tables are:
We feature the first link only:
When we make a new draft, the tables become:
We will be adding the new edition as a new parent for all our existing links, featured or not. The join table looks like so:
The feature can correctly continue pointing to the offsite link ID:
What this allows us to do:
offsite_linkstable would affect future draft generationTest plan
Run migrations up; confirm
offsite_link_parentsexists and is populated from legacy columns.Spot-check that offsite links with legacy parents now have rows in the join table.
offsite_links-> 4,172 rows in theoffsite_link_parentstableCreate offsite link under each parent type:
For each parent type above:
Ensure you're unable to create an offsite link without a parent type. NOT SURE IF THIS CAN BE TESTED
Post migration, worth also checking that there are no orphaned Offsite Links created between this PR and [WHIT-2926] Drop offsite links polymorphic columns #11029 that drops
parent_idandparent_typecolumns fromoffsite_linkstable. In Rails console, runOffsiteLink.where(parent_id: nil). The expectation is to have 0 results, unless the data was inserted prior to now.Jira
Follow-up PRs:
#11028
#11029