WGBH-MLA / ams

Archival Management System to support the American Archive of Public Broadcasting
GNU General Public License v3.0
5 stars 8 forks source link

Refactors transaction step for saving admin data #889

Closed afred closed 3 months ago

afred commented 4 months ago

By default, multi-valued input fields display an additional set of input fields that are blank. These get submitted by the form and are usually discarded, but because we are treating Sony Ci ID field special, it was erroneously saving additional blank values for Sony Ci ID, which was resulting in multiple players of the same media dislaying on AAPB when the record was published.

Multiple Sony Ci IDs are allowed in cases of mult-part Assets. They are stored as a serialized JSON array in the AdminData.sonyci_id field, which is related to Asset reords via a global ID field on AdminData.gid. And the global ID fields was used instead of a normal foreign key because the AssetResource used to be just Asset, and stored in Fedora, not the database, and a global ID was the closest thing to a foreign key across 2 persistence layers that we could think of :)

Fixes #882.

afred commented 4 months ago

@bkiahstroud and/or @orangewolf could I get an extra set of 👁️ on the logic here? This is some consequential code as it handles not only our Asset relations to Sony Ci media, but the Asset relation to the Bulkrax Importers and Batch Ingest Batches as well.

I think i tested all possibilities of adding/removing Sony Ci IDs, but did not do any kind of regression testing on persistence of bulkrax identifier or other admin data fields.

afred commented 4 months ago

@bkiahstroud even more of a simplification...

afred commented 4 months ago

Specs are passing for me locally, so idk what's up with the failures here. Can you replicate the failures locally?

bkiahstroud commented 4 months ago

Specs are passing for me locally, so idk what's up with the failures here. Can you replicate the failures locally?

I can, I get the same failures locally

bkiahstroud commented 4 months ago

@afred do you have changes locally that haven't been pushed up to this PR yet? For example, I don't see a new method named AdminData#validate_undeletable_fields that you referenced in this comment