Conversation
Why these changes are being introduced: The DSpace 8 REST API has updated metadata formatting requirements which will require updates to ETD. Relevant ticket(s): - [ETD-685](https://mitlibraries.atlassian.net/browse/ETD-685) How this addresses that need: - Adds a new Flipflop feature flag, dspace_v8_metadata, defaulting to `DSPACE_V8_METADATA=false` in `features.rb`. When enabled, the feature serializes metadata in the new format. - Refactors metadata construction in dspace_metadata.rb to use explicit metadata entries for clearer serialization. Side effects of this change: Setting the default behavior to v6 will require some cleanup once we fully launch DSpace 8. This seemed like a reasonable tradeoff, given that we are still using v6.
There was a problem hiding this comment.
Pull request overview
This PR adds a feature-flagged switch to support the DSpace 8 REST API metadata payload format while preserving the existing DSpace 6 “metadata array” serialization as the default behavior.
Changes:
- Added a Flipflop feature flag (
dspace_v8_metadata) driven byDSPACE_V8_METADATA. - Refactored
DspaceMetadatato build metadata as explicit entries and serialize to either DSpace 6 or DSpace 8 formats based on the flag. - Added test coverage for both serialization formats and documented the new ENV var.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
app/models/dspace_metadata.rb |
Refactors metadata construction and adds conditional serialization for DSpace 6 vs DSpace 8. |
config/features.rb |
Introduces dspace_v8_metadata Flipflop feature defaulted from DSPACE_V8_METADATA. |
test/models/dspace_metadata_test.rb |
Adds tests verifying both serialization formats and ensures existing tests run with the flag off. |
app.json |
Adds DSPACE_V8_METADATA to Heroku app env defaults. |
README.md |
Documents the new DSPACE_V8_METADATA environment variable. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| default: ENV.fetch('MAINTENANCE_MODE', false), | ||
| description: "Put application in maintenance mode, disabling file transfer uploads." | ||
|
|
||
| feature :dspace_v8_metadata, | ||
| default: ENV.fetch('DSPACE_V8_METADATA', false), |
There was a problem hiding this comment.
ENV.fetch returns strings, so when DSPACE_V8_METADATA is set to "false" (as in app.json) the feature default becomes the string "false" rather than a boolean. To avoid the flag being interpreted incorrectly, cast this ENV value to a boolean (e.g., via ActiveModel::Type::Boolean.new.cast(...)) before passing it as the Flipflop default: (and consider doing the same for MAINTENANCE_MODE for consistency).
| default: ENV.fetch('MAINTENANCE_MODE', false), | |
| description: "Put application in maintenance mode, disabling file transfer uploads." | |
| feature :dspace_v8_metadata, | |
| default: ENV.fetch('DSPACE_V8_METADATA', false), | |
| default: ActiveModel::Type::Boolean.new.cast(ENV.fetch('MAINTENANCE_MODE', false)), | |
| description: "Put application in maintenance mode, disabling file transfer uploads." | |
| feature :dspace_v8_metadata, | |
| default: ActiveModel::Type::Boolean.new.cast(ENV.fetch('DSPACE_V8_METADATA', false)), |
There was a problem hiding this comment.
This is a good reminder that booleans don't work in FlipFlop...but, I'm not sure casting the value as a boolean would, either? I'll have to test this out.
There was a problem hiding this comment.
FWIW I'm sort of fine either way. The way you had it is how everything else in the app probably already works so it's arguably better to not "fix" it if the outcome is different behavior than elsewhere.
| `DSPACE_V8_METADATA` - Toggle metadata format for publication payloads. Set to `true` for DSpace 8 format. | ||
| set to `false` for DSpace 6 `metadata` array format. Default is `false`. |
There was a problem hiding this comment.
The new DSPACE_V8_METADATA documentation is split across two lines, and the second line starts with lowercase "set" which reads like a sentence fragment in the rendered README. Consider combining into one bullet/paragraph and capitalizing "Set" to keep the ENV list formatting consistent.
| `DSPACE_V8_METADATA` - Toggle metadata format for publication payloads. Set to `true` for DSpace 8 format. | |
| set to `false` for DSpace 6 `metadata` array format. Default is `false`. | |
| `DSPACE_V8_METADATA` - Toggle metadata format for publication payloads. Set to `true` for DSpace 8 format, set to `false` for DSpace 6 `metadata` array format. Default is `false`. |
JPrevost
left a comment
There was a problem hiding this comment.
I added a comment about some method documentation I think would help, but this looks good.
| @metadata_entries | ||
| end | ||
|
|
||
| def serialize_dspace8 |
There was a problem hiding this comment.
Would you mind adding comments to serialize_dspace8 and serialize_dspace6 explaining the different formats. When we ultimately refactor to only serialize_dspace8 I'd like to keep the explanation of what is going on as the data model is very odd (to me) and it feels like some docs here explaining the intent and maybe even showing a simple example would go a long way to help us remember wtf is going on in the future.
Why these changes are being introduced:
The DSpace 8 REST API has updated metadata formatting requirements which will require updates to ETD.
Relevant ticket(s):
How this addresses that need:
DSPACE_V8_METADATA=falseinfeatures.rb. When enabled, the feature serializes metadata in the new format.Side effects of this change:
Setting the default behavior to v6 will require some cleanup once we fully launch DSpace 8. This seemed like a reasonable tradeoff, given that we are still using v6.
Developer
our guide and
all issues introduced by these changes have been resolved or opened as new
issues (link to those issues in the Pull Request details above)
Code Reviewer
(not just this pull request message)
Requires database migrations?
NO
Includes new or updated dependencies?
NO