brigand / jellobot

16 stars 13 forks source link

Factoids Rewrite #36

Open brigand opened 5 years ago

brigand commented 5 years ago

This is initial work for the reimplementation of factoids so we can sunset ecmabot in the near future (a bot we can't update isn't healthy). Please open issues or message me if other things need to be done before then.

Future plans (maybe tomorrow):

In case it's confusing, the .persistent.js file is immune to HMR, while .internal.js can be evicted from the cache.

Edit: will add some tests tomorrow.

brigand commented 5 years ago

@ljharb @caub please review this if you have time.

Changes without the extra lint fixes

ljharb commented 5 years ago

What's the overarching goal here? What's wrong with the current factoids system and format that needs changing?

brigand commented 5 years ago

I would like to work on the new features mentioned above, and be able to add new features in the future.

I actually intend to change the format a little bit more. It'll be simpler if even the first !learn is in the changes array (with previous: null). The first item in the array is "most recent" and the last item is "the initial value".

This allows implementing access control + personal factoids with changes.find(item => item.editor === msg.from || item.global), where global is set by an elevated user on !learn, or on someone else's edit via a to-be-implemented command.

There's quite a bit of duplication in the current format, but only after the first edit. It's just pretty weird how the data is laid out. It would be quite awkward to implement various things in the current format.

ljharb commented 5 years ago

I'm quite interested in access controls.

When you say "personal factoids", however, that seems rife for abuse. I actively would not want individuals to be able to store their own private factoids in the bot.

brigand commented 5 years ago

Sure, makes sense. Then the check for retrieving a factoid is reduced to changes.find(item => item.global), and the pending changes will only appear in the factoid file, waiting to be approved.

brigand commented 5 years ago

Some updates...

The config file now has an entry for factoid moderators.

{
  "plugins": {
    "jsEval": { "timeout": 5000 },
    "factoid": {
      "moderators": ["GreenJello", "Other", "Names", "2B", "Added"]
    }

Anyone can !learn and they get a different message depending on if they're a moderator or not.

NormalUser: !learn foo = bar bot: NormalUser, change proposed to "foo"

Moderator: !learn foo = bar bot: Moderator, got it. I'll remember this for when "!foo" is used.

Moderator: !publish foo bot: Moderator, done. Everyone will now see the previous draft when "!foo" is used.

The JSON file format has been reduced to the following:

  "classes": {
    "type": "factoid",
    "popularity": 5,
    "editors": [
      "ljharb",
      "some-user"
    ],
    "changes": [
      {
        "date": "2019-10-07T01:38:06.850Z",
        "editor": "some-user",
        "value": "is this java?",
        "live": false
      },
      {
        "date": "2019-01-29T00:18:51.526Z",
        "editor": "ljharb",
        "value": "Class hierarchies? Don't do that! http://raganwald.com/2014/03/31/class-hierarchies-dont-do-that.html (See also, !inheritance)",
        "live": true
      },
      {
        "editor": "mjcd",
        "date": "2019-01-28T12:43:13.327Z",
        "value": "Class hierarchies? Don't do that! http://raganwald.com/2014/03/31/class-hierarchies-dont-do-that.html (See also, !inheritance)",
        "live": true
      }
    ]
  },

I might also get rid of "editors".

Given that config (note the first item is a proposed change, i.e. live: false):

Anyone: !classes bot: Anyone, Class hierarchies? Don't do that! http://raganwald.com/2014/03/31/class-hierarchies-dont-do-that.html (See also, !inheritance)

Moderator: !publish classes bot: Moderator, done. Everyone will now see the previous draft when "!classes" is used.

Anyone: !classes bot: Anyone, is this java?

Also, !forget is implemented for both moderators and other users. In the store it simply adds a change where the value is null.

I still need to work out the best way to review factoids others have proposed. Suggestions welcome on that. Maybe we just take the most recent proposals and put them in a markdown file in a gist.

ljharb commented 4 years ago

@brigand moderator-only text should only appear in PM

brigand commented 4 years ago

You're free to run the commands in PM. Is that insufficient?

ljharb commented 4 years ago

Yes - it would be pretty bad if "being a moderator" meant that factoids i triggered (or that were triggered pointing at me) contained noise that was irrelevant to the majority of the people that saw it.

People without permissions should ideally not even know that those commands exist.

brigand commented 4 years ago

Triggering factoids doesn't consider the moderator status of anyone; only !learn and !publish consider this. You're always free to execute those in private messages, and if you do then no one will even know you're a moderator.

For those commands, I think it's important that we signal to users that e.g. !learn foo = bar won't be immediately visible when !foo is used, and !publish should present an error, even if only for the cases where a human that is a moderator is presenting with a nick not in the moderator list.

We could require these to be run in private messages, but I think we can leave the agency of where to invoke them in the hands of individual users.

Edit: to clarify my previous message, "Anyone" means any user on IRC, in the moderator list or not.

ljharb commented 4 years ago

If I wanted to truly hide being a moderator, I'd have to use !learn in a channel and not have the moderator-specific commands show up.

brigand commented 4 years ago

That actually doesn't really solve it, because there will be a difference if you do !learn foo = bar in a channel and someone immediately runs !foo. It'll either return the old value if you're not a moderator, or the new value if you are.

The only way around that is for the bot to make your edit live at a random time, simulating an approval from a human. I don't think we want to go that route.

What is your cause and level of concern with people knowing someone is a moderator?

ljharb commented 4 years ago

I suspect it will immediately and frequently generate complaints about "i want to be a moderator", "why is that person a moderator", etc; it will also generate confusion and frequent re-explanation about how permissions work.

brigand commented 4 years ago

I think you're right on that, but I'm not clear on what the solution is. Should we only allow commands like !learn in private messages?

ljharb commented 4 years ago

Maybe even moderator-created learn commands should require a PM-only publish command?

brigand commented 4 years ago

I like that idea. I'll implement it that way.