-
Notifications
You must be signed in to change notification settings - Fork 12
Add roles.addnote_access config option
#259
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
base: main
Are you sure you want to change the base?
Conversation
Test with
```ruby
require 'ostruct'
module QBot
def self.config
OpenStruct.new(
roles: OpenStruct.new(
note_access: [123, 456] # IDs allowed to access
# Comment above and uncomment below, or delete both to test
#note_access: []
)
)
end
end
def self.has_note_access?(event)
required_role_ids = Array(QBot.config&.roles&.note_access)
puts "Required role IDs: #{required_role_ids.inspect}"
# If nil or empty array, allow access
return true if required_role_ids.empty?
user_role_ids = event.author.roles.map(&:id)
puts "User role IDs: #{user_role_ids.inspect}"
(required_role_ids & user_role_ids).any?
end
Role = Struct.new(:id)
Author = Struct.new(:roles)
Event = Struct.new(:author)
def self.run_example(event)
if has_note_access?(event)
puts "Access granted."
else
puts "Access denied."
end
end
user_roles = [Role.new(111), Role.new(123)]
event = Event.new(Author.new(user_roles))
run_example(event)
```
anna328p
left a comment
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.
Please also make sure your commit messages follow the Conventional Commits spec.
Although it needs work before the PR can be merged, this is a really good feature and I would love to see it in qbot! Thank you for your PR!
| bot_id_allowlist: | ||
| - 204255221017214977 | ||
|
|
||
| # To allow access to everyone, either remove the whole array or remove/empty the lists |
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.
This should not be global. In its current state, this filter would be applied across every server qbot is in. Instead this should be a server config setting.
See https://github.com/arch-community/qbot/blob/main/lib/qbot/db/concerns/configurable/option_types.rb.
This does not support an array type right now, that would need to be added.
Usage examples of the config registry:
Lines 4 to 53 in c6803f4
| # Enum class for sitelen pona fonts | |
| class TSPFontSelect < Configurable::OptionTypes::TEnum | |
| # rubocop: disable Lint/MissingSuper | |
| def initialize | |
| @options = SPGen.font_metadata | |
| end | |
| # rubocop: enable Lint/MissingSuper | |
| def find_by_abbrev(...) | |
| @options_hash ||= @options.to_h { |e| [e.nickname || e.typeface, e] } | |
| @abbrev ||= @options_hash.keys.abbrev | |
| @options_hash[super].typeface | |
| end | |
| def find_by_index(...) | |
| super.typeface | |
| end | |
| def self.get_metadata(typeface) | |
| SPGen.font_metadata.find { _1.typeface == typeface } | |
| end | |
| def format_value(value, ...) | |
| meta = self.class.get_metadata(value) | |
| name = meta.nickname || meta.typeface | |
| super(name, ...) | |
| end | |
| def describe_validation | |
| super options.map(&:typeface) | |
| end | |
| end | |
| UserConfig.extend_schema do | |
| group :sitelenpona do | |
| defaults = SPGen::DrawOptions.new | |
| option :fg_color, TString.new, default: defaults.fg_color | |
| option :bg_color, TString.new, default: defaults.bg_color | |
| option :fontsize, | |
| TInteger.new(min: 1, max: 128), default: defaults.fontsize | |
| option :fontface, TSPFontSelect.new, default: defaults.fontface | |
| option :glyphs, TString.new | |
| end | |
| end |
Lines 5 to 11 in c6803f4
| ServerConfig.extend_schema do | |
| option :use_bare_colors, TBoolean.new, default: false | |
| option :auto_assign_colors, | |
| TEnum.new(%w[on_join on_screening_pass never]), | |
| default: 'on_join' | |
| end |
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 see, I was thinking of a single server bot oops
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.
Every nix file in this repo got formatted for some reason, unnecessarily. Please don't needlessly reformat code and instead follow the style of the code around 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.
It's in a separate commit, it's annoying to manually format (and my editor formats on save :P).
nixfmt is the official formatter for nix code, is there a reason why you don't want to adopt it?
Is there some other formatter you prefer?
I can split it to a separate PR or to the update PR.
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.
My code was all written before nixfmt existed. If I'm moving to a formatter style, I'd like to do it on my own terms, or in a separate PR, not as part of an unrelated PR. Especially because nixfmt makes my code look ugly since I don't write nix like nixpkgs does lol
| # rubocop: enable Metrics/MethodLength | ||
|
|
||
| def self.has_addnote_access?(event) | ||
| required_role_ids = Array(QBot.config&.roles&.addnote_access) |
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.
Again, this should be a per-server configuration option. Make sure to specify in the option's description that an empty list means that everyone has addnote permissions.
Option names and descriptions are kept in the locale definitions: https://github.com/arch-community/qbot/blob/main/share/locales/en.yml
|
hmm I wonder how this works |
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.
Pull Request Overview
This PR adds a new configuration option roles.addnote_access to control access to the addnote command by Discord role IDs. The change allows server administrators to restrict who can add notes to the bot based on their Discord roles.
Key changes:
- Added role-based access control for the
addnotecommand - Updated configuration schema to include the new
roles.addnote_accessoption - Applied consistent code formatting across Nix configuration files
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| modules/notes.rb | Implements role-based access control for the addnote command |
| nix/module.nix | Adds the new roles.addnote_access configuration option to the Nix module |
| config/global.yml.example | Provides example configuration for the new role access feature |
| shell.nix | Code formatting improvements |
| flake.nix | Code formatting improvements |
| default.nix | Code formatting improvements |
|
|
||
| roles = { | ||
| addnote_access = | ||
| mkOpt (listOf int) "List of Discord role IDs that are allowed access the addnotes command" |
Copilot
AI
Aug 11, 2025
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's a missing word in the description. It should be "that are allowed access to the addnotes command" or "that are allowed to access the addnotes command".
| mkOpt (listOf int) "List of Discord role IDs that are allowed access the addnotes command" | |
| mkOpt (listOf int) "List of Discord role IDs that are allowed to access the addnotes command" |
|
it doesn't work well at all! who knew? |
Bot not tested but there's a tester in the commit msg of the second commit