Feature: Optionally make embargo reason required#12067
Conversation
cmbz
added
FY26 Sprint 14
| if(json.containsKey("reason")) { | ||
| reason = json.getString("reason"); | ||
| } | ||
| if(reason == null && FeatureFlags.REQUIRE_EMBARGO_REASON.enabled()) { |
There was a problem hiding this comment.
Minor point, but would we want to check for blank ((null or blank) and flag) here when the reason is required to return the reason is required message?
There was a problem hiding this comment.
The next check is for blank - just a slightly different error message.
| } else if(reason != null && reason.isBlank()) { | ||
| return error(Status.BAD_REQUEST, "Reason cannot be blank (whitespace only)"); | ||
| } | ||
| embargo.setReason(reason); |
There was a problem hiding this comment.
I guess it's out of scope for this, but did you consider using a command? (To do permission checks and even apply the FileUtil validation method.)
There was a problem hiding this comment.
For this PR I was just following the existing code, but yeah, that could have been done in a command. I'm not sure I understand about the FileUtil method - that seems pretty tied to JSF.
| embargo.date.invalid=Date is outside the allowed range: ({0} to {1}) | ||
| embargo.date.required=An embargo date is required | ||
| embargo.reason.required=An embargo reason is required | ||
| embargo.reason.blank=An embargo reason cannot be blank |
There was a problem hiding this comment.
These could be used in the Datasets method
| import static org.mockito.Mockito.when; | ||
|
|
||
| @LocalJvmSettings | ||
| public class DatasetsEmbargoAPITest { |
There was a problem hiding this comment.
I've been writing integration tests for my api endpoints and unit tests for commands. Beyond not having to deal with the database is there a reason for testing like this?
There was a problem hiding this comment.
My understanding is you can't easily test FeatureFlags (or their non-default value) in IT tests since we don't have a way to change the flag remotely from the test. This style of test allows changing the flag value.
There was a problem hiding this comment.
Right. I've run into issues with setting feature flags for integration tests. Thanks!
There was a problem hiding this comment.
DNO-make_embargo_reason_required
github-project-automation
Bot
moved this from In Review 🔎
to Ready for QA ⏩
in IQSS Dataverse Project
github-project-automation
Bot
moved this from QA ✅
to Merged 🚀
in IQSS Dataverse Project
7 participants
What this PR does / why we need it: This PR adds a flag that, when set, makes it required to set the reason when creating an embargo, via the UI or API. In addition it adds a new check to assure that a supplied reason, whether required or not, is not blank.
Which issue(s) this PR closes:
Special notes for your reviewer:
Suggestions on how to test this:
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?:
Additional documentation: