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

unban+admin #80

Closed
wants to merge 4 commits into from
Closed

unban+admin #80

wants to merge 4 commits into from

Conversation

ghost
Copy link

@ghost ghost commented May 6, 2020

This patch builds on #78 and #77 and adds:

  • unban
  • addMod
  • removeMod
  • addAdmin
  • removeAdmin
  • moderation.getRole
  • moderation.modInfo
  • moderation.listMods
  • moderation.listAdmins

Comment on lines +127 to +128
- `channel` (string, default="@") - the channel to ban the user from. `"@"` is cabal-wide.
- `reason` (string, default="") - reason for the banishment.
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: should be unban


Valid `opts` include:

- `channel` (string, default="@") - the channel to ban the user from. `"@"` is cabal-wide.
Copy link
Member

Choose a reason for hiding this comment

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

does the underlying view allow adding a mod/admin for a single channel?

nitpick: says ban. same case for the next four segments containing channel

this._modCmd('admin/remove', key, opts, cb)
}

Cabal.prototype._modCmd = function (type, key, opts, cb) {
Copy link
Member

Choose a reason for hiding this comment

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

i like this structure. i was considering something similar for parsing the extended moderation syntax in cabal-client

Copy link
Member

Choose a reason for hiding this comment

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

Nice!

@cblgh cblgh mentioned this pull request May 6, 2020
@cblgh
Copy link
Member

cblgh commented May 6, 2020

looks like the failing ci requires pushing package-lock.json, to make sure the materialized-group-auth version is ^1.2.0

@ghost
Copy link
Author

ghost commented May 6, 2020

I am very hesistant to make any changes to a package-lock in a PR. This creates automatic merge conflicts between any other open PRs.

It also makes the patch very noisy and hard to read.

@cblgh
Copy link
Member

cblgh commented May 6, 2020

cool, that makes sense. i'll try to remember to make that part of my workflow going forward, too

Copy link
Member

@hackergrrl hackergrrl left a comment

Choose a reason for hiding this comment

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

Yay! Excited to see this coming together. I'm going to close #78 in favour of getting this in.

this._modCmd('admin/remove', key, opts, cb)
}

Cabal.prototype._modCmd = function (type, key, opts, cb) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

if (typeof key === 'string') return key.length === 64 && /^[0-9a-f]+$/.test(key)
else if (Buffer.isBuffer(key)) return key.length === 32
if (typeof key === 'string') return /^[0-9a-f]{64}$/.test(key)
else if (Buffer.isBuffer(key)) return key.toString('hex').length === 32
Copy link
Member

Choose a reason for hiding this comment

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

Glad you noticed this bug.

@@ -134,11 +187,16 @@ module.exports = function (cabal, modKey, db) {
})
if (--pending === 0) cb(null, banned)
})
},

getRole: function (core, opts, cb) {
Copy link
Member

Choose a reason for hiding this comment

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

Great API expansion! Do you have capacity to add these to the README docs as you go?

@hackergrrl hackergrrl mentioned this pull request May 7, 2020
@ghost ghost mentioned this pull request May 18, 2020
@ghost
Copy link
Author

ghost commented May 18, 2020

closing in favor of #83

@ghost ghost closed this May 18, 2020
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants