Don't re-assign user if they're already assigned

[awaitlink]

Example:

1. Comment by @user:

   @rustbot claim
    
   [some other text]
   

   Bot assigns @user (directly or indirectly) and edits original issue comment if needed.

2. @user edits their comment:

   @rustbot claim
   
   -[some other text]
   +[some different text]

   Bot unassigns @user (directly or indirectly) and edits original issue comment if needed, then assigns @user (directly or indirectly) and edits original issue comment if needed.

rust-lang/rust example

Perhaps the bot should not do anything in 2, if possible. Especially if the target user is already assigned.

[kellda]

I'm interested to work on this. What would be the best strategy to implement it ?

• Bypass command if it is an edit, something like done here
• Always check the target user isn't already assigned
• Somehow process only changed text

What happens / should happen when a user delete a command from its comment ?

[Mark-Simulacrum]

I would probably start by just checking if they're already assigned, since we have access to that (or can easily get it). Ideally, yes, we'd only process the changed text and if that overlaps with the command then rerun it but otherwise not. That's a harder task but if you want to take it on go for it :)

[kellda]

I'll see later if I want to try to process only the diff, for now I implemented the check.
However I see one caveat to this implementation:

1. Comment by @someuser:

   @rustbot claim
   
   [some other text]

   Bot assigns @someuser.

2. For some reason @otheruser claim:

   @rustbot claim

   Now the bot unassigns @someuser and assign @otheruser.

3. @someuser edits their comment:

   @rustbot claim
   
   -[some other text]
   +[some different text]

   Bot unassigns @otheruser and reassign @someuser

1 and 2 are how it should work (I guess) but 3 may be unexpected.

I also think it's better to stay coherent between edits on PR/issue description (currently bypass) and edits on PR/issue comment

[Mark-Simulacrum]

PR/issue description is edited much more frequently than comments; that's the reason for the divergence there. I think the behavior that you've noted is fine for now, I'm hoping that someone (maybe me) can find the time soon to nail down the diffing support which would essentially eliminate this problem I think.

[kellda]

A with diff / edit support comes the question of what to do when a command is suppressed, e.g.

-@rustbot claim

[some other text]

Should I be unassigned from the issue in the above case ?

[Mark-Simulacrum]

No, I think we shouldn't ever try to undo commands, but instead just run "new commands" (including potentially an edited old command). But to start off I wouldn't even try to intelligently diff commands -- my suggestion would be to do something like running the parsing algorithm on the before and the after versions of the comment/description, then we run any commands in the new version that aren't present in the old version (or are different). This may need some deriving of PartialEq or so (or possibly manual impls for some of the specific commands, but I'm hopeful this isn't necessary).

[kellda]

I still wonder if it makes sense to allow issuing commands when editing because:

• You can always add a comment
• If there are comments after the one you're editing, the result (rustbot assigned rustbot message) of your command may be far away from the actual command.

[Mark-Simulacrum]

It's a much smoother workflow to edit a comment than leaving ~N comments when trying to get a command to work. Ideally we'd have a workflow that didn't mean that each comment generates notifications for everyone on the thread, but today that's the case and editing comments is the only way to get around that.

[kellda]

OK, now I see how it can be usefull.

I'm interested to working on this. I think the easier way is to parse commands and execute only if it overlaps with edits (something like whats done to exclude code blocks). I think parsing twice is quite hard to do because it will need a lot of refactoring to delay execution of commands.

I have some trouble understanding what the changes field looks like in the JSON payload. Could you give me some example of events with this field ?

[Mark-Simulacrum]

Yeah, it might be easier to do something like what you suggest, I'm fine with pretty much whatever approach works.

I can try to pull out a few examples from the logs we have, yeah. Will post here when I get a chance to do so.

[kellda]

Hi,

I may have some free time in the coming weeks and I'd like to contribute to this project.

Were you able to pull out example from the logs for edited messages ? I think I eventually undersood the spec for the API, but some examples would still be useful.