Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

ashimokawa
Copy link
Contributor

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 😅

@codecov-io
Copy link

Codecov Report

Merging #6312 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
models/repo_unit.go 61.4% <ø> (ø) ⬆️
modules/auth/repo_form.go 43.24% <ø> (ø) ⬆️
routers/repo/setting.go 10.18% <0%> (-0.02%) ⬇️
models/repo_permission.go 68.18% <0%> (-3.85%) ⬇️
models/unit.go 0% <0%> (-14.29%) ⬇️
routers/repo/view.go 41.08% <0%> (-1%) ⬇️
modules/process/manager.go 81.15% <0%> (+4.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2e9894...e56187c. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 12, 2019
@lunny lunny added the type/enhancement An improvement of existing functionality label Mar 16, 2019
@lunny
Copy link
Member

lunny commented Mar 16, 2019

I think you mean default read or default write for a unit of repository?

@ashimokawa
Copy link
Contributor Author

@lunny
Sorry, I do not understand your question.

This PR is for optionally making wikis of organizations writable to all logged in users (not anonymously).

@lunny
Copy link
Member

lunny commented Mar 17, 2019

@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.

@ashimokawa
Copy link
Contributor Author

@lunny
I intended to only set the permission to write if the flag AllowWriteAll is set for a unit (this flag is only set for the wiki unit type so far, if the checkbox is set an orianziations repo)

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?

@lunny
Copy link
Member

lunny commented Mar 17, 2019

Yes, I mean the column AllowWriteAll is too special. Maybe DefaultPermission is better. This column could be Read or Write and default should be Read to keep compatible with old version.

@ashimokawa
Copy link
Contributor Author

@lunny
You mean literally storing a string "Read" or "Write" in a string type database column?
I can do that if you want, I just looked at #5833, and there you also introduced a new flag - so I followed that pattern.

@stale
Copy link

stale bot commented May 16, 2019

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.

@stale stale bot added the issue/stale label May 16, 2019
@lunny lunny added the issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented label May 16, 2019
@stale stale bot removed the issue/stale label May 16, 2019
@ashimokawa
Copy link
Contributor Author

@lunny
Still waiting for an answer to the question above.
As I said I am willing to change that as you wish at long as there is a chance then that it gets actually merged.

Copy link
Contributor

@davidsvantesson davidsvantesson left a 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}}
Copy link
Contributor

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a migration.

Copy link
Contributor Author

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?

Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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!

Copy link
Contributor Author

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 ;)

@ashimokawa
Copy link
Contributor Author

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.

@a1012112796
Copy link
Member

Hmm, I think we should do more work about improving collaborative authoring support for wiki.

@wxiaoguang
Copy link
Contributor

This PR has been stale for long time, and it seems there are not enough people having interest in it?

@wxiaoguang wxiaoguang added the issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail label Apr 25, 2023
@fnetX
Copy link
Contributor

fnetX commented Apr 28, 2023

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.

@wxiaoguang wxiaoguang removed the issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail label Apr 29, 2023
@wxiaoguang
Copy link
Contributor

Thank you for letting me know the context.

Then the question is:

  1. Is the approach good and safe enough, any design document? eg: make sure writers won't do force push to destroy the wiki, or block some user from spamming (I am not sure, just curious)
  2. Is this PR active and maintainable?

@fnetX
Copy link
Contributor

fnetX commented May 2, 2023

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:

  • easy reverting of commits via the GUI
  • commits from users without write access (or better: without previously accepted contribution) require an approval from a maintainer

But I think the patch as-is is useful and a quick fix for a problem where moderation features can be added later.

@wxiaoguang
Copy link
Contributor

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

@wxiaoguang wxiaoguang closed this Apr 15, 2024
wxiaoguang added a commit that referenced this pull request Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants