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.

Sign up for GitHub

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

Jump to bottom

Add club diff feature #705

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Add club diff feature #705

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f56a70f
add club diff feature
Porcupine1 Jun 20, 2024
c89fc81
add YAML doc
Porcupine1 Jun 20, 2024
7dd0ad1
address nits + add tests
Porcupine1 Jul 6, 2024
d82c835
address yaml error
Porcupine1 Jul 6, 2024
01ccd5f
address requested changes
Porcupine1 Sep 2, 2024
2ec546e
add detail and list diff
Porcupine1 Oct 13, 2024
1c14e0a
small changes
Porcupine1 Oct 13, 2024
1724ec7
small changes
Porcupine1 Oct 13, 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
31 changes: 31 additions & 0 deletions backend/clubs/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -698,6 +698,37 @@ def send_approval_email(self, request=None, change=False):
reply_to=settings.OSA_EMAILS + [settings.BRANDING_SITE_EMAIL],
)

def get_latest_and_latest_approved_versions(self):
"""
Returns the latest and latest approved versions of club key fields
[name, description, image] contents that trigger reapproval.
"""
aviupadhyayula marked this conversation as resolved.
Show resolved Hide resolved
latest_approved_version = (
self.history.filter(approved=True).order_by("-history_date").first()
aviupadhyayula marked this conversation as resolved.
Show resolved Hide resolved
)
latest_version = self.history.order_by("-history_date").first()

# if this is the first time the club is being approved
if not latest_approved_version:
return {
self.code: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would prefer to add the club code to the response in the view instead of this helper

Copy link
Contributor Author

@Porcupine1 Porcupine1 Sep 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if we make the route instance-specific, we won't require the club code, right? With this new implementation, it looks like the approval queue list will remain as is, only that the link to a club in the queue is to /api/clubs/<code>/diff/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we'll point to a new page for clubs in the queue (e.g. right now clicking on a club in the queue goes to /api/clubs/<code>).

Instead, the idea is to expose an endpoint /diff that returns some historical comparison (either the last approved version, or the diffs themselves). We'll then add a param when an admin clicks on a club in the queue (e.g. /api/clubs/<code>?review=True) that triggers some frontend logic. The frontend logic is what's going to call /api/clubs/<code>/diff and display the results in some fashion.

@julianweng @rm03 let me know if that's what we were thinking here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, that makes sense. I was actually thinking that there would be code repetition (with /api/clubs/<code> route) if implemented the way I described it earlier

field: {"old": None, "new": getattr(latest_version, field)}
for field in ["name", "description", "image"]
}
}

# if the current version is not approvedthe and it has been approved before
if not self.approved and latest_approved_version:
return {
self.code: {
field: {
"old": getattr(latest_approved_version, field),
"new": getattr(latest_version, field),
}
for field in ["name", "description", "image"]
}
}

class Meta:
ordering = ["name"]
permissions = [
Expand Down
64 changes: 64 additions & 0 deletions backend/clubs/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2058,6 +2058,70 @@ def perform_destroy(self, instance):
context=context,
)

@action(detail=False, methods=["GET"])
rm03 marked this conversation as resolved.
Show resolved Hide resolved
def pending_clubs_old_new_data(self, request, *args, **kwargs):
"""
Return old and new data for clubs that are pending approval.
---
responses:
"200":
content:
application/json:
schema:
type: array
items:
type: object
properties:
code:
type: object
description: club code
properties:
name:
type: object
description: Changes in the name field
properties:
old:
type: string
description: >
Old name of the club
new:
type: string
description: >
New name of the club
description:
type: object
description: >
Changes in the club description
properties:
old:
type: string
description: >
Old description of the club
new:
type: string
description: >
New description of the club
image:
type: object
description: >
Changes in the image of the club
properties:
old:
type: string
description: >
Old image URL of the club
new:
type: string
description: >
New image URL of the club
---
"""
pending_clubs = Club.objects.filter(approved=None, active=True)

return Response(
[club.get_latest_and_latest_approved_versions() for club in pending_clubs]
)

@action(detail=False, methods=["GET"])
def fields(self, request, *args, **kwargs):
"""
Expand Down
60 changes: 60 additions & 0 deletions backend/tests/clubs/test_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1306,6 +1306,66 @@ def test_club_list_filter(self):
codes = [club["code"] for club in data]
self.assertEqual(set(codes), set(query["results"]), (query, resp.content))

def test_pending_clubs_old_new_data(self):
"""
Test that pending_clubs_old_new returns the correct data for new and clubs
seeking reapproval
"""

# create club that requires approval
new_club = Club.objects.create(
code="new-club",
name="New Club",
description="We're the spirits of open source.",
approved=None,
active=True,
email="[email protected]",
)

# add officer to club
Membership.objects.create(
person=self.user4, club=new_club, role=Membership.ROLE_OFFICER
)

# login to officer user
self.client.login(username=self.user4.username, password="test")

# New club should not have any old data
resp = self.client.get(reverse("clubs-pending-clubs-old-new-data"))
data = json.loads(resp.content.decode("utf-8"))
self.assertEqual(data[0]["new-club"]["name"]["new"], "New Club")
self.assertEqual(data[0]["new-club"]["name"]["old"], None)

# approve club
new_club.approved = True
new_club.save(update_fields=["approved"])
self.assertTrue(new_club.approved)

# Make a change that edit description (requires reapproval)
resp = self.client.patch(
reverse("clubs-detail", args=(new_club.code,)),
{"description": "We are open source, expect us."},
content_type="application/json",
)
# Ensure change successful
self.assertIn(resp.status_code, [200, 201], resp.content)

# Ensure club is marked as not approved
new_club.refresh_from_db()
self.assertFalse(new_club.approved)

# Should now have old and new data
resp = self.client.get(reverse("clubs-pending-clubs-old-new-data"))
new_data = json.loads(resp.content.decode("utf-8"))
self.assertEqual(
new_data[0]["new-club"]["description"]["new"],
"We are open source, expect us.",
)
self.assertEqual(
new_data[0]["new-club"]["description"]["old"],
"We're the spirits of open source.",
)

def test_club_modify_wrong_auth(self):
"""
Outsiders should not be able to modify a club.
Expand Down
Loading