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.

Sign up for GitHub

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

Jump to bottom

privileges, url: configurable minimum channel access requirement for .urlban and friends #2352

Merged
merged 2 commits into from
Oct 26, 2022
Merged

privileges, url: configurable minimum channel access requirement for .urlban and friends #2352

merged 2 commits into from
Oct 26, 2022

Conversation

dgw
Copy link
Member

@dgw dgw commented Oct 4, 2022

Description

Tin, more or less.

Added __all__ to sopel.privileges for ease of fetching the names of valid privilege constants programmatically.

.urlban/.urlallow and friends are now only available to bot owner/admins and channel OPs by default, with the option to change the channel access level required via config setting.

Checklist

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make quality and make test)
  • I have tested the functionality of the things this change touches

Notes

Naming Things Is Hard as always. Suggestions welcome if anyone's got a better idea for the new option name in UrlSection.

Decided not to add the new option to configure(), at least for now. Doing so seemed like overkill.

dgw added 2 commits October 3, 2022 19:55
@dgw
privileges: set __all__ to make exported names explicit
2a72adf 
Happens to be the most convenient way I could think of to expose a list
of supported privilege constants programmatically.

Using `dir(sopel.privileges)` (or `var()`) and filtering for non-dunder
names still left `annotations` as part of the list.
@dgw
url: configurable channel access requirement for managing excludes
98c0afa 
Defaults to `sopel.privileges.OP`; auto-populates the `ChoiceAttribute`
using exported names from the `privileges` submodule.

Also allows bot admins, as is traditional.

If we encounter a spacebar-heating* situation, the mechanism here can be
revisited to allow explicitly setting no access requirement.

* - https://xkcd.com/1172/
@dgw dgw added the Feature label Oct 4, 2022
@dgw dgw added this to the 8.0.0 milestone Oct 4, 2022
@dgw dgw requested a review from a team October 4, 2022 06:36
half-duplex
half-duplex approved these changes Oct 26, 2022
View reviewed changes
Copy link
Member

@half-duplex half-duplex left a comment

Choose a reason for hiding this comment

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

Why is sopel.privileges a module, anyway? If it was an enum.Flag class, optionally with X=enum.auto(), we would get a lot of this stuff free...
Besides that, lgtm.

@dgw
Copy link
Member Author

dgw commented Oct 26, 2022

Why is sopel.privileges a module, anyway?

Because of reorganizing in #2179. These constants used to live inside sopel.plugin, which was even worse.

If it was an enum.Flag class, optionally with X=enum.auto(), we would get a lot of this stuff free...

You're not wrong. No package version has ever been released with sopel.privileges, so if you want to throw together a follow-up to #2179 that does something sensible with an enum.Flag class (while staying backward compatible with the aliases in sopel.plugin for the time being), I'd be all for it. If it's replaced by something better, we could just pretend sopel.privileges-the-module never happened.

@dgw dgw merged commit e5be43c into master Oct 26, 2022
@dgw dgw deleted the url-exclude-perms branch October 26, 2022 04:17
@half-duplex
Copy link
Member

Such flexibility! Releases suck, -dev is comfy, we should stay here forever.

@Exirel
Copy link
Contributor

Exirel commented Oct 27, 2022

Why is sopel.privileges a module, anyway? If it was an enum.Flag class, optionally with X=enum.auto(), we would get a lot of this stuff free... Besides that, lgtm.

We would still need to find a place for this enum class. Fixing circular import is way more important than the small free stuffs that enum would give us in that case. My opinion here is that there is not a lot of free stuff, only few free stuffs.

And it would not be worth it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@half-duplex half-duplex half-duplex approved these changes

Assignees
No one assigned
Labels
Feature
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@dgw @half-duplex @Exirel