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.

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

Conversation

destrex271
Copy link
Contributor

Added stats generally visible on the generic commitfest page to the personal dashboard.

@destrex271 destrex271 marked this pull request as ready for review March 5, 2025 18:30
Copy link
Collaborator

@JelteF JelteF left a comment

Choose a reason for hiding this comment

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

Overall I think this is a good addition, but it needs some changes because it's not working well yet.

{"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

Comment on lines 4 to 7
<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.

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
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

Comment on lines 96 to 97
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""")

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

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
LEFT JOIN commitfest_patch_subscribers psubs ON psubs.patch_id=poc.patch_id
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.

@destrex271 destrex271 requested a review from JelteF March 19, 2025 17:37
Copy link
Collaborator

@JelteF JelteF left a comment

Choose a reason for hiding this comment

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

Looks mostly good. Some small fixes for the query suggested.

destrex271 and others added 2 commits March 21, 2025 12:32
Co-authored-by: Jelte Fennema-Nio <[email protected]>
Co-authored-by: Jelte Fennema-Nio <[email protected]>
@destrex271
Copy link
Contributor Author

Looks mostly good. Some small fixes for the query suggested.

done

@JelteF JelteF merged commit 309d9f7 into postgres:main Mar 22, 2025
1 check passed
@JelteF
Copy link
Collaborator

JelteF commented Mar 22, 2025

It seems you didn't actually test the changes after applying them, because the page was broken. I fixed those issues and merged the result.

@destrex271
Copy link
Contributor Author

It seems you didn't actually test the changes after applying them, because the page was broken. I fixed those issues and merged the result.

Oops😅 Yeah really sorry for the incovinience

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants