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

Add club diff feature #705

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
25 changes: 25 additions & 0 deletions backend/clubs/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -698,6 +698,31 @@
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 = (

Check warning on line 706 in backend/clubs/models.py

View check run for this annotation

Codecov / codecov/patch

backend/clubs/models.py#L706

Added line #L706 was not covered by tests
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()

Check warning on line 709 in backend/clubs/models.py

View check run for this annotation

Codecov / codecov/patch

backend/clubs/models.py#L709

Added line #L709 was not covered by tests

if (

Check warning on line 711 in backend/clubs/models.py

View check run for this annotation

Codecov / codecov/patch

backend/clubs/models.py#L711

Added line #L711 was not covered by tests
not latest_version
or not latest_approved_version
Porcupine1 marked this conversation as resolved.
Show resolved Hide resolved
or (latest_approved_version == latest_version)
):
return {}

Check warning on line 716 in backend/clubs/models.py

View check run for this annotation

Codecov / codecov/patch

backend/clubs/models.py#L716

Added line #L716 was not covered by tests
Porcupine1 marked this conversation as resolved.
Show resolved Hide resolved
Porcupine1 marked this conversation as resolved.
Show resolved Hide resolved

diff = latest_version.diff_against(latest_approved_version)

Check warning on line 718 in backend/clubs/models.py

View check run for this annotation

Codecov / codecov/patch

backend/clubs/models.py#L718

Added line #L718 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

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

If we're only reporting name, description, and image changes, do we need to diff every field? Hint

Copy link
Member

Choose a reason for hiding this comment

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

Yep, in retrospect I'm not sure if this line is strictly necessary in the first place if we're computing diffs on the frontend (as discussed) anyways. We could compute on the backend but it would require more detailed serialization and not necessarily be much more performant at this scale.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, agreed. I'm sure React has some great diff libraries. I think it'd be cleaner to expose something that fetches the most recently approved version of a club, and we can do diffing on the frontend.

I'm also not sure where the pending_clubs endpoint would be used. We already know which clubs we want to diff (e.g. the ones in the approval queue), and I can't see where we'd use this outside of the queue.

@Porcupine1 it'd be helpful to iron out the details with Julian further.

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 remember we discussed that initially we were just the calling the clubs viewset list function with the approved = false and active = true query/search params. I added pending_clubs because we are now returning pending clubs data


return {

Check warning on line 720 in backend/clubs/models.py

View check run for this annotation

Codecov / codecov/patch

backend/clubs/models.py#L720

Added line #L720 was not covered by tests
change.field: {"old": change.old, "new": change.new}
for change in diff.changes
if change.field in ["name", "description", "image"]
}

class Meta:
ordering = ["name"]
permissions = [
Expand Down
52 changes: 52 additions & 0 deletions backend/clubs/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -2058,6 +2058,58 @@
context=context,
)

@action(detail=False, methods=["GET"])
rm03 marked this conversation as resolved.
Show resolved Hide resolved
def pending_clubs(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:
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)

Check warning on line 2107 in backend/clubs/views.py

View check run for this annotation

Codecov / codecov/patch

backend/clubs/views.py#L2107

Added line #L2107 was not covered by tests

return Response(

Check warning on line 2109 in backend/clubs/views.py

View check run for this annotation

Codecov / codecov/patch

backend/clubs/views.py#L2109

Added line #L2109 was not covered by tests
[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
Loading