-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Notify about recent pushes if a branch has no PR yet #14003
Conversation
I'm worried it might be too cluttered when more than few message are displayed (especially with rather long time limit). How about moving it to repo page similar to how GH does it? |
How about display it in the repository home page but not user's dashboard page. Once the user push to serval repositories, which one should we display? |
@CirnoT I think this could be addressed with either a shorter time limit or limiting the max messages to 2 or 4 (or both) I think github shows it on both the repo page and the dashboard. IIRC gitlab does that as well. |
Fix #23743 |
Would there be interest in picking this PR up again? |
Maybe it should be on top of the repository home page but not dashboard? |
The interest is definitely there, but I wonder if it wouldn't make more sense to create a new branch, one to resolve all conflicts that happened in the mean time, and two to give it the attention boost of a new PR? |
@delvh Probably. Should I do that or wants someone else pick it up? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense if you publish the new PR as you're also the author of this one.
Below you find some more things I just noticed.
options/locale/locale_en-US.ini
Outdated
@@ -1381,6 +1381,8 @@ pulls.merge_instruction_hint = `You can also view <a class="show-instruction">co | |||
pulls.merge_instruction_step1_desc = From your project repository, check out a new branch and test the changes. | |||
pulls.merge_instruction_step2_desc = Merge the changes and update on Gitea. | |||
|
|||
pulls.recently_pushed_to_branches = You recently pushed to <strong>%[1]s/%[2]s</strong> on branch <strong>%[3]s</strong> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to @lunny's suggestion above, this message should then be renamed.
# Conflicts: # models/action.go # routers/web/user/home.go
That is my impression as well. Single database allows many optimizations and features that just are not possible with multi-database. We might consider it if we can provide migrations, but I think that won't be so easy. |
Also on unrelated note, that query returns for quite weird results:
These branches do not exist, Multiple PRs for them exists but they are either merged or closed (unmerged). |
I think we could also do some optimization at the moment but those are temporary methods. As I said, once branches have been synced into databases, this PR could be implemented very easily. Assume we have a new table |
We could on branch push push all pushed branches into a per-user append-only data structure where values would automatically expire after the 2 hour time period (like redis EXPIRE command). Then, when rendering, the operation is just a simple retrieval of that list with some filtering for cases where branch was deleted or has an associated PR. The only thing I'm not sure about if auto-expiring keys can be done with SQL, or if that would require a cache like redis. |
I think GitHub keeps it out of the initial page render. Maybe it can be done in the background with an API call and render the suggestion only when ready? |
Yeah, it is definitely async on GitHub which causes ugly JS pop-in. If we can find a performant synchronous solution it would be ideal. |
Thanks everyone for your investigations and queries. My goal was to use existing data structures to avoid a higher overhead for such a simple feature. Well, looks like this is not as simple as I thought.
Should this PR be blocked and reworked when the other one got merged? |
|
@techknowlogick why? |
@yardenshoham caught up in a PR cleanup sweep, thanks for re-opening :) |
I think maybe this PR could be taken in 1.20 if no objection for "only querying recent N hours
|
Yes, and ideally only show notification on the repo page. Not on the frontpage. Oh, and it needs to filter out branches that have a associated PR, haven't checked if it already does that. |
Since #22743 merged, I think this could be continued now. |
As already mentioned, I would like to see this changed to only show on the repo page as it would be too annoying on the frontpage. |
I don't really have time to implement this in the next few weeks, so if anyone wants to pick it up, feel free. Otherwise I'll take a stab at it at the end of August / early September. |
This PR will display a pull request creation hint on the repository home page when there are newly created branches with no pull request. Only the recent 6 hours and 2 updated branches will be displayed. Inspired by #14003 Replace #14003 Resolves #311 Resolves #13196 Resolves #23743 co-authored by @kolaente
This PR adds a nice little message on the dashboard if the current user recently pushed to a branch which has no PR open yet, with a link to do so. If the user recently pushed to a fork, it will show a message as well, but the link will go to create a PR with the upstream repository instead.
This PR explicitly does not show a link to existing PRs after pushes to a branch to keep it simple.
Screenshots
TODO
Resolves #311, resolves #13196, resolves #23743