Update media moderation view #4169

obulat · 2024-04-19T10:59:30Z

Fixes

Description

This PR updates the reporting view in line with the implementation plan requirements.

The media moderation view is split into 3 sections:

current report details
media item details
other reports for the media item

The IP says to use TabularInline for the other reports, however, it's not possible to do so because a TabularInline can be used for displaying the child models on the parent admin page¹. The other reports for the media item, on the other hand, display the children of the parent of the admin page model (Child (report) -> Parent (media) -> Children (reports)). That's why I used a separate function to build a table of other reports.

Screenshots

Testing Instructions

Run the app using just up.
Go to http://localhost:50280/admin and log in using the local credentials (user: deploy user, password: deploy)
Add several reports for images and audio. Add several reports for the same item.
Then click on change report. You should see the updated form that contains a blurred image or an audio, as well as other parts of the report.
Check that add report/change report/view report views work correctly.

Checklist

My pull request has a descriptive title (not a vague title likeUpdate index.md).
My pull request targets the default branch of the repository (main) or a parent feature branch.
My commit messages follow best practices.
My code follows the established code style of the repository.
I added or updated tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no visible errors.
I ran the DAG documentation generator (if applicable).

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

https://docs.djangoproject.com/en/5.0/ref/contrib/admin/#inlinemodeladmin-objects ↩

AetherUnbound

This is fantastic! I was able to test this locally and have things behave exactly as expected. I have a few comments about how we're setting the domain that might require changes.

api/api/admin/__init__.py

AetherUnbound · 2024-04-22T22:16:38Z

api/api/admin/__init__.py

+        openverse_url = (
+            f"https://openverse.org/{self.media_type}/{obj.media_obj.identifier}"
+        )


Shouldn't this use settings.CANONICAL_ORIGIN instead?
https://github.com/WordPress/openverse/blob/520cd7ab15897f33f16cca344b03ae15a687318d/api/conf/settings/security.py#L23-L22

Edit: realizing now this is not the API URL but rather the frontend one - I think it would be good for us to have a similar setting we could use for this, that way we could also point this to a locally running version of the frontend.

AetherUnbound · 2024-04-22T22:17:16Z

api/api/admin/__init__.py

