Skip to content
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

Enable soft-locking functionality for media reports to assist simultaneous moderators #4374

Closed
wants to merge 23 commits into from
Closed
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
12b7190
Create a lock manager class to encapsulate locking utilities
dhruvkb May 23, 2024
a9c976c
Create admin endpoints for soft locking/unlocking
dhruvkb May 23, 2024
6bc0e6b
Highlight works that are in-moderation in the list view
dhruvkb May 23, 2024
385690d
Add a message if another moderator is looking at the same report
dhruvkb May 23, 2024
b97ef4e
Call the lock/unlock endpoints based on client events
dhruvkb May 23, 2024
2168445
Fix case of "API" in the Django Admin UI
dhruvkb May 23, 2024
d97dbc5
Handle failure to connect to Redis
dhruvkb May 23, 2024
88f9863
Add function to get score for a username-object pair
dhruvkb May 23, 2024
c2e373a
Add tests for `LockManager`
dhruvkb May 23, 2024
2d08af0
Use singular lock from usernames to report IDs
dhruvkb May 24, 2024
9a9ffad
Create a function to get all moderators
dhruvkb May 24, 2024
58ba5f7
Avoid using `KEYS` because of bad performance
dhruvkb May 24, 2024
a5632e9
Use very short TTL for locks
dhruvkb May 24, 2024
2e7c59e
Remove `self.redis` in favour of getting connections when needed
dhruvkb May 24, 2024
53368df
Remove need for unlocking as TTL is very short
dhruvkb May 24, 2024
26cd1c6
Simplify the Redis handler decorator
dhruvkb May 24, 2024
5562bbb
Send the expiration timestamp in the lock endpoint response
dhruvkb May 24, 2024
7db9145
Highlight soft-locks in single Redis query
dhruvkb May 24, 2024
00b9999
Cleanup code in `change_view`
dhruvkb May 24, 2024
e4bee71
Undo frivolous change
dhruvkb May 24, 2024
48783cc
Fix punctuation
dhruvkb May 24, 2024
a705df6
Update `soft_lock_view` API endpoint docstring
dhruvkb May 24, 2024
157457d
Merge branch 'main' into 'soft_lock'
dhruvkb May 24, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 92 additions & 2 deletions api/api/admin/media_report.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
from functools import update_wrapper
from typing import Sequence

from django.conf import settings
from django.contrib import admin
from django.contrib import admin, messages
from django.contrib.admin.views.main import ChangeList
from django.db.models import Count, F, Min
from django.http import JsonResponse
from django.urls import reverse
from django.utils.html import format_html
from django.utils.safestring import mark_safe
Expand All @@ -14,6 +16,7 @@
from openverse_attribution.license import License

from api.models import PENDING
from api.utils.moderation_lock import LockManager


logger = structlog.get_logger(__name__)
Expand Down Expand Up @@ -135,8 +138,17 @@ def get_changelist(self, request, **kwargs):


