-
-
Notifications
You must be signed in to change notification settings - Fork 5.9k
Edit file workflow for creating a fork and proposing changes #34240
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
Conversation
1 A middle page may be necessary like GH did? Looks like GH will not display a fork form page and will fork at the backend. 2 The branches may be fixed as 3 Is that really necessary to allow user to fork to an organization when forking for the edit? 4 The edit button on README file of repository home page could be displayed. 5 Too many steps and it will make some beginner confusing. |
I rewrote the whole thing, which should hopefully address all the comments. I think the permissions checking is rather confusing now. Previously |
The "fork" logic has too much legacy patched code, it seems that it is not good to introduce more patches, instead maybe it needs refactoring .... And I think it needs tests to make sure the logic is overall right. Do we have enough time to take it into 1.24? (TBH I guess no) |
Fixed now, with test.
Ok, I'm not familiar enough with the overall code to see what is legacy here. Some hints or help would be appreciated.
I'm not in a rush to put it in 1.24. |
The edit buttons might be disabled for anonymous users. I’m not sure if they should be, but GitHub does it that way. Otherwise LGTM |
This comment was marked as resolved.
This comment was marked as resolved.
|
I don't really have anything to mention regarding the code. It's much simpler now and the hard work seems to have been done in the refactoring. Many thanks! |
Fixed and added a test
They share the same backend logic (the same as "Apply Patch"), so I think it's fine to leave it as-is. We can expose it in the future if anyone really needs it.
Moved to the top (see the screenshot)
Fixed and added some tests.
Improved the frontend error handling (see the screenshot)
I will try to pick some tests later. And could you help to list the tests you think are important? Screenshots (slightly outdated, the new commits moved the prompt to before the editor header line) |
Made some refactorings to |
Looks great now, no more comments from my side. |
Let's merge and try (and fix regressions 😁 ) |
When viewing a file that the user can't edit because they can't write to
the branch, the new, upload, patch, edit and delete functionality is no
longer disabled.
If no user fork of the repository exists, there is now a page to create one.
It will automatically create a fork with a single branch matching the one
being viewed, and a unique repository name will be automatically picked.
When a fork exists, but it's archived, a mirror or the user can't write
code to it, there will instead be a message explaining the situation.
If the usable fork exists, a message will appear at the top of the edit page
explaining that the changes will be applied to a branch in the fork. The
base repository branch will be pushed to a new branch to the fork, and
then the edits will be applied on top.
The suggestion to fork happens when accessing /_edit/, so that for
example online documentation can have an "edit this page" link to
the base repository that does the right thing.
Also includes changes to properly report errors when trying to commit
to a new branch that is protected, and when trying to commit to an
existing branch when choosing the new branch option.
Resolves #9017, #20882