feat: add reverse sorting #32
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This adds our own custom replication between the CFbot and the commitfest app. We currently keep the last CFbot runs in our database, in an attempt to keep the queries easy and efficient. The CFbot results are now shown on the overview page of each commitfest, as well as on the page for each specific patch.
In previous commits (2ada851 & b6010e9) some initial links and git checkout instructions were added to the patch page. I received some in person feedback at PgConf EU that those used quite some vertical space on the page. Now that we have a dedicated row in the table for CFbot statuses, it seems natural to integrate those previous links and checkout instructions into that row. Both to keep all CFbot content together, as well as to reduce vertical space needed. To reduce the necessary space needed for the checkout instructions, these previously explicit checkout instructions have been changed to button that copies the required commands to the clipboard. Finally, this also makes "Needs rebase!" link to CFbot logs instead of CI results. I updated these logs recently on the CFbot side to become much more useful to look at, by making them actually show the conflicts.
This is needed to send notifications to a patch author that it subscribed. We don't send updates to other subscribers to the patch, as this information is generally only useful for the author themselves. By using the "history item" infrastructure it's also displayed on the patch page in the "History" table. That seems quite useful too, because it makes it clear how long the patch has needed a rebase.
This is mostly useful to avoid people from adding trailing whitespace to files.
The ID of a CF entry is the only stable identifier (people can change the name). That's why tooling such as CFbot uses the ID of the patch for a lot of things (including showing it on the cfbot overview page). Having it visible on the page is quite useful for debugging purposes In eee60a5 the ID was added to the title of the entry, but that made the the title of the page itself less prominent. So this is an attempt to have the information available, but not too prominently visible.
If the commitfest entries are sorted by a column clicking the header again will now remove the sort. In a future commit, it would be nice to also support for reverse sorting.
It was pretty confusing that clicking the patch name header would sort by created time, grouped by topic. This makes that sort by name.
Sorting order impacts closed patches too. So not showing the arrow that indicates sort direction on that header is confusing. While we're at it, we might as well allow people to toggle sorting direction using that header too. This also automatically fixes the problem that the cell in the closed patches header did not contain any title at all.
Previously we'd include the ID of the commitfest in the URL of the patch. In 9f12a5e we introduced a stable URL for patches that would redirect to the one for the latest commitfest. This starts to use that URL as the valid only URL for a patch (with the previous URL redirecting to this one). The reasoning behind this is that the old approach resulted in N different URLs for each patch, which all showed the exact same patch information. The only difference between all these URLs would be the breadcrumb at the top of the page. The only benefit of that approach is that if you're on an old commitfest, and click a link there, then the breadcrumb will bring you back to where you came from. Since people rarely have a reason to browse closed commitfests, the that benefit seems pretty small. Especially because people can just as well press their browser back button, in that case. The problems that these N links cause seem much more impactful to most users: 1. If you click an old link to a cf entry (e.g. one from an email in the archives), then the breadcrumb will contain some arbitrarily old commitfest. It seems much more useful to have the breadcrumb show the commitfest that the patch is currently active in (or got committed/rejected in). 2. If you arrive on such an old link you also won't be able to change the status. Instead you'd get a message like: "The status of this patch cannot be changed in this commitfest. You must modify it in the one where it's open!". Which requires you to go to the latest page. 3. Places that use the stable URLs require an extra round-trip to actually get to the patch page. 4. It's a bit confusing that old pages of a patch still get updated with all the new information, i.e. why have all these pages if they contain the exact same content. 5. Problem 3 is generally also bad for Search Engine Optimization (SEO), for now we don't care much about that though. Finally this also changes the links on the patch page itself for each of the commitfests that a patch has been part of. Those links were already rather useless, since all they effectively did was change the breadcrumb. But with this new commit, they wouldn't even do that anymore, and simply redirect to the current page. So now they start pointing to the commitfest itself, which seems more useful behaviour anyway. Co-Authored-By: Jacob Brazeal <[email protected]>
The bottom dropdowns on the patch page would expand downwards, requiring the user to scroll down to see and click any of the buttons in the dropdown. With this change these are changed into "dropup" menus, so the expand upwards.
This PR implements a way to use dummy data for development in two ways: 1. Dump a working dev database with Django's `dumpdata` command. We dump the `auth` and `commitfest` modules separately. This data can likewise be reloaded when starting from scratch with the corresponding `loaddata` commands (see the README.) 2. Mocks the archives server, to allow users to search and add sample mailing threads to their patches. To avoid an infinite recursion error this change also required moving the ManyToMany relationship between MailThread and Patch from the MailThread to Patch side. --------- Co-authored-by: Jelte Fennema-Nio <[email protected]>
As far as I can tell this doesn't matter in practice, because it seems that with the way we use CirrusCI it never actually returns the SCHEDULED status. I ran into this when working with some dummy development data though (see #8).
This adds some basic functionality to allow making changes to the CFBot integration locally, without having to push to prod/staging. To do so this adds the CFBot integration tables to the admin panel and adds some dummy CFBot data to our dummy development dataset. One somewhat interesting thing this required that now the `CfbotBranch` has an overridden `save` method, which stores `NULL` in the commit_id if an empty string is saved. This only impacts the save behaviour of the Django admin interface because the webhook that the actual CFBot calls bypasses the Django models and inserts directly into the underlying tables.
Browsers cache static files. That's obviously good. Currently it results in our we change files server side, users don't always see them. This generates a GET parameter that's unique per startup of the python process to bust the cache of the client, so that it pulls in possibly updated JS/CSS files. Obviously this is a bit more often than necessary, but caching should still work most of the time and our custom css and js files are tiny anyway. At least the changes that are made will now be reflected to our users.
This syncs and displays some of the new patch statistics that the CFbot gathers on the commitfest patch page. These stats are: 1. Version of the patch 2. Number of patches in the last patchset 3. Additions/deletions in the first patch of the last patchset 4. Total additions/deletions when combining all patches in the last patchset It also displays the "Total additions/deletions" in the commitfest overview.
There was a typo, and it was using the wrong id column.
People sometimes forget to configure the author field when adding a patch to the commitfest. It's also kinda silly to always have to search for your own name. In the cases that people add commitfest entries for patches of others, removing themselves and adding the other person is only a backspace presses away. Apart from that this also updates the patch creation form to include all the fields from the patch edit form. Instead of having the patch creation submit button redirect to the patch edit page. Fixes #14
It's a bit silly that you have to manually enter something in the description, when you just selected a thing with that title from the thread search modal. So this starts auto-filling the description. This only happens when the field is empty, and will strip some common prefixes like `Re:` and `[PATCH]`. Fixes #16
JelteF
added a commit
that referenced
this pull request
Feb 16, 2025
Implements reverse sorting in the columns of the commitfest pages. This is a cleaned up version of #28 and #32 Fixes #20 --------- Co-authored-by: Jelte Fennema-Nio <[email protected]>
JelteF
added a commit
that referenced
this pull request
Mar 4, 2025
Implements reverse sorting in the columns of the commitfest pages. This is a cleaned up version of #28 and #32 Fixes #20 --------- Co-authored-by: Jelte Fennema-Nio <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a cleaned up version of PR #28
Implements reverse sorting for commitfest form columns.
Fixes: