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

Jump to bottom

fix moderator commands that change affiliation #1413

Merged
merged 1 commit into from Feb 5, 2019
Merged

fix moderator commands that change affiliation #1413

merged 1 commit into from Feb 5, 2019

Conversation

ChaosKid42
Copy link
Contributor

@ChaosKid42 ChaosKid42 commented Jan 14, 2019 •
edited

I believe it should be possible to change affiliation of non-occupants. Currently converse rejects this. This tries to fix it.

@ChaosKid42 ChaosKid42 force-pushed the fix_moderator_commands_that_change_affiliation branch from 1be5087 to dd166e7 Compare January 14, 2019 21:03
@ChaosKid42 ChaosKid42 changed the title WIP: fix moderator commands that change affiliation fix moderator commands that change affiliation Jan 14, 2019
@ChaosKid42 ChaosKid42 force-pushed the fix_moderator_commands_that_change_affiliation branch from dd166e7 to ad94201 Compare January 14, 2019 21:11
@ChaosKid42 ChaosKid42 force-pushed the fix_moderator_commands_that_change_affiliation branch from ad94201 to cdc3b27 Compare January 23, 2019 16:41
@ChaosKid42 ChaosKid42 force-pushed the fix_moderator_commands_that_change_affiliation branch 2 times, most recently from c147378 to 83fbffb Compare January 28, 2019 20:41
@jcbrand
Copy link
Member

jcbrand commented Jan 29, 2019

This will only work if you provide a valid JID instead of just a nickname right?

I think you'll need to do some validation that it's really a JID being passed in.
You can use the isValidJID utility method.

@ChaosKid42 ChaosKid42 force-pushed the fix_moderator_commands_that_change_affiliation branch from 83fbffb to 1c9ed79 Compare January 29, 2019 13:10
@ChaosKid42
Copy link
Contributor Author

@jcbrand Ok. Added the check.

jcbrand
jcbrand reviewed Jan 30, 2019
View reviewed changes
src/converse-muc-views.js Outdated
@@ -841,7 +841,8 @@ converse.plugins.add('converse-muc-views', {
);
return false;
}
if (!this.model.occupants.findWhere({'nick': args[0]}) && !this.model.occupants.findWhere({'jid': args[0]})) {
if (!(_.includes(['admin', 'ban', 'owner', 'member', 'revoke'], command) && u.isValidJID(args[0])) &&
Copy link
Member

@jcbrand jcbrand Jan 30, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please create a const AFFILIATION_CHANGE_COMANDS at the top of the file which is set to these commands and then use that here.

Would be nice if you also wrapped the line (below).

And then please rebase again, so that I can merge from github.

Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jcbrand Done.

@ChaosKid42 ChaosKid42 force-pushed the fix_moderator_commands_that_change_affiliation branch 4 times, most recently from a18db46 to 9b3f36d Compare February 1, 2019 10:02
@ChaosKid42
Copy link
Contributor Author

@jcbrand Friendlly reminder: Do you still plan to merge this PR?

@jcbrand
Copy link
Member

jcbrand commented Feb 1, 2019

Yes, but I first need to figure out why the tests on Travis are failing. They're all green on my own machine.

@ChaosKid42 ChaosKid42 force-pushed the fix_moderator_commands_that_change_affiliation branch from 9b3f36d to 9a0daaf Compare February 3, 2019 14:18
@ChaosKid42
Copy link
Contributor Author

Thanks for fixing the tests. This PR now passes the checks on travis.

@jcbrand jcbrand merged commit 5e1f578 into conversejs:master Feb 5, 2019
@ChaosKid42 ChaosKid42 deleted the fix_moderator_commands_that_change_affiliation branch February 10, 2019 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcbrand jcbrand jcbrand left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ChaosKid42 @jcbrand