+        )
+        return mark_safe(
+            f"<p>{create_link(obj.media_obj.foreign_landing_url, obj.media_obj.provider)}, "
+            f'{create_link(openverse_url, "openverse.org")}'


Same note for here (or different language for the link name might be better).

AetherUnbound · 2024-04-22T22:20:26Z

api/api/admin/__init__.py

+        Use the image thumbnail if available, else replace with the direct image url.
+        """
+        if obj.media_obj.url:
+            thumb_url = f"https://api.openverse.engineering{obj.media_url()}thumb/"


We will definitely want to use settings.CANONICAL_DOMAIN here!

It should use request.build_absolute_uri and reverse, actually. Together they make both settings references and string interpolation completely unnecessary, and inherently validates that routes actually exist (e.g., if they got renamed or something caused a typo). We can avoid build_absolute_uri if we just use reverse and rely on the relative link, which you've done elsewhere in this PR, Olga. The relative link should theoretically work perfectly fine.

AetherUnbound · 2024-04-23T00:40:13Z

api/api/admin/__init__.py

+            creator_string = f" by {creator}" if creator else ""
+            return f"{title}{creator_string}"
+
+    def media_display(self, obj):


Should this be an @abstractmethod?

sarayourfriend

Requesting changes to move the HTML building into the templates, rather than using string concatenation.

Things look good from a functionality perspective, so it's really just moving the HTML code into a place that is easier to read, reason about, and maintain, plus is the canonical "Django-way".

I've left a couple other clarifying comments about some things I didn't understand about the implementation, and +1'd some of Madison's comments, but overall looking very good.

api/api/admin/__init__.py

sarayourfriend · 2024-04-23T03:25:01Z

api/api/admin/__init__.py

+        if not reports:
+            return ""
+        result = "<table><thead><tr><th>Date</th><th>Report reason</th><th>Status</th><th>Report link</th></tr></thead><tbody>"
+        for report in reports:
+            report_link_html = f'<a href="{reverse("admin:api_imagereport_change", args=[report.id])}">{report.id}</a>'
+            created_at = report.created_at.strftime("%Y-%m-%d %H:%M:%S")
+            report_row = f"<tr><td>{created_at}</td><td>{report.reason}</td><td>{report.status}</td><td>{report_link_html}</td></tr>"
+            result += report_row
+        result += "</tbody></table>"
+        return mark_safe(result)


This is really not the right way to do things in Django. Most important issue being that this string concatenation approach does not get any benefit from Django's template cache. A particularly important benefit, regarding maintainability, is the separation of view from model, which is essential for making the data model an understandable and distinctly separate thing from the presentation. Extracting all the presentation logic in the template forces us to write admin pages that cleanly silo the data model into template context, rather than creating functions like this where the two are meshed together such that it makes it unnecessarily harder to understand. That's even the more important benefit, in our context, is to avoid this approach because it is so much harder to read, reason about, and maintain than Django templates.

Please update the change_form template you've added in this PR to render the table in the template. It's also worth checking out Django's form building tools, if you're not familiar with them, as there are canonical ways to do just about everything: https://docs.djangoproject.com/en/5.0/topics/forms/#the-template and https://docs.djangoproject.com/en/5.0/ref/contrib/admin/#overriding-admin-templates (the latter of which you've almost definitely seen, given the template override you've already added, but including it just in case and for other reviewers who might not be familiar with Django, like Madison).

api/api/admin/__init__.py

sarayourfriend · 2024-04-23T03:34:49Z

api/api/admin/__init__.py

+            thumb_url = f"https://api.openverse.engineering{obj.media_url()}thumb/"
+            return mark_safe(
+                f'<div class="container"><img src="{thumb_url}" alt="Media Image" class="blur-image" height="300" '
+                f'onclick="toggleBlur(this)" onerror="replace(this, \'{obj.media_obj.url}\')" style="cursor: pointer;" />'


What is replace doing here in the onerror? The implementation of the function doesn't clear anything up to me. Why would the img element's src ever need to change to point to a non-image URL?

By default, we use the thumbnail url. However, especially locally, this URL might not contain an image: some of the sample data identifiers do not exist in the production database. So, for local testing, we try loading the thumbnail url as the image src. If that cannot be loaded due to image not being available at that URL, we try loading the image's direct URL, which most likely exists.
This behavior mirrors the behavior of the frontend single result image, where we try to load the direct image URL, and if that is not available - we load the thumbnail.

We should update the sample data to only contain data that exists in production, even if the links are dead - at least we should have the items for all identifiers in the sample data - but that's a different issue.

Ohhhh I see, I totally misread obj.media_obj.url, and interpreted it as the API route for the image rather than the upstream image URL.

Okay, so I understand this now, thanks for clarifying. The other question is then... do we need to do anything to make sure this works as expected for both images and audio (audio thumbnail doesn't proxy .url, I think? 🤔)

obulat · 2024-04-23T05:09:37Z

Drafting this PR to address @sarayourfriend 's change requests.

AetherUnbound

Some small issues/bugs I've noticed, but this is really close!

api/api/admin/media_report.py

AetherUnbound · 2024-04-26T20:36:53Z

api/api/admin/media_report.py

+            additional_data["thumb_url"] = (
+                f"https://api.openverse.engineering{obj.media_url()}thumb/"
+            )


Just noting that I think we'll still want to use the methods Sara described here for this URL building.

If done in the template, it's a bit less fussy to just use the url filter: https://docs.djangoproject.com/en/5.0/ref/templates/builtins/#url

AetherUnbound · 2024-04-26T20:49:42Z

api/api/templates/admin/api/media_report/change_form.html

+      <div class="flex-container">
+        <label>Tags</label>
+        <div class="readonly">
+          <p>{{ tags }}</div>


This is a bit of a nit, but this just shares the dictionary that's built up of the tags. Would it be possible to render this in a more friendly manner, separating each group of tags by source?

obulat · 2024-04-29T16:51:41Z

I refactored the changes to extract the data in the python code, and render it only in the template. The urls are also built in the template.

AetherUnbound

Two small notes, this is very close!

api/api/templates/admin/api/media_report/change_form.html

AetherUnbound · 2024-04-29T22:49:46Z

api/api/templates/admin/api/media_report/change_form.html

+      </tbody>
+    </table>
+  {% else %}
+    <p>No other reports for <a href="{{reverse_media_url}}">{{ identifier }}</a> found.</p>


When I click on this for a report, I get redirected to the admin home page with this message 🤔

The URL it's trying to go to is something like http://localhost:50280/admin/api/image/de37812d-2daa-49f6-91ab-e87e780ca653/change/, is it possibly because the /change/ URL isn't available for images? Should something else be used here instead?

The IP does not require us to add a link to a media admin change page, so I thought it's best to remove it completely.
The URL for image change view is different than what's returned by reverse_media_url, something like http://localhost:50280/admin/api/image/4347/change/?_changelist_filters=q%3Dde37812d-2daa-49f6-91ab-e87e780ca653. I have no idea where the 4347 comes from.

That'll be the database id, primary key column.

sarayourfriend

I've left some comments, and I am approving despite the issues @AetherUnbound identified. My sense about the context using media_obj being clearer is strong, but it isn't a blocker, and we can refactor this after the fact without issue. I don't want to prolong this PR's review on these problems.

The problems Madison identified are indeed problems, but I feel comfortable saying we can address them in fast follow issues, rather than in this PR, and it won't cause harm to the future issues in this milestone that depend on this baseline implementation existing. In the interest of unblocking that work, and recognising this PR has already been long in review, I'd prefer you created follow-up issues for those, and we merged this either as-is or with only minor modifications (up to you how much).

@AetherUnbound deferring to you on how much you feel are definite blockers vs things that can be fixed in fast-follows.

api/api/admin/media_report.py

api/api/templates/admin/api/media_report/change_form.html

sarayourfriend · 2024-04-29T23:29:25Z

api/api/templates/admin/api/media_report/change_form.html

+      <div class="flex-container">
+        <label>Title and creator</label>
+        <div class="readonly">
+          <p>{{ title }} {% if creator_url %}by <a href="{{creator_url}}">{{creator}}</a> {% else %} {{creator}} {% endif %} </p></div>


Can we lean on the attribution generation here to make things easier? It already resolves the creator_url/creator resolution, printing the title, source, etc.

Apparently, we don't have a function to get the html attribution, only the text one that does not add creator and work urls.
For some reason, I was under the impression that all the items mentioned in the IP need to be on separate lines. Now, I updated the form to display the media details more compactly. For attribution, I created a new sub-template.

api/api/templates/admin/api/media_report/change_form.html

Signed-off-by: Olga Bulat <obulat@gmail.com>

Co-authored-by: Madison Swain-Bowden <bowdenm@spu.edu>

Signed-off-by: Olga Bulat <obulat@gmail.com>

obulat · 2024-04-30T05:23:21Z

I moved as much as possible to the templates, simplified the context object passed to the template, removed the link to the reverse image admin page when there were no reports (it wasn't in the IP, and the link to media change form is incorrect).

Here's an updated form:

sarayourfriend · 2024-04-30T05:25:31Z

Looks awesome, @obulat

AetherUnbound

Fantastic, I'm with @sarayourfriend on the other changes, those can be done in follow up if/as necessary. Thanks so much Olga, this looks excellent!

github-actions bot added the 🧱 stack: api Related to the Django API label Apr 19, 2024

openverse-bot added 🟧 priority: high Stalls work on the project or its dependents ✨ goal: improvement Improvement to an existing user-facing feature 🕹 aspect: interface Concerns end-users' experience with the software labels Apr 19, 2024

obulat force-pushed the add/improved-media-moderation-view branch 5 times, most recently from aab8c29 to d1d6158 Compare April 22, 2024 08:16

obulat self-assigned this Apr 22, 2024

obulat marked this pull request as ready for review April 22, 2024 09:53

obulat requested review from a team as code owners April 22, 2024 09:53

obulat requested review from krysal and AetherUnbound April 22, 2024 09:53

obulat force-pushed the add/improved-media-moderation-view branch from a9e8e3d to bbb7633 Compare April 22, 2024 11:16

AetherUnbound requested changes Apr 23, 2024

View reviewed changes

sarayourfriend requested changes Apr 23, 2024

View reviewed changes

obulat force-pushed the add/improved-media-moderation-view branch 2 times, most recently from 6737f09 to e5ecb1a Compare April 23, 2024 05:02

obulat marked this pull request as draft April 23, 2024 05:09

obulat force-pushed the add/improved-media-moderation-view branch from e5ecb1a to d9c67c2 Compare April 25, 2024 11:37

sarayourfriend mentioned this pull request Apr 25, 2024

Incorporate djlint to lint HTML templates #4210

Open

obulat force-pushed the add/improved-media-moderation-view branch from d9c67c2 to 2f37695 Compare April 26, 2024 12:43

obulat marked this pull request as ready for review April 26, 2024 12:43

AetherUnbound requested changes Apr 26, 2024

View reviewed changes

obulat force-pushed the add/improved-media-moderation-view branch from 1791a80 to 38726b3 Compare April 29, 2024 16:13

obulat requested a review from sarayourfriend April 29, 2024 16:51

obulat requested a review from AetherUnbound April 29, 2024 16:51

AetherUnbound reviewed Apr 29, 2024

View reviewed changes

sarayourfriend approved these changes Apr 29, 2024

View reviewed changes

obulat force-pushed the add/improved-media-moderation-view branch from 38eb70d to 24a4a5b Compare April 30, 2024 03:54

obulat and others added 9 commits April 30, 2024 08:19

Update media moderation view

7c23382

Signed-off-by: Olga Bulat <obulat@gmail.com>

Add has_sensitive_text

9863dff

Signed-off-by: Olga Bulat <obulat@gmail.com>

Move media report admins to a separate file

e0e5ff6

Signed-off-by: Olga Bulat <obulat@gmail.com>

Move form html to the template

777b848

Signed-off-by: Olga Bulat <obulat@gmail.com>

Update api/api/admin/media_report.py

79748dd

Co-authored-by: Madison Swain-Bowden <bowdenm@spu.edu>

Remove left-over templates from python file

330c365

Signed-off-by: Olga Bulat <obulat@gmail.com>

Fix thumb and report url; tags display

3bcb077

Signed-off-by: Olga Bulat <obulat@gmail.com>

Reorder code

953c54b

Signed-off-by: Olga Bulat <obulat@gmail.com>

Add changes from code review

f6f942d

Signed-off-by: Olga Bulat <obulat@gmail.com>

obulat force-pushed the add/improved-media-moderation-view branch from 841e76f to f6f942d Compare April 30, 2024 05:20

obulat requested a review from AetherUnbound April 30, 2024 05:26

AetherUnbound approved these changes Apr 30, 2024

View reviewed changes

obulat merged commit 1d875c8 into main Apr 30, 2024
48 checks passed

obulat deleted the add/improved-media-moderation-view branch April 30, 2024 05:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update media moderation view #4169

Update media moderation view #4169

obulat commented Apr 19, 2024 •

edited

Loading

AetherUnbound left a comment

AetherUnbound Apr 22, 2024

AetherUnbound Apr 22, 2024

AetherUnbound Apr 22, 2024

sarayourfriend Apr 23, 2024 •

edited

Loading

AetherUnbound Apr 23, 2024

sarayourfriend left a comment

sarayourfriend Apr 23, 2024

sarayourfriend Apr 23, 2024

obulat Apr 23, 2024

sarayourfriend Apr 23, 2024

obulat commented Apr 23, 2024

AetherUnbound left a comment

AetherUnbound Apr 26, 2024

sarayourfriend Apr 27, 2024

AetherUnbound Apr 26, 2024

obulat commented Apr 29, 2024

AetherUnbound left a comment

AetherUnbound Apr 29, 2024

obulat Apr 30, 2024

sarayourfriend Apr 30, 2024

sarayourfriend left a comment

sarayourfriend Apr 29, 2024

obulat Apr 30, 2024

obulat commented Apr 30, 2024

sarayourfriend commented Apr 30, 2024

AetherUnbound left a comment

Update media moderation view #4169

Update media moderation view #4169

Conversation

obulat commented Apr 19, 2024 • edited Loading

Fixes

Description

Screenshots

Testing Instructions

Checklist

Developer Certificate of Origin

Footnotes

AetherUnbound left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarayourfriend Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarayourfriend left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obulat commented Apr 23, 2024

AetherUnbound left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obulat commented Apr 29, 2024

AetherUnbound left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarayourfriend left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obulat commented Apr 30, 2024

sarayourfriend commented Apr 30, 2024

AetherUnbound left a comment

Choose a reason for hiding this comment

obulat commented Apr 19, 2024 •

edited

Loading

sarayourfriend Apr 23, 2024 •

edited

Loading