⚙️ Temporarily disable batch_upload feature#7197
Conversation
This commit prevents admin users from enabling batch_upload in settings. This addresses the issue that batch uploads and Valkyrized works are incompatible. The current Batch Add uses a fake Active Fedora form with fixed terms that ignore the selected work type, causing Valkyrie validations to fail (especially on 6.2). It was determined that fixing this issue is low-priority compared to general Bulkrax improvements. To ensure users are not enabling broken features, batch_upload is disabled until further decisions are made. Ref: - notch8/palni_palci_knapsack#479
Test Results 13 files + 12 13 suites +12 2h 55m 21s ⏱️ + 2h 55m 21s Results for commit 59edc8a. ± Comparison against base commit 1ed93ef. ♻️ This comment has been updated with latest results. |
The longer explanation about the disabled feature and accompanying URL causes a long line error in Rubocop. Ignoring the offense. Ref: - notch8/palni_palci_knapsack#479
There was a problem hiding this comment.
Pull Request Overview
Temporarily disables the batch_upload feature to prevent admins from enabling a known-broken workflow with Valkyrized works. The change introduces a custom Flipflop strategy to force the feature off, updates the feature description with guidance toward Bulkrax, styles the feature row to appear disabled in the admin UI, and skips related feature specs.
- Added DisableFeatureStrategy to always return disabled for batch_upload.
- Updated feature configuration and description with HTML notice.
- Skipped affected batch edit specs (though current skip usage is flawed).
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| config/features.rb | Adds disabling strategy and updates batch_upload description. |
| app/strategies/hyrax/strategies/disable_feature_strategy.rb | Implements custom Flipflop strategy to force-disable batch_upload. |
| app/assets/stylesheets/hyrax/_hyrax.scss | Imports new admin_features stylesheet. |
| app/assets/stylesheets/hyrax/_admin_features.scss | Styles batch_upload feature row to appear disabled. |
| spec/features/batch_edit_spec.rb | Skips tests dependent on disabled feature (implementation issue present). |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Batch uploads have been disabled since batch uploads and Valkyrized works are incompatible. The current Batch Add uses a fake Active Fedora form with fixed terms that ignore the selected work type, causing Valkyrie validations to fail (especially on 6.2). It was determined that fixing this issue is low-priority compared to general Bulkrax improvements. This commit skips the batch_upload tests until further decisions are made. Ref: - #7185 - notch8/palni_palci_knapsack#479
sjproctor
force-pushed
the
i479-disable-batch-upload-feature
branch
from
b0dd16f to
59edc8a
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| feature :batch_upload, | ||
| default: false, | ||
| description: "This functionality has been deprecated. Please use the bulk importer feature (Bulkrax) to perform this function moving forward." | ||
| description: "<strong>NOTICE:</strong> This feature has been temporarily disabled. To add or upload works in bulk, please use the <a href='https://github.com/samvera/bulkrax/wiki' target='_blank'>Bulkrax importer</a>.".html_safe |
There was a problem hiding this comment.
Using .html_safe on a string containing HTML markup can introduce XSS vulnerabilities. Consider using a safer approach like Rails' content_tag helper or ensure the HTML is properly sanitized.
| def enabled?(feature) | ||
| # Set batch upload to disabled | ||
| return false if feature == :batch_upload | ||
| # Return nil to pass through to next strategy for other features | ||
| nil | ||
| end | ||
| end |
There was a problem hiding this comment.
[nitpick] Hard-coding the feature name :batch_upload makes this strategy inflexible. Consider making it configurable by accepting a list of features to disable, or using a constant to avoid magic values.
| def enabled?(feature) | |
| # Set batch upload to disabled | |
| return false if feature == :batch_upload | |
| # Return nil to pass through to next strategy for other features | |
| nil | |
| end | |
| end | |
| DISABLED_FEATURES = [:batch_upload].freeze | |
| def enabled?(feature) | |
| # Disable features listed in DISABLED_FEATURES | |
| return false if DISABLED_FEATURES.include?(feature) | |
| # Return nil to pass through to next strategy for other features | |
| nil | |
| end |
There was a problem hiding this comment.
I do like this concept, but if the text is hardcoded, then it doesn't make a lot of sense to do this.
| // Modify the batch_upload feature flipper to be disabled | ||
| tr[data-feature="batch-upload"] { |
There was a problem hiding this comment.
[nitpick] The CSS selector uses a hard-coded feature name 'batch-upload' that should match the feature key. Consider using a CSS class or data attribute that can be dynamically applied based on disabled features to make this more maintainable.
| // Modify the batch_upload feature flipper to be disabled | |
| tr[data-feature="batch-upload"] { | |
| // Apply disabled styling to any feature row marked as disabled | |
| tr.feature-disabled { |
laritakr
left a comment
laritakr
left a comment
There was a problem hiding this comment.
I don't see a strong reason to make it configurable/flexible at this point.
|
@sjproctor Sorry if I'm missing it. Friendly reminder to make a PR against main with these changes and close this PR. 🙏🏿 thanks! |
This change prevents admin users from enabling batch_upload in settings. This addresses the issue that batch uploads and Valkyrized works are incompatible. The current Batch Add uses a fake Active Fedora form with fixed terms that ignore the selected work type, causing Valkyrie validations to fail (especially on 6.2).
It was determined that fixing this issue is low-priority compared to general Bulkrax improvements. To ensure users are not enabling broken features, batch_upload is disabled until further decisions are made.
Ref:
@samvera/hyrax-code-reviewers