-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Allow setting wiki writeable for all users on an instance #6312
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6312 +/- ##
==========================================
- Coverage 38.82% 38.81% -0.02%
==========================================
Files 359 359
Lines 51137 51146 +9
==========================================
- Hits 19854 19850 -4
- Misses 28411 28425 +14
+ Partials 2872 2871 -1
Continue to review full report at Codecov.
|
I think you mean default read or default write for a unit of repository? |
@lunny This PR is for optionally making wikis of organizations writable to all logged in users (not anonymously). |
@ashimokawa I think your PR is for the unit default permission for all logged users. Currently public repositories will let logged in user read all the unit of them. But this PR will change the default to write. |
@lunny This is my first time tinkering with the code base, would you mind to explain what I missed? Your comment suggest that my PR is actually doing more than I intended, right? |
Yes, I mean the column |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions. |
@lunny |
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.
As lunny I think this should be more generic. Maybe even None should be a possible value (to restrict an otherwise public repo)
<label>{{.i18n.Tr "repo.settings.use_internal_wiki"}}</label> | ||
</div> | ||
</div> | ||
{{if .Repository.Owner.IsOrganization}} |
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.
Setting should only be available for public repositories?
ID int64 | ||
RepoID int64 `xorm:"INDEX(s)"` | ||
Type UnitType `xorm:"INDEX(s)"` | ||
AllowWriteAll bool |
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.
We need a migration.
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 thought false is the default, then we would still need it?
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.
All database structure changes should have a migration. You can find many example on models/migration
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.
Thanks, I see, I somehow thought that there was some magic invovled which inserts missing columns automagically.
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.
There is some magic involved, and you've probably got your column migrated automagically.
There are two stages to Gitea migration:
- The "manual steps" stage (from
models/migration
), that update the internal version and change data as needed. - The "startup" stage, where any remaining database structure is updated.
The problem comes when you rely on the 2nd stage to do your migration. It will work the first time, but in the future (e.g. Gitea 12.2) some manual steps can be added in the first stage of the migration that will need the structure that is not there yet (as it is created in the 2nd stage). If some user attempts to migrate from 1.10 to 12.2, all the steps on stage 1 will be executed, and that particular step will fail because of the missing column.
That's why we don't rely on the 2nd stage and make all migration steps explicit, so any upcoming step can count on the previous steps to upgrade the database to the expected state.
I hope this makes sense!
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.
Thank you for a great explanation! Understood and I am convinced ;)
To be honest I do not like that this setting only works for wikis of repos of organizations (not if the repo is owned by a normal user), it has been a while, so I have to look up why I found it difficult to do it for all wikis. |
Hmm, I think we should do more work about improving collaborative authoring support for wiki. |
e56187c
to
92bf953
Compare
This PR has been stale for long time, and it seems there are not enough people having interest in it? |
There is a lot of interest in it. It's repeatedly requested by users on Codeberg since this is literally what a "wiki" is meant for. For example: https://codeberg.org/Codeberg/Community/issues/28 and https://codeberg.org/leiningen/leiningen/issues/13 and sometimes via Mastodon. |
Thank you for letting me know the context. Then the question is:
|
No, the code is not currently used, because rebasing it was too much effort. I was thinking about this in the past days, but I'm not fully sure. While we considered a trust/reputation system for new users a few times already, this is likely out of scope for this PR. Since this Pr added a switch, it is probably for the projects to deal with it (one project had a patch in use for a few times quite successfully). Bonus features could be:
But I think the patch as-is is useful and a quick fix for a problem where moderation features can be added later. |
Thank you ashimokawa for this PR. Since it has been stale for long time (there is still no migration or progress), I think we could close it. I managed to proposed a new one: -> Allow everyone to read or write a wiki by a repo unit setting #30495 |
This change allows to set wikis of an organization wiki to writable for all user of an instance.
This setting is also available on github (even default?).
Internally the setting is available to all Units but currently only used for wiki.
This PR is heavily inspired by #5833 by @lunny, and will conflict with his. Thanks and sorry 😅