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

Implementation of XEP-0191 "Blocking command" #829

Merged
merged 5 commits into from
Jun 7, 2016
Merged

Conversation

bartekgorny
Copy link
Collaborator

This PR implements XEP-0191. Mostly as a separate module, though there were some minor changes to mod_privacy (so that it doesn't handle what it should not) and to ejabberd_c2s (since part of privacy logic is there, and there were bits and pieces of blocking commands implementation already).

@Nyco
Copy link
Contributor

Nyco commented May 18, 2016

In the doc/modules/mod_blocking.md:

  • "This extension is allows": remove the "is"
  • "with a single commands.": remove the s, "command"
  • "The protocol is much simpler then in privacy lists.": "The protocol is much simpler than privacy lists."

Otherwise, thanks a lot!

{ok, L} ->
{ok, L};
_ ->
{error, e}
Copy link
Contributor

@ludwikbukowski ludwikbukowski May 23, 2016

Choose a reason for hiding this comment

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

I would catch some Variable instead of wildcard and return it in {error, Other} tuple. I think bare e is too little meaningful

@bartekgorny bartekgorny merged commit 2dc52e7 into master Jun 7, 2016
@michalwski michalwski deleted the blocking_commands branch June 14, 2016 07:58
@@ -0,0 +1,641 @@
%%==============================================================================
Copy link
Contributor

Choose a reason for hiding this comment

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

This suite looks quite comprehensive. I don't see it run though.

  1. It's not present here: http://mongooseim-ct-results.s3-website-eu-west-1.amazonaws.com/branch/master/1619/1/big/index.html
  2. The coverage for mod_blocking is very low: https://coveralls.io/builds/6502277/source?filename=apps%2Fejabberd%2Fsrc%2Fmod_blocking.erl
  3. It's not present in default.spec

How can we be sure the implementation works as expected? Do we need this suite at all?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I'd rather make most of the tests parallel, if this is possible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Grrr, I forgot to add it to default.spec...

@ludwikbukowski
Copy link
Contributor

since the blocking_commands module uses privacy lists API functions there shouldn't be any inconsistency on the backend side. Due to XEP-0191:Blocking Commands/Relationship to Privacy Lists seems that protocol also allows such a situation but:

Because of the potential for confusion between block lists and privacy lists, it is NOT RECOMMENDED for a client to request both the block list and privacy lists in the same session.

In my opinion, If we'd like to allow users to use both modules maybe we should intruduce some tests with scenarios where client uses both privacy and blocking commands

@michalwski
Copy link
Contributor

I think we shouldn't allow users to use both modules in the same session. And since blocking commands are simpler than privacy lists and do most of the job user wants, I think we should have only mod_blocking enabled by default. If someone needs more advanced features provided by mod_privacy, the module can be enabled.

@bartekgorny
Copy link
Collaborator Author

Needs some testing, but can be done, no problem

@bartekgorny
Copy link
Collaborator Author

edit: nope, it is a problem, blocking commands don't work because privacy backend is not loaded. Can probably be worked around somehow, but is it worth it? Client has been warned...

@michalwski
Copy link
Contributor

Right, I've forgotten about this. Let's leave it as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants