Add club diff feature #705
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #705 +/- ##
==========================================
+ Coverage 71.10% 71.15% +0.05%
==========================================
Files 31 31
Lines 6824 6833 +9
==========================================
+ Hits 4852 4862 +10
+ Misses 1972 1971 -1 ☔ View full report in Codecov by Sentry. |
backend/clubs/models.py
Outdated
| ): | ||
| return {} | ||
|
|
||
| diff = latest_version.diff_against(latest_approved_version) |
There was a problem hiding this comment.
If we're only reporting name, description, and image changes, do we need to diff every field? Hint
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
backend/clubs/models.py
Outdated
| # if this is the first time the club is being approved | ||
| if not latest_approved_version: | ||
| return { | ||
| self.code: { |
There was a problem hiding this comment.
would prefer to add the club code to the response in the view instead of this helper
There was a problem hiding this comment.
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/
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
backend/clubs/views.py
Outdated
| @@ -2058,6 +2058,70 @@ def perform_destroy(self, instance): | |||
| context=context, | |||
| ) | |||
|
|
|||
| @action(detail=False, methods=["GET"]) | |||
There was a problem hiding this comment.
Can we make this an instance-specific route? Ideally, it will be accessed when OSA is viewing a specific club's page and wants to see the diff between the latest and latest approved versions.
This pull request introduces a new feature aimed at enhancing the admin experience by providing a concise display of changes made to a club's information when it is re-submitted for approval. This feature addresses the current challenge where admins have to manually compare and identify changes by reviewing the entire club page, which is not ideal.