class MediaReportAdmin(admin.ModelAdmin):
change_list_template = "admin/api/media_report/change_list.html"
change_form_template = "admin/api/media_report/change_form.html"
list_display = ("id", "reason", "is_pending", "description", "created_at", "url")
list_display = (
"id",
"reason",
"is_pending",
"description",
"created_at",
"url",
"get_is_soft_locked",
)
list_filter = (
("decision", admin.EmptyFieldListFilter), # ~status, i.e. pending or moderated
"reason",
Expand All @@ -148,6 +160,53 @@ class MediaReportAdmin(admin.ModelAdmin):
actions = None
media_type = None

@admin.display(boolean=True, description="Soft locked")
def get_is_soft_locked(self, obj):
"""
Get whether this particular report is soft-locked.

This field is hidden from the rendered table, but the alt-text
the field helps to highlight the soft-locked rows.

:param obj: the report object
:return bool: whether the report is soft-locked
"""

return len(self.lock_manager.moderator_set(obj.id)) != 0

def __init__(self, *args, **kwargs):
self.lock_manager = LockManager(self.media_type)
super().__init__(*args, **kwargs)

def get_urls(self):
# Code partially copied from django/contrib/admin/options.py

from django.urls import path

def wrap(view):
def wrapper(*args, **kwargs):
return self.admin_site.admin_view(view)(*args, **kwargs)

wrapper.model_admin = self
return update_wrapper(wrapper, view)

info = self.opts.app_label, self.opts.model_name

urls = super().get_urls()
urls[-1:-1] = [
path(
"<path:object_id>/lock/",
wrap(self.soft_lock_view),
name="{:s}_{:s}_lock".format(*info),
),
path(
"<path:object_id>/unlock/",
wrap(self.soft_unlock_view),
name="{:s}_{:s}_unlock".format(*info),
),
]
return urls

def get_fieldsets(self, request, obj=None):
if obj is None:
return [
Expand Down Expand Up @@ -245,7 +304,38 @@ def _get_media_obj_data(self, obj):
logger.info(f"Additional data: {additional_data}")
return additional_data

def soft_lock_view(self, request, object_id):
"""
Add soft-locks for the current user and object ID.

This view is called from the frontend when a user loads the
change page. It is also called when the page visibility is
restored.
"""

self.lock_manager.add_locks(request.user.get_username(), object_id)
return JsonResponse({"status": "OK"})

def soft_unlock_view(self, request, object_id):
"""
Remove soft-locks for the current user and object ID.

This view is called from the frontend when the change page
visibility is lost, including the case where the user unloads
the page.
"""

self.lock_manager.remove_locks(request.user.get_username(), object_id)
return JsonResponse({"status": "OK"})

def change_view(self, request, object_id, form_url="", extra_context=None):
mods = self.lock_manager.moderator_set(object_id)
mods -= {request.user.get_username()}
if len(mods):
messages.warning(
request, f"Other moderator(s) looking at this report: {', '.join(mods)}"
)

extra_context = extra_context or {}
extra_context["media_type"] = self.media_type

Expand Down
1 change: 1 addition & 0 deletions api/api/apps.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,5 @@

class ApiConfig(AppConfig):
name = "api"
verbose_name = "API"
default_auto_field = "django.db.models.AutoField"
38 changes: 38 additions & 0 deletions api/api/templates/admin/api/media_report/change_form.html
Original file line number Diff line number Diff line change
Expand Up @@ -147,4 +147,42 @@ <h2>Other reports</h2>
img.src = url;
}
</script>

<script>
// This script tag manages the soft-locking mechanism for the report.

function toggleLock(url) {
// Toggle the soft-lock for the current user-report pair.
fetch(url, {
method: 'POST',
keepalive: true,
headers: {
'Content-Type': 'application/json',
'X-CSRFToken': document.querySelector('[name=csrfmiddlewaretoken]').value
},
})
.then(res => console.log(res))
.catch(err => console.error(err));
}

function softLock() {
toggleLock('{% url "admin:api_"|add:media_type|add:"report_lock" object_id=object_id %}')
}

function softUnlock() {
toggleLock('{% url "admin:api_"|add:media_type|add:"report_unlock" object_id=object_id %}')
}

// On first load, set the soft-lock.
document.addEventListener("DOMContentLoaded", softLock)
// Subsequent visibility changes (including unload and other ways to
// close the page) will toggle the soft-lock.
document.addEventListener("visibilitychange", function() {
dhruvkb marked this conversation as resolved.
Show resolved Hide resolved
if (document.visibilityState === 'visible') {
softLock()
} else {
softUnlock()
}
})
</script>
{% endblock %}
23 changes: 23 additions & 0 deletions api/api/templates/admin/api/media_report/change_list.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
{% extends "admin/change_list.html" %}
{% load i18n admin_urls static admin_list %}

{% block extrastyle %}
{{ block.super }}
<style>
th.column-get_is_soft_locked,td.field-get_is_soft_locked {
display: none;
}

tr:has(td.field-get_is_soft_locked>img[alt="True"]) {
background-color: var(--message-warning-bg);
}
</style>
{% endblock %}

{% block result_list %}
<p>
<strong>Note:</strong>
Reports that are being moderated by another user are highlighted in yellow.
</p>
{{ block.super }}
{% endblock %}
122 changes: 122 additions & 0 deletions api/api/utils/moderation_lock.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
import time

import django_redis
import structlog
from redis import Redis
from redis.exceptions import ConnectionError


REPORT_LOCK_PREFIX = "soft_lock_report"
MODERATOR_LOCK_PREFIX = "soft_lock_moderator"
PREFIXES = [REPORT_LOCK_PREFIX, MODERATOR_LOCK_PREFIX]

logger = structlog.get_logger(__name__)


class LockManager:
def __init__(self, media_type):
self.media_type = media_type
redis: Redis = django_redis.get_redis_connection("default")
try:
redis.ping()
self.redis = redis
except ConnectionError:
logger.error("Redis connection failed")
self.redis = None
dhruvkb marked this conversation as resolved.
Show resolved Hide resolved

def prune(self):
"""Delete all expired locks."""

def _prune(pattern: str):
now = int(time.time())
pipe = self.redis.pipeline()
for key in self.redis.keys(pattern):
for value, score in self.redis.zrange(key, 0, -1, withscores=True):
if score <= now:
logger.info("Deleting expired lock", key=key, value=value)
pipe.zrem(key, value)
pipe.execute()

for prefix in PREFIXES:
_prune(f"{prefix}:*")
dhruvkb marked this conversation as resolved.
Show resolved Hide resolved

def add_locks(self, username, object_id):
"""
Add soft-locks for a given username and report pair.

:param username: the username of the moderator viewing a report
:param object_id: the ID of the report being viewed
"""

if not self.redis:
return

object = f"{self.media_type}:{object_id}"
expiration = int(time.time()) + 5 * 60 # 5 minutes from now

pipe = self.redis.pipeline()
logger.info("Adding lock", object=object, user=username, expiration=expiration)
pipe.zadd(f"{REPORT_LOCK_PREFIX}:{object}", {username: expiration})
pipe.zadd(f"{MODERATOR_LOCK_PREFIX}:{username}", {object: expiration})
pipe.execute()

def remove_locks(self, username, object_id):
"""
Remove soft-locks for a given username and report pair.

:param username: the username of the moderator viewing a report
:param object_id: the ID of the report being viewed
"""

if not self.redis:
return

object = f"{self.media_type}:{object_id}"

pipe = self.redis.pipeline()
logger.info("Removing lock", object=object, user=username)
pipe.zrem(f"{REPORT_LOCK_PREFIX}:{object}", username)
pipe.zrem(f"{MODERATOR_LOCK_PREFIX}:{username}", object)
pipe.execute()

def moderator_set(self, object_id) -> set[str]:
"""
Get the list of moderators on a particular item.

:param object_id: the ID of the report being viewed
:return: the list of moderators on a particular item
"""

if not self.redis:
return set()

self.prune()

object = f"{self.media_type}:{object_id}"
mods = set(
item.decode("utf-8")
for item in self.redis.zrange(
f"{REPORT_LOCK_PREFIX}:{object}",
start=0,
end=-1,
)
)
logger.info("Retrieved moderators", object=object, mods=mods)
return mods

def score(self, username, object_id) -> int:
"""
Get the score of a particular moderator on a particular item.

:param username: the username of the moderator viewing a report
:param object_id: the ID of the report being viewed
:return: the score of a particular moderator on a particular item
"""

if not self.redis:
return 0

object = f"{self.media_type}:{object_id}"
score = self.redis.zscore(f"{REPORT_LOCK_PREFIX}:{object}", username)
logger.info("Retrieved score", object=object, user=username, score=score)
return score
67 changes: 67 additions & 0 deletions api/test/unit/utils/test_moderation_lock.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
from datetime import datetime, timedelta

import pytest
from freezegun import freeze_time
from redis import Redis

from api.utils.moderation_lock import LockManager


@pytest.mark.parametrize(
"is_cache_reachable, cache_name",
[(True, "redis"), (False, "unreachable_redis")],
)
def test_lock_manager_handles_missing_redis(is_cache_reachable, cache_name, request):
request.getfixturevalue(cache_name)

lm = LockManager("media_type")
lm.add_locks("test", 10)

if is_cache_reachable:
assert isinstance(lm.redis, Redis)
assert lm.moderator_set(10) == {"test"}
else:
assert lm.redis is None
assert lm.moderator_set(10) == set()


def test_lock_manager_adds_and_removes_locks():
lm = LockManager("media_type")

lm.add_locks("one", 10)
assert lm.moderator_set(10) == {"one"}
lm.add_locks("two", 10)
assert lm.moderator_set(10) == {"one", "two"}
lm.remove_locks("two", 10)
assert lm.moderator_set(10) == {"one"}


def test_relocking_updates_score(redis):
lm = LockManager("media_type")
now = datetime.now()

with freeze_time(now):
lm.add_locks("one", 10)
init_score = lm.score("one", 10)

with freeze_time(now + timedelta(minutes=2)):
lm.add_locks("one", 10)
updated_score = lm.score("one", 10)

assert updated_score == init_score + 120


def test_lock_manager_prunes_after_timeout():
lm = LockManager("media_type")
now = datetime.now()

with freeze_time(now):
lm.add_locks("test", 10)

with freeze_time(now + timedelta(minutes=2)):
lm.prune()
assert lm.moderator_set(10) == {"test"}

with freeze_time(now + timedelta(minutes=6)):
lm.prune()
assert lm.moderator_set(10) == set()