[D7] xmpp support chat

Git clone failed for http://git.drupal.org/sandbox/Sergey_Stepanov/2381557.git while invoking http://pareview.sh/pareview/httpgitdrupalorgsandboxSergey_Stepanov238155...

Git default branch is not set, see <a href="https://www.drupal.org/node/1659588">the documentation on setting a default branch</a>.
There is a git tag that has the same name as the branch 7.x-1.x. Make sure to remove this tag to avoid confusion.

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Hello serg_admin, your module is quite interesting. I would exploit it for installing a server side chat bot. Please check the following, unfortunately the installation process was quite unfriendly:

Automated review
Seems to be OK, please report the link on this page to help reviewers to quickly take a glance at it.
http://pareview.sh/pareview/httpgitdrupalorgsandboxsergeystepanov2381557git

No duplication
You say Different from other similar projects - it is full open source and do not need to use any comercial services. Can you please put a reference to those projects you already identified? This would allow you to be more detailed about what your contribution is adding to the existing ones.

README.txt/README.md
The README file, together with the project page is a big help during the install process but seems to be neglected. It has many typos inside (you can correct them with some spell checker) and it doesn't fulfill some of the guidelines. Please take a look to https://www.drupal.org/node/2181737 in order to improve it.

Installation Process
Enabling the module break the website. This must not happen, even if the user still didn't download the jaxl library.
As for this is usually googd practice to warn the user that the module has been succesfully enabled but the libraries are needed,
As an example, you can place this notice inside the hook_install() by mean of drupal_set_message(...).

Fatal error: Class 'JAXL' not found in /home/artsakenos.tk/websites/drupal.test/sites/all/modules/custom/xmpp_support_chat/xmpp_chat_check_operator.inc on line 78

Call Stack:
0.0000 328796 1. {main}() /home/artsakenos.tk/websites/drupal.test/index.php:0
0.3773 10049308 2. menu_execute_active_handler() /home/artsakenos.tk/websites/drupal.test/index.php:21
2.1145 24248704 3. drupal_deliver_page() /home/artsakenos.tk/websites/drupal.test/includes/menu.inc:532
2.1204 24325964 4. drupal_deliver_html_page() /home/artsakenos.tk/websites/drupal.test/includes/common.inc:2589
2.1207 24326832 5. drupal_render_page() /home/artsakenos.tk/websites/drupal.test/includes/common.inc:2701
2.6269 28428044 6. xmpp_support_chat_page_build() /home/artsakenos.tk/websites/drupal.test/includes/common.inc:5784
2.6279 28453792 7. XmppSupportChatCheckOperator::getStatus() /home/artsakenos.tk/websites/drupal.test/sites/all/modules/custom/xmpp_support_chat/xmpp_support_chat.module:41
2.6279 28453824 8. XmppSupportChatCheckOperator::startClient() /home/artsakenos.tk/websites/drupal.test/sites/all/modules/custom/xmpp_support_chat/xmpp_chat_check_operator.inc:65
Better explain what to download, and where to install. In the project page you already answer some of these questions. 

The project page
The project page must also be reviewed to correct some typos and be clearest about the installation process.
I don’t understand: Dowload latest version "XMPP suppor chat" and instal it to your site. Maybe you mean you have to download the libraries before than the module itself? Anyway I can’t get rid of the JAXL not found.

Style of Code
Please exploit the t() function all over the module (e.g., also in the user registration interface) to make the module internationalization compliant, with the proper placeholders when you have string with variables inside.

The code structure is well organized. I would keep using English variable names wherever possible. Most of the code would be easier to understand. Also double check the doc blocks, some is missing the parameter.

just as example, here I would use $room instead of $stanza and put a comment on the parameters:

