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

implement /destroy command in muc #1388

Merged
merged 1 commit into from Jan 3, 2019

Conversation

@ChaosKid42
Copy link
Contributor

commented Dec 22, 2018

This implements the possibility to destroy a MUC.

@ChaosKid42 ChaosKid42 force-pushed the ChaosKid42:implement_destroy_muc branch 4 times, most recently from 67f1ff6 to 5f8f315 Dec 22, 2018

@jcbrand jcbrand force-pushed the conversejs:master branch 3 times, most recently from 8ca2731 to f16b6d2 Jan 2, 2019

@jcbrand
Copy link
Member

left a comment

Thanks @ChaosKid42!

I have a few small improvement suggestions.

}
this.destroy(
this.model.get('jid'), args[0],
this.onDestroySuccess.bind(this), this.onCommandError.bind(this));

This comment has been minimized.

Copy link
@jcbrand

jcbrand Jan 3, 2019

Member

Instead of making a new method onDestroySuccess, you can simply use this.close here, and you can use an arrow function to avoid having to use .bind.

In destroy, you can simply return _converse.api.sendIQ(iq) so that it returns a promise.

So it could be something like this:

 this.destroy(this.model.get('jid'), args[0])
       .then(() => this.close())
       .catch(e => this.onCommandError(e));
const destroy = $build("destroy");
const iq = $iq({to: groupchat, type: "set"}).c("query", {xmlns: Strophe.NS.MUC_OWNER}).cnode(destroy.node);
if (reason && reason.length > 0) { iq.c("reason", reason); }
return _converse.api.sendIQ(iq).then(onSuccess).catch(onError);

This comment has been minimized.

Copy link
@jcbrand

jcbrand Jan 3, 2019

Member

I'd like us to move away from using callbacks in favour of using promises.

Here, you can simply return return _converse.api.sendIQ(iq) and then later when you call destroy you can either use then and catch or async/await.

spyOn(_converse.connection, 'sendIQ').and.callFake(function (iq, callback, errback) {
sent_IQ = iq;
IQ_id = sendIQ.bind(this)(iq, callback, errback);
});

This comment has been minimized.

Copy link
@jcbrand

jcbrand Jan 3, 2019

Member

Not a dealbreaker for merging, but I'd like us to move away from using var in favour of let and const.

@ChaosKid42 ChaosKid42 force-pushed the ChaosKid42:implement_destroy_muc branch 2 times, most recently from 564e03d to 5ec837c Jan 3, 2019

@ChaosKid42 ChaosKid42 force-pushed the ChaosKid42:implement_destroy_muc branch from 5ec837c to 2315ef1 Jan 3, 2019

@ChaosKid42

This comment has been minimized.

Copy link
Contributor Author

commented Jan 3, 2019

@jcbrand Thanks fo your review. Updated the PR.

@jcbrand jcbrand merged commit eacd7fd into conversejs:master Jan 3, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ChaosKid42 ChaosKid42 deleted the ChaosKid42:implement_destroy_muc branch Jan 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.