Hey, thanks for the patch!

On 17 April 2017 at 13:43, Lennart Sauerbeck
<de...@lennart.sauerbeck.org> wrote:
>
...
> - In line 40 of the patch, I create a temporary variable to find out
> whether the loop was run at least once. Is there a different mechanic in
> Lua to do this nicer, or is this the way to go?

I agree it's a little annoying, but I don't think there is a better
approach that is reasonable. The stanza API doesn't have a method to
count tags of a particular type, but even if it did it would just be
an iterator just the same as what the code is already doing, and I
generally dislike hidden performance costs like that.

So I think it's fine as it is.

> - Regarding the error handling in lines 49, 55 and in-tree 801 (the last
> one is not in the patch because I did no changes there): Is it okay to
> just send multiple error messages or should they be collected somehow
> and sent as batch after the complete stanza was processed? I could find
> nothing about this in either the MUC XEP, nor in the three (or so) base
> RFCs of XMPP.

Yeah, this is a problem. Of the three stanza types, changing
affiliations uses <iq/> which is always request (get/set) -> response
(result/error), and it's not acceptable to have multiple responses to
a single request. Otherwise if you were the client, you wouldn't know
when the responses had stopped coming, for example.

I don't have a good suggestion right now. There are a few solutions
that come to mind:

  - Always report success, just ignore errors. The client will see the
affiliation change notifications generated by the room for the
successful changes.
  - Treat the request as an atomic change: succeed for all, or fail
all (i.e. if one of the JIDs is malformed, don't allow any of the
other changes in the request to take place). This would require work
to either pre-verify the changes (which we don't have code for) or to
roll back changes that were already made (which is a bit hacky, we'd
already have sent out notifications, unless we added a new 'batch
update' version of :set_affiliation()).

> If this patch gets accepted (with possible modifications), I can easily
> cook up one just like it for trunk as well. I already saw that the muc
> code has changed quite a lot, but in regards to this patch it's only for
> the better.

Sounds great - ideally we'd have a version of the patch for trunk
before merging, but let's keep the focus on 0.10 until we figure out
the issue with the errors.

Regards,
Matthew

-- 
You received this message because you are subscribed to the Google Groups 
"prosody-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to prosody-dev+unsubscr...@googlegroups.com.
To post to this group, send email to prosody-dev@googlegroups.com.
Visit this group at https://groups.google.com/group/prosody-dev.
For more options, visit https://groups.google.com/d/optout.

Reply via email to