-
Notifications
You must be signed in to change notification settings - Fork 219
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
mature
-> sensitive
code changes for the API
#3769
Conversation
Also correctly reference mature vs sensitive depending on what type of object is being handed into the serializer
This PR has migrations. Please rebase it before merging to ensure that conflicting migrations are not introduced. |
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.
The code looks good and seems to work well.
I was testing this locally, and think I found an error with our local setup, or with this PR.
If you open http://0.0.0.0:50280/v1/images/41c8f114-f77d-45bc-b1ec-55c205f96e9f/
, you should see the an item with sensitive_text
, but the value there is an empty array. And this item is in both filtered and main indices, even though it's supposed to only be in the main one (due to sensitive term).
I don't remember how we set up the filtered index locally, and how we add the sensitivities
in the API: is that the local setup problem?
@obulat I tested that result on |
Ah, right! It's because we use a mock sensitive terms list: https://github.com/WordPress/openverse/blob/7427bbd4a8178d05a27e6fef07d70905ec7ef16b/ingestion_server/ingestion_server/static/mock_sensitive_terms.txt |
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.
The code looks good. I have a question similar to Olga's.
we use a mock sensitive terms list: 7427bbd/ingestion_server/ingestion_server/static/mock_sensitive_terms.txt
Does that means that items with those terms should be marked as sensitive? I searched for them but I couldn't find any. How can we check for an item where "mature": true
?
I'm not sure! I believe it's separate from the work on this PR though; @krysal or @obulat would either of you be willing to look into it this week to see if/how that mock list is being used? |
Yes, anything with those terms locally will get marked with having sensitive text. Compare the following two requests: http://0.0.0.0:50280/v1/images/?q=water&unstable__include_sensitive_results=True All results with
None of these exist in the testing data. That's why the one @obulat brought up isn't matched, it has none of the mock terms, and isn't marked mature in the index. To test for |
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.
Tested locally: added a mature report for an image, searched for it, checked that it has correct values in the sensitivities array. Tested that both mature
and unstable__include_sensitive_results
query parameters work.
@AetherUnbound, are there any special considerations for deploying this PR? If so, could you add a note for the next deployer?
No special considerations here! It will run the two no-op migrations on deployment, but nothing about the application itself should change 😄 I'll confirm it works in staging before deploying though. |
Based on the medium urgency of this PR, the following reviewers are being gently reminded to review this PR: @krysal Excluding weekend1 days, this PR was ready for review 13 day(s) ago. PRs labelled with medium urgency are expected to be reviewed within 4 weekday(s)2. @AetherUnbound, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings. Footnotes
|
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.
Continues to work as expected! Thanks for reminding us of the testing instructions, @sarayourfriend, and great work, @AetherUnbound!
Going to merge and make sure it works in staging! |
Fixes
Fixes #3723 by @AetherUnbound
Description
This PR applies the code changes for the API necessary to use
sensitive
internally rather thanmature
. Per the implementation plan linked in #3723, themature
property on the serializer and themature
property of Elasticsearch hits are unaffected.This PR requires 2 migrations in order to appease Django. Both are no-ops:
mature
tosensitive
Below are the
sqlmigrate
outputs for each migration:Testing Instructions
Tests should pass, and all routes should also work locally!
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin