- Notifications
You must be signed in to change notification settings - Fork12
Jelte's not-yet-deployed commits#7
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Closed
Closed
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This file contains hidden or 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 thecommitfest app. We currently keep the last CFbot runs in our database,in an attempt to keep the queries easy and efficient. The CFbot resultsare now shown on the overview page of each commitfest, as well as on thepage for each specific patch.
In previous commits (2ada851 &b6010e9) some initial links and gitcheckout instructions were added to the patch page. I received some inperson feedback at PgConf EU that those used quite some vertical spaceon the page.Now that we have a dedicated row in the table for CFbot statuses, itseems natural to integrate those previous links and checkoutinstructions into that row. Both to keep all CFbot content together, aswell as to reduce vertical space needed.To reduce the necessary space needed for the checkout instructions,these previously explicit checkout instructions have been changed tobutton that copies the required commands to the clipboard.Finally, this also makes "Needs rebase!" link to CFbot logs instead ofCI results. I updated these logs recently on the CFbot side to becomemuch more useful to look at, by making them actually show the conflicts.
This is needed to send notifications to a patch author that itsubscribed. We don't send updates to other subscribers to the patch, asthis information is generally only useful for the author themselves.By using the "history item" infrastructure it's also displayed on thepatch page in the "History" table. That seems quite useful too, becauseit makes it clear how long the patch has needed a rebase.
This is mostly useful to avoid people from adding trailing whitespace tofiles.
The ID of a CF entry is the only stable identifier (people can changethe name). That's why tooling such as CFbot uses the ID of the patch fora lot of things (including showing it on the cfbot overview page).Having it visible on the page is quite useful for debugging purposesIneee60a5 the ID was added to the title of the entry, but that made thethe title of the page itself less prominent. So this is an attempt tohave the information available, but not too prominently visible.
If the commitfest entries are sorted by a column clicking the headeragain will now remove the sort. In a future commit, it would be nice toalso support for reverse sorting.
It was pretty confusing that clicking the patch name header would sortby created time, grouped by topic. This makes that sort by name.
Sorting order impacts closed patches too. So not showing the arrow thatindicates sort direction on that header is confusing. While we're at it,we might as well allow people to toggle sorting direction using thatheader too.This also automatically fixes the problem that the cell in the closedpatches header did not contain any title at all.
Previously we'd include the ID of the commitfest in the URL of thepatch. In9f12a5e we introduced a stable URL for patches that wouldredirect to the one for the latest commitfest. This starts to use thatURL as the valid only URL for a patch (with the previous URL redirectingto this one).The reasoning behind this is that the old approach resulted in Ndifferent URLs for each patch, which all showed the exact same patchinformation. The only difference between all these URLs would be thebreadcrumb at the top of the page.The only benefit of that approach is that if you're on an oldcommitfest, and click a link there, then the breadcrumb will bring youback to where you came from. Since people rarely have a reason to browseclosed commitfests, the that benefit seems pretty small. Especiallybecause people can just as well press their browser back button, in thatcase.The problems that these N links cause seem much more impactful to mostusers: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 ofthe commitfests that a patch has been part of. Those links were alreadyrather useless, since all they effectively did was change thebreadcrumb. But with this new commit, they wouldn't even do that anymore,and simply redirect to the current page. So now they start pointing tothe commitfest itself, which seems more useful behaviour anyway.Co-Authored-By: Jacob Brazeal <jacob.brazeal@scale.com>
The bottom dropdowns on the patch page would expand downwards, requiringthe 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 dumpthe `auth` and `commitfest` modules separately. This data can likewisebe 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 samplemailing threads to their patches.To avoid an infinite recursion error this change also required movingthe ManyToMany relationship between MailThread and Patch from theMailThread to Patch side.---------Co-authored-by: Jelte Fennema-Nio <postgres@jeltef.nl>
Sign up for freeto 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.
Uh oh!
There was an error while loading.Please reload this page.
This PR contains bunch of commits that have been running on the staging environment for a while now, but haven't been deployed yet. Reviews of any of them would be highly appreciated. The commit messages of each patch have details on the reasoning. If you want to access the staging environment, there's currently a password on it, but I can provide it to you if you want to test the changes.