/*  [...] * Event when recive message. */
 static public function funOnMessage($stanza) {

Thank you for your contribution!

Hi,

The project page is a good start, but I would encourage you to improve the English grammar and (where possible) elaborate on the project further. That is, don't assume knowledge that an ordinary reader might not have.

What follows is what I think is an improvement, though you should be able to improve this in turn:

- Try to fill out the Features further.
- Link each of the alternative chat project names to the Drupal project
- JAXL is linked to code.google.com in the Requirements list and linked to Github from the Install list. It might be worth adding some words on how sensitive to version changes you would expect your code to be (i.e. are you using anything in the library that might be considered experimental by the jaxl devs?).

--------------

Description

This module creates a "chat" area on a web page for fast communication between people using the site and specific "operators" (such as for a "live support" chat). Each operator uses an XMPP client (such as Jabber).

The module does not implement an XMPP server: a list is available at XMPP Servers

Features

- enables a site user to communicate textually with a person ("operator") working for the site
- ...
- ...
- this module does not use commercial services.
- you can select your preferred XMPP client and server.

Requirements

- This module requires external XMPP server (tested with eJabber).
- The Libraries API module
- 3rd party library: JAXL
- A "content" region in your theme (most themes have this by default).

Different from other modules for support chat

Some similar Drupal modules (although these do use commercial services) include:

- cSupport Live Chat Plugin
- Live Agent
- My Live Chat
- Purechat Live Chat
- Casengo Live Chat Widget

Install

1. Download this module and copy it to your site, for example using "drush pm-download xmpp_support" or otherwise. See: https://drupal.org/documentation/install/modules-themes/modules-7 for further information.
2. Download JAXL, the php XMPP and HTTP client/server library, from GitHub https://github.com/jaxl/JAXL/archive/v3.x.zip and instal it to folder site/all/libraries/JAXL.
2. Install this module as you would normally install a contributed Drupal module.

Configuration

1. Go to the xmpp_support setup page (/admin/config/services/xmpp_chat_config) and configure the XMPP server's location and two account names:
- first for operator of messenger.
- second for service requests from web client.
2. After submitting options first account must receive request for subscribe, you must allow it through you XMPP client.
3. You can set permission by role defining who can see the chat area.

The Pareview.sh output mentioned there were no simpletests. It is a good idea to implement some for your module for two reasons:

- it enables you to spot when things go wrong in ways you weren't expecting
- creating the tests tends to make you write testable code, which in turn is often better code and also more easily understood.

I would encourage you to have a go.

The following projects are at least related. Please include here an explanation why yours is different, and on the final project page include links to the others.

https://www.drupal.org/project/jabber
https://www.drupal.org/project/livechat
https://www.drupal.org/project/chatroll
https://www.drupal.org/project/drupalchat

There are some other projects which appear to have been abandoned or are now dead. I am unsure if those should 'count' in this respect:

https://www.drupal.org/project/actionjabber
https://www.drupal.org/project/xmppframework
https://www.drupal.org/project/juick
https://www.drupal.org/project/messaging

Hi Serg, I understand why you're having problems with English - yours is much better than I am at non-English - which is why I provided a suggested update. Perhaps you can find a friend who can help?

Thanks for the links. The reason I included the old projects was the collaboration vs competition part of the standards. However, I am not sure how far to take that principle, and because of that I'm not going to insist on anything in respect of those I listed earlier.

Re tests - I have the same problem on the module I'm developing and have adopted a two-pronged approach. First, split out as much code as possible that can be tested in isolation - that is, even if the code is manipulating stuff from the server, it's actual inputs and outputs are not. Then you can write unit tests for those functions very easily. Second - and I'm not saying this is perfect - write a new test class which requires a complete setup - Drupal + XMPP server and more - and write tests there that verify that the units tested earlier work together properly.

One huge benefit to you of doing this would be that you could much more easily verify whether the module works properly with servers other than eJabber.

Hi Serg, the page is looking better, but the bad link to JAXL is still there (see the start of my post #16) and you could usefully use much more of the suggested text I gave you. I would also ask you to link the 'libraries API' list item to the drupal project.

I have had a look at the code. Some general comments first:

- Naming files and Drupal variables: Some use a prefix project name xmpp_chat, while others use use xmpp_support_chat. I don't see any particular reason for this and think it would be much better to use a consistent name, which should also be the name of the module itself.

- On a related note the templates in the module use a number of CSS IDs. The names of these IDs should also be prefixed by the module name (IMO) to avoid conflict with others and actually they shouldn't be IDs: CSS classes should be used instead.

- There are several static php classes implemented in the code. Personally I avoid static classes unless there is an overriding reason, and I can't see one here. Have I missed something? Would it be possible to use normal non-static classes and $this pointers, but create a static instance of it? (My reason for avoiding static classes is that it hinders code reuse and makes code unnecessarily complex to understand).

- There are several spelling mistakes in strings and comments. Enabling spell-check in your editor with an English dictionary enabled should help (though I will point out some in the detailed description).

- A point for future development. Some modules split out the admin/UI into a submodule so that site devs can reduce the attack surface and chance of unintended config changes.

Some more detailed comments:

README.txt:
- Where this file and the d.o project page have the same sections, please keep them the same, to avoid creating confusion.

xmpp_chat_check_operator.inc:
- There are a number of strings using the " quote whereas the guideline for d.o work is to use ' unless you need special characters (e.g. \n).
- Please reduce the length of line 103 e.g. by doing variable_get in a separate statement.
- Line 117 and following: do the variable_get() calls to local vars, run any validation that is appropriate, then create the array.
- General note about static class use applies.

xmpp_chat_message.inc:
- Line 59, 65, 172, 173 and others are too long. Please keep lines under about 80 characters.
- Direct use of $_SESSION is making me nervous about XSS attacks, though that may be unwarranted. I would prefer to see more validation checks done, and if possible a simple function wrapper used.
- The string 'xmpp_suport_chat_sentens' should probably be 'xmpp_support_chat_sentence'
- The variable '$rezult' should probably be '$result'
- Line 178, 'chating': if this is meant to be the continuing-tense spelling of 'chat', it's spelt 'chatting'
- There are quite a few hard-coded constants in the code. e.g. 3000, 1000, 18000. Good programming practice would be to use named constants and/or comments to explain them. It may also be a good thing to pull some (e.g. the cookie timeout?) out into the admin interface.

xmpp_chat_register_user.inc:
- Functions are using the php statement 'echo' (e.g. line 83, 90, 95), while I am used to seeing 'print' in Drupal code. It would probably be better to change this, and even better to move such text out of the logic entirely.
- Line 108: rigistration is spelt registration
- Line 123: comment why this code is commented out, or better delete it.

More:
xmpp_support_chat.admin.inc:

The admin form would benefit from more '#description' entries.

Function comment line 9: comments are best when they say why the code is there, not what it does.
Line 15: Addres is spelt Address.
Line 23: The default for the server domain should not be 'mydomain'. If you must put text here then 'example.org' (or .com) is the right option, but '' would be ok too.
As someone new to setting this up I don't understand why I need to put a server address and a server domain. Could this be explained in #description nodes?
Line 52: requsts is spelt requests. Also between is duplicated.
Line 84: 'Can not sent' -> 'Cannot send'

xmpp_support_chat.info:

Could the CSS stylesheets be included only where the output of the module is used (i.e. only where they're needed)? Include drupal_add_css(), perhaps in the hook_page_build() ?

Line 1: I'm not sure about including 'my_version' in this way. I understand why you put it there, but the reason d.o says don't include a version line is so there's only one actual version for the module, and so no chance of difference.

Line 13 (and in the menu setup elsewhere) xmpp_chat_config should be xmpp_support_chat. Don't pollute namespaces.

xmpp_support_chat.install:

Please change line 13:
$library['library path'] = libraries_get_path('JAXL');
into:
$library_path = libraries_get_path('JAXL');
and adjust the rest to suit.

Line 18: 'JAXL library not install or installed wrong. ' .
should be:
'The JAXL library is not installed, or installed incorrectly. ' .
Line 19: This comment about README would be better replaced by making the README file available in the UI as is demonstrated in the hook_help() docs on d.o. (and then linking to it from this message).

I am constructing a module that requires an external server, and have put an entry in hook_requirements() for phase 'runtime' (only) that checks that the server exists and can be contacted and that the other config variables make sense. It would be good to do that here.

xmpp_support_chat.module:
Permission: it's good to have a permission setting, but I'm not sure this is the right granularity.

Line 14: 'Show region of chat'
should probably be:
'Show chat region'
Line 15: 'Wath kaind of groups will be see region of chat'
should probably be:
'Roles that are allowed to see the Jabber chat area.'
or similar.

Line 24: Elsewhere you've created empty arrays before using them, but here you don't.

Line 27: I haven't tried, but I suspect the download URL should actually be a URL starting with 'http'. Github do publish such URLs

Line 41 & 42: I think it would be better to swap the order of these tests - if you don't have permission to see things it's irrelevant whether the library exists, and a message about it not existing would be confusing, while if you do have permission, the library message is (somewhat) more understandable.

Line 73 and on: Please assign the module path to a variable and use it.

xmpp_support_chat_menu(): again, using another variant of the module name for the menu path. Keep to just one name.

xmpp-support-chat.js:

I'm no JS expert. This code looks ok but I don't have the experience to check. There are some long lines though so it would be good to cut them down. It would also be good to provide a known-good minified version.

I see you're using localStorage[] and wonder if the system gracefully degrades if the browser doesn't support or grant local storage access?

Please do check spelling: line 11 'stoped' -> 'stopped'. Line 27 & 67 'chating' -> 'chatting'.

Your project page is looking better now as is the code.

There are still spelling issues, eg: xmpp_support_chat_sentens -> xmpp_support_chat_sentence

I would encourage you to review the hook_requirements() function further as I suggested, especially the variable $library['library path'].

Has been marked Needs Review for 11 months.
Bumping to critical.

A lot of issues on PAReview, please check them out. I am not going to paste them here as it is quite lengthy.

@serg_admin - You can now promote this to a full project yourself.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.