-
Notifications
You must be signed in to change notification settings - Fork 204
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
Drop status
column from media reports
#4590
Conversation
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.
LGTM, except there are usages of status
and the status choice constants that we should also remove.
If you search for status=
in test_media_report.py
there are three more related to the report factory usage that should be removed.
After that it should be possible to also remove the status choices constants from api/models/media.py.
@@ -41,7 +40,7 @@ def test_pending_reports_have_no_subreport_models( | |||
media = media_type_config.model_factory.create() | |||
report = media_type_config.report_factory.create(media_obj=media, reason=reason) | |||
|
|||
assert report.status == PENDING | |||
assert report.decision is None |
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.
I suppose this can also be assert report.is_pending
, to be more directly relevant to the previous expression.
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.
Good observation! Changes applied.
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 like this might have gotten mixed up in commits? Left a comment with suggested change.
8872977
to
81630b6
Compare
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.
LGTM! Looks like maybe a small mix up with commits and a commented assertion, but an easy fix and non-blocker really.
@@ -41,7 +40,7 @@ def test_pending_reports_have_no_subreport_models( | |||
media = media_type_config.model_factory.create() | |||
report = media_type_config.report_factory.create(media_obj=media, reason=reason) | |||
|
|||
assert report.status == PENDING | |||
assert report.decision is None |
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 like this might have gotten mixed up in commits? Left a comment with suggested change.
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.
LGTM, other than the comment Sara noticed 👍
Co-authored-by: sarayourfriend <[email protected]>
This PR has migrations. Please rebase it before merging to ensure that conflicting migrations are not introduced. |
Fixes
Fixes #3642 by @sarayourfriend
Description
As title says, the column is removed, making the
AbstractMediaReport.STATUS_CHOICES
unnecessary, so they're also removed.Testing Instructions
Try creating a new report to confirm it still works.
Checklist
Update index.md
).main
) or a parent feature branch../ov just catalog/generate-docs
for catalog PRs) or the media properties generator (./ov just catalog/generate-docs media-props
for the catalog or./ov just api/generate-docs
for the API) where applicable.Developer Certificate of Origin
Developer Certificate of Origin