-
Notifications
You must be signed in to change notification settings - Fork 1
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
Bridge polls #4
base: main
Are you sure you want to change the base?
Bridge polls #4
Conversation
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.
Thank you for the new feature, @clehner! Overall, LGTM. I'm assuming you've tested this -- remember that people get /super cranky/ during a call when the cgbot disappears/crashes. It's set up to re-join, but if there is some bug that happens when it rejoins and it crashes again, it stops the groups ability to scribe minutes... just putting that out there as a concern and to underscore that any addition to cgbot needs copious amounts of testing.
lib/command.js
Outdated
|
||
// the current speaker queue | ||
const speakerQueue = []; | ||
|
||
export async function processCommand({nick, message, participants, say}) { | ||
function nonCryptographicHash8(data) { | ||
return crypto.createHash('md5').update(data).digest('hex').slice(0, 8); |
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.
Don't use known broken hash functions, even if md5, even if it's safe to do so in this particular instance. Other developers will come in behind you and copy what you did here, which might proliferate the bad md5 function elsewhere. Just do a sha-256 and take the last 8 characters, the result is the same, but without risking bad practice proliferation.
Granted, this entire file is bad code due to 10+ years of neglect; we need a rewrite at some point in the near future.
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.
Fixed: a19e641
lib/command.js
Outdated
if(args.length > 0) { | ||
pollNum = args.shift().replace(/^#/, ''); | ||
if(isNaN(pollNum)) { | ||
say('Expected poll number'); |
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.
Remember that almost no one will remember the syntax. If you raise an error, tell people what a typical good command looks like.
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 part of the code (the getPoll
function) can be reached from multiple commands (vote, votes, and poll).
The poll number is the last part to parse: for the vote
command the vote argument has already been parsed at this point; for poll
and votes
, the poll is the only argument.
How's this? ade680c
21:46 <@cel> cgbot-cel: vote 4 on \a
21:46 <cgbot-cel> Unrecognized poll "\a". Try e.g. "1", "#1", or "poll 1".
21:47 <@cel> cgbot-cel: poll asdf
21:47 <cgbot-cel> Unrecognized poll "asdf". Try e.g. "1", "#1", or "poll 1".
21:49 <@cel> cgbot-cel: votes 1!
21:49 <cgbot-cel> Unrecognized poll "1!". Try e.g. "1", "#1", or "poll 1".
lib/command.js
Outdated
} | ||
} else { | ||
if(polls.count === 0) { | ||
say('No polls.'); |
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.
Same as above, how does one create a poll if there are no polls?
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.
lib/command.js
Outdated
pollNum = polls.count; | ||
} | ||
if(args.length > 0) { | ||
say('Unexpected data after poll number') |
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.
Same, show them what a correct command looks like.
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.
In this case the command can (maybe) be corrected by removing the unexpected trailing arguments. To output the whole (modified) command may be possible but not straightforward, because the command/arguments are parsed mutably (using shift
). But the unrecognized part can be highlighted. How's this? bc189e3
22:06 <@cel> cgbot-cel: vote 1 on 4 a
22:06 <cgbot-cel> Unexpected data after poll number. Try again without this part: "a"
22:07 <@cel> cgbot-cel: poll 1 2
22:07 <cgbot-cel> Unexpected data after poll number. Try again without this part: "2"
lib/command.js
Outdated
} | ||
const pollId = polls.idsByNum[pollNum]; | ||
if(!pollId) { | ||
say(`Unknown poll "${pollNum}"`); |
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.
Might want to list known poll numbers here.
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.
lib/command.js
Outdated
if(args[0] === 'on') { | ||
args.shift(); | ||
} else { | ||
say('Try: vote option X on poll Y'); |
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.
Might try making "on poll Y" optional and just use the last poll sent to the channel?
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.
That is the case. If "on poll Y" is omitted, the most recently created poll is used, and this highlighted code path is not reached.
lib/command.js
Outdated
answersMap[answerI] = true; | ||
const answer = poll.msg.answers[answerI]; | ||
if(answer == null) { | ||
say(`Unknown option "${option}"`); |
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.
People might get confused here since IRC is async, so people's votes might not get counted and they might not realize?
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.
Do you mean in this particular error (unknown option chosen)? Or in all the error/failure cases for this command?
The responses could say something additional like "vote not processed, try again" to be more explicit about the vote not being counted? Or the response could be targeted to the user (maybe using <nick>, message
since nick: message
might get interpreted as scribing?) so they would be more likely to notice that it is relevant to them. There could also be unhandled vote attempts that are not matched at all by this handler and so would not generate a response. But a successful vote will result in a IRC message saying that the answer/vote took place, as generated by the bot when it receives back its own answer-poll
xmpp message, and that (along with the vote appearing in the Jitsi UI) is the positive confirmation.
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.
Changed to list available options if an unknown option is picked: 2bb6c53
22:14 <@cel> cgbot-cel: create poll Test? | Yes | No
22:14 <cgbot-cel> cel created a poll #1 with 2 options.
22:14 <cgbot-cel> POLL: Test?
22:14 <cgbot-cel> ... Option 1: Yes
22:14 <cgbot-cel> ... Option 2: No
22:14 <@cel> cgbot-cel: vote 3
22:14 <cgbot-cel> Unknown option "3" on poll #1. Available options: 1,2
// the message is a poll answer message | ||
if(msg && msg.type === 'answer-poll') { | ||
const poll = polls.byId[msg.pollId]; | ||
if(!poll) { | ||
console.error('Unknown poll ' + msg.pollId); | ||
return; | ||
} | ||
const answers = poll.msg.answers; | ||
poll.votes[msg.voterId] = msg; | ||
const message = `${msg.voterName} answered poll #${poll.num}: ` | ||
+ msg.answers.map((chosen, i) => chosen | ||
? `(${i+1}) ${answers[i]}` : '') | ||
.filter(Boolean).join(', '); | ||
ircSay(message); | ||
if(ircOptions.log) { | ||
log({logFileBasePath, nick: ircOptions.nick, message}); |
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.
Is there a "finalize poll" or "close poll" command that shows the totals? It might be difficult for more complex polls to understand the final tally? I know we have the "PROPOSAL / RESOLUTION" syntax already for proposals... but POLLs are different, maybe?
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.
The votes
IRC command added in this PR shows the votes summary for a poll (i.e. the latest poll, or a previous one if referenced); it aims to present similar information as the web view in Jitsi (i.e. number of votes per option, names of voters, and percentage of voters who chose each option). In the web UI, the number of voters is shown in the "details" view but in this IRC implementation I have it show every time - If that is excessive with many voters, it could be adjusted.
There's no operation in the Jitsi front-end to close, remove or finalize a poll. New polls can be created and the old ones remain. The closest I can see that we could do to close a poll purely in the bot, I think would be the following:
- The bot could state in IRC and Jitsi chat that the poll is to be considered closed.
- The bot could zero out the poll - removing all existing votes. I think this could work because the web UI and conference system does not appear to verify that a vote message's sender property corresponds to the actual message sender; also, even though a user cannot remove their vote in the web UI (the client requires at least one option chosen), I don't think that is enforced internally.
- The bot could detect and revert votes made on a poll that has been marked closed. Additionally it could send a private message to the voter as a warning/alert.
Edit: actually, a poll can also be reset by sending a new-poll
message with the existing poll id. That clears the votes., and can change the poll question and options. So, to close a poll, the options could be set to an empty list, and the title have e.g. " [CLOSED]" appended to it. It will still appear in the polls list but no one will able to vote in it.
Polls are different from proposal/resolutions but might be similarly useful in some situations.
For example, to emulate the current proposal method of evaluating a proposed resolution, a poll could have the question be a proposal, and options "+1", "0", and "-1" (or "yes", "no", "abstain", "block", "still deciding").
If there are multiple alternatives to be considered as proposals, a single poll could have a more general question, and give the proposed alternatives as options. Participants can vote for more than one option in a poll. This could be useful for comparing multiple possible alternatives. This might not have the nuance of the earlier method, as each option would get only a binary response per person (or ternary if you consider not voting as a response) rather than a spectrum of responses. It could be followed up with a poll specifically for an option to consider further.
lib/manage.js
Outdated
if(ircOptions.log) { | ||
log({logFileBasePath, nick: ircOptions.nick, message}); | ||
} | ||
const message2 = `... Question: ${msg.question}`; |
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.
const message2 = `... Question: ${msg.question}`; | |
const message2 = `POLL: ${msg.question}`; |
Let's do this to keep with the current syntax for "PROPOSAL/USE CASE/RESOLUTION/etc."
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.
Applied: f119cc3
lib/manage.js
Outdated
// bot, so use senderName instead. | ||
const bridged = stanza.attrs.from.endsWith('/cgbot'); | ||
const name = bridged ? msg.senderName : nick; | ||
const message = `${name} created poll #${pollNum} with ${numOptions} options.`; |
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.
const message = `${name} created poll #${pollNum} with ${numOptions} options.`; | |
const message = `${name} created a poll #${pollNum} with ${numOptions} options.`; |
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.
Applied: db3508b
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.
+1 to merge as long as comments are processed.
@msporny thanks for the review. I'll be addressing the things you pointed out. I believe I've tested (manually) all the added functionality so far. After making changes I'll test again and write what was tested. |
- Set bot sender id/name for new-poll message. - Put chat nick id in question prefix and in separate message property. - Strip question prefix when processing message.
Prevent accidentally resetting poll by reusing id.
@msporny Other changesAdditionally I made some other minor changes, mostly for error handling; the most significant one is probably 38c1322 (Set poll sender name), described below: 38c1322When creating a poll from chat (IRC/XMPP), the nick could not be set in the poll message; it would appear empty because the sender id was not a valid Jitsi chat participant. The fix implemented is to put the nick in the poll question (e.g. Before
After
Documentation of IRC commandsThe bot has a help command that responds with a link to http://bit.ly/2CR6pZK, which is a redirect to https://github.com/w3c-ccg/w3c-ccg.github.io/blob/master/irc_ref.md. I opened a PR to add documentation to that page for the poll commands as currently implemented: w3c-ccg/w3c-ccg.github.io#27 TestingI tested the commands and operations in various scenarios, as follows. Tests
|
Oh, no... did I never merge this!? I thought I merged this a while ago... just noticed as I went to look at the latest code base for cgbot. Looks like we need to do some merging... I'll try to get mainline fixed first... then merge this on top? |
OK I fixed some merge conflicts just now. |
Two-way bridging of Jitsi polls with IRC.
Polls are identified by a number starting at one. Options in each poll are also identified by a number starting at one.
Poll creation
From Jitsi
From IRC
Voting in a poll
From Jitsi
From IRC
Multiple votes:
Querying poll state
Show info about the current poll or a particular poll:
List all polls since the bot joined:
Show votes on the most recent poll or a particular poll: