Refactors transaction step for saving admin data #889
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
|
@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. |
bkiahstroud
requested changes
Jun 11, 2024
Co-authored-by: Kiah Stroud <[email protected]>
Co-authored-by: Kiah Stroud <[email protected]>
Uses AdminData.attributes_for_updates as a filter to the change set fields. Filters out blank sony ci ids so they don't get serialized. Adds validation routine to AdminData for guarding against fields that cannot be deleted once set: bulkrax_importer_id and batch_ingest_batch_id. These IDs are foreign keys and if they accidentally get unset, we loase relational integrity, so need to make sure they are not accidentally getting unset via ingest, or updating Assets via UI, or anywhere else.
|
@bkiahstroud even more of a simplification...
|
|
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
reviewed
Jun 15, 2024
|
@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 |
…please squash when merging and remove this msg.
bkiahstroud
reviewed
Jun 17, 2024
|
|
||
| # The presence of the key with a "blank" value indicates we're intentionally emptying the value | ||
| change_set.fields.key?(key) && change_set.fields[key].blank? | ||
| admin_data.update!(admin_data_values) |
There was a problem hiding this comment.
#update! will persist the changes to the database, but I think the purpose of this method is just to set the values. The changes will be persisted by #save_aapb_admin_data. I recommend using #assign_attributes
bkiahstroud
approved these changes
Jun 29, 2024
afred
added a commit
that referenced
this pull request
Aug 20, 2024
* Refactors transaction step for saving admin data 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. * Handles non-multiple, non-blank (default) case Co-authored-by: Kiah Stroud <[email protected]> * Add debug log for when nothing gets written Co-authored-by: Kiah Stroud <[email protected]> * Enumerates over SERIALIZED_FIELDS explicitly rather than duck typing * Another refactor/simplification Uses AdminData.attributes_for_updates as a filter to the change set fields. Filters out blank sony ci ids so they don't get serialized. Adds validation routine to AdminData for guarding against fields that cannot be deleted once set: bulkrax_importer_id and batch_ingest_batch_id. These IDs are foreign keys and if they accidentally get unset, we loase relational integrity, so need to make sure they are not accidentally getting unset via ingest, or updating Assets via UI, or anywhere else. * adds missing code that was supposed to be part of the last commit 🤦, please squash when merging and remove this msg. * Do not persist data in CreateAapbAdminData#set_admin_data_attributes --------- Co-authored-by: Kiah Stroud <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.