-
Notifications
You must be signed in to change notification settings - Fork 499
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
7291 spam check #7479
7291 spam check #7479
Conversation
…r the dataverse and dataset validation. (#7291)
|
||
`curl -X PUT -d true http://localhost:8080/api/admin/settings/:ExternalValidationAdminOverride` | ||
|
||
When the override is enabled, the external check will be skipped for admin users. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should say superusers instead of admins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, better question - do we need this setting? i.e. what's the use case for not just always allowing superusers to publish?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I added a few minor comments (docs, and a test ignore).
The biggest question is the one about superusers? It seems to me that that aetting may be overkill and not needed.
I'm only "commenting" and not approving, because we don't want this to get merged just yet.
|
||
|
||
|
||
DataverseMetadataValidationFailureMsg, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if this text is in the right place.
@@ -16,6 +16,7 @@ | |||
import javax.xml.parsers.DocumentBuilder; | |||
import javax.xml.parsers.DocumentBuilderFactory; | |||
import static junit.framework.Assert.assertEquals; | |||
import org.junit.Ignore; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was this meant to be checked in?
|
||
`curl -X PUT -d true http://localhost:8080/api/admin/settings/:ExternalValidationAdminOverride` | ||
|
||
When the override is enabled, the external check will be skipped for admin users. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, better question - do we need this setting? i.e. what's the use case for not just always allowing superusers to publish?
Merge branch 'develop' into 7291-spam-check
… to the exported metadata fragment. (it's already in prod., in the current v5.3 spam patch; but wasn't checked in in the branch). (#7291)
Resolving merge conflicts - straightforward enough (#7291)
Closing. This has been superseded by pull request #8155. |
What this PR does / why we need it:
An anti-spam mod that prevents sus datasets and dataverses from being published. See the issue for more info. The plan is to throw this on our prod. servers shortly in order to gauge the real life usefulness of the solution. We are likely going to revise it before adding this to an official release.
(but this brings up a question, do we even want to merge this into develop at all?)
Which issue(s) this PR closes:
Closes #7291
Special notes for your reviewer:
Please keep in mind that in its current form this is intended for local use only. Hence no documentation in the official guide etc.
Suggestions on how to test this:
Let's talk once it goes to QA.
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: