Skip to content

enhancement: added commitfest stats to /me page #52

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

Merged
merged 11 commits into from
Mar 22, 2025
Merged

enhancement: added commitfest stats to /me page #52

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
3f1bc1d
added stats to commitfest personal dashboard page
destrex271 Mar 5, 2025
c8482b5
fixed query to include patches where user is either a reviewer or has…
destrex271 Mar 5, 2025
bb4e557
fixed linting errors in views.py
destrex271 Mar 5, 2025
ff3fe8a
removed print statements; removed subscribers in status list data
destrex271 Mar 13, 2025
cdec174
added check to exclude closed patches
destrex271 Mar 13, 2025
abfff8c
replaced left join with exists clause in where
destrex271 Mar 13, 2025
f0f6ebc
minor fix for stats
destrex271 Mar 13, 2025
75b5fb4
added returned with feedback to not in filter
destrex271 Mar 13, 2025
d716195
make stats query consistent
destrex271 Mar 21, 2025
b4258ef
updated query
destrex271 Mar 21, 2025
7f76579
Fix issues
JelteF Mar 22, 2025
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
5 changes: 5 additions & 0 deletions pgcommitfest/commitfest/templates/me.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
{%extends "base.html"%}
{%load commitfest %}
{%block contents%}
<p>
<br/>
<b>Status summary: </b>{%for id,title,num in statussummary%}<a href="?status={{id}}">{{title}}</a>: {{num}}. {%endfor%}
</p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's move this below the buttons, so it matches the layout of the commitfest page.


<a class="btn btn-default" href="/open/new/">New patch</a>
<a class="btn btn-default" href="/current/">Current commitfest</a></li>
<a class="btn btn-default" href="/open/">Open commitfest</a></li>
Expand Down
24 changes: 23 additions & 1 deletion pgcommitfest/commitfest/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,14 +86,36 @@ def me(request):
if patch_list.redirect:
return patch_list.redirect

# Get stats related to user for current commitfest
curs = connection.cursor()
curs.execute(
"""SELECT
ps.status, ps.statusstring, count(*)
FROM commitfest_patchoncommitfest poc
INNER JOIN commitfest_patchstatus ps ON ps.status=poc.status
LEFT JOIN commitfest_patch_authors pa ON pa.patch_id=poc.patch_id
LEFT JOIN commitfest_patch_reviewers pr ON pr.patch_id=poc.patch_id
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will inflate counts if a patch has more than one author or reviewer. Instead of doing this join we should use the same WHERE filter we use for the display list:

            EXISTS (
                SELECT 1 FROM commitfest_patch_reviewers cpr WHERE cpr.patch_id=p.id AND cpr.user_id=%(self)s
            ) OR EXISTS (
                SELECT 1 FROM commitfest_patch_authors cpa WHERE cpa.patch_id=p.id AND cpa.user_id=%(self)s
            ) OR p.committer_id=%(self)s""")

LEFT JOIN commitfest_patch_subscribers psubs ON psubs.patch_id=poc.patch_id
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd keep out the subscribers for now, I think it's a good addition, but we don't use that in the display query so that's just confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, removing this

WHERE commitfest_id=%(id)s
Copy link
Collaborator

Choose a reason for hiding this comment

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

This check should be removed, right now we don't filter the list by commitfest_id. So the counts shouldn't be filtered by it either.

AND (
pa.user_id=%(user_id)s
OR pr.user_id=%(user_id)s
OR psubs.user_id=%(user_id)s
Copy link
Collaborator

Choose a reason for hiding this comment

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

A check for the patch status should be added. Right now it's also showing closed patches, even though we filter these out when displaying the list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding a check to eliminate committed, withdrawn and rejected patches.

Although wouldn't it be better to also show the withdrawn patches? Just in case if someone wants to revisit the patch in future

)
GROUP BY ps.status ORDER BY ps.sortkey""",
{"id": cf.id, "user_id": request.user.id},
)
statussummary = curs.fetchall()
print(statussummary)
Copy link
Collaborator

Choose a reason for hiding this comment

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

test print that should go away


return render(
request,
"me.html",
{
"form": form,
"title": "Personal Dashboard",
"patches": patch_list.patches,
"statussummary": "",
"statussummary": statussummary,
"has_filter": patch_list.has_filter,
"grouping": patch_list.sortkey == 0,
"sortkey": patch_list.sortkey,
Expand Down