This project installs the Formilla Live Chat script to a Drupal 7 site. Simply install this module, sign up for a Formilla.com account, enter your chat ID, and you can enable the script in your footer to be up and live in minutes. Formilla offers a free standard live chat package for small customers, along with very affordable premium services for larger customers.
We strive to keep our installation as simple as possible to get up and running quickly. The features of our product can be found on the project sandbox page at:
Project Page:
https://www.drupal.org/sandbox/formilla/2294765
git clone command:
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/formilla/2294765.git formilla_live_chat
cd formilla_live_chat
Comments
Comment #1
ethantLot to do here before a manual review can be performed effectively.
Please see here:
http://pareview.sh/pareview/httpgitdrupalorgsandboxformilla2294765git
Comment #2
labboy0276 commentedAutomated Review
Your code needs some work as pointed out here:
http://pareview.sh/pareview/httpgitdrupalorgsandboxformilla2294765
Manual Review
Need to use check_plain in your textarea deifned in .admin: $form['general']['formillachat_id']
Minor finding: try using module_load_include instead of :require_once drupal_get_path('module', 'formilla_live_chat') . '/formillalivechat.php'; in your .module
*Major finding: You have your class file named .php. API files should be named filename.api.php,
If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.
This review uses the Project Application Review Template.
Comment #3
zgilyana commentedComment #4
zgilyana commentedGentlemen, thank you both for your feedback. All major findings and errors are complete based on your automated and manual reviews. Converted to branch instead of master as well.
Please let me know your thoughts, thanks again!
Comment #5
zgilyana commentedComment #6
PA robot commentedWe 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.
Comment #7
jepster_Automated Review
[Best practice issues identified by pareview.sh] There's still work outstanding: http://pareview.sh/pareview/httpgitdrupalorgsandboxformilla2294765git
Manual Review
The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.
This review uses the Project Application Review Template.
Comment #8
zgilyana commentedjepSter -- Thanks so much for the review, good recommendations. Here is a summary of the fixes prefixed with "Formilla Response:". Happy Thanksgiving! :-)
Automated Review
[Best practice issues identified by pareview.sh] There's still work outstanding: http://pareview.sh/pareview/httpgitdrupalorgsandboxformilla2294765git
Formilla Response: These have been completed.
Manual Review
No duplication
[No: Causes] module duplication and/or fragmentation. There are many other chat modules which are offering the same. Quite every chat module has some "free"-option.
Formilla Response: This is the only module in the repository that installs the Formilla.com live chat software script (cloud-based solution).
README.txt/README.md
[No: Does not follow] the guidelines for in-project documentation and/or the README Template.
Formilla Response: This has been completed per the template recommendations.
Coding style & Drupal API usage
(*) formillalivechat.api.php: getChatCode(); - here you're bypassing the way how Drupal adds JavaScript. Please learn on this page and add your js into a own js-file: https://api.drupal.org/api/drupal/includes%21common.inc/function/drupal_...
Formilla Response: Good suggestion. This has been completed.
Please add your HTML markup into seperate html files and use Drupal's theme api, so that the user can theme your html markup.
Formilla Response: The reason for this is that all theming and customization of the live chat button and form is managed via a user's Formilla.com account without modifying any code after installing the module.
Comment #9
zgilyana commentedHi guys - just checking in to see if we're all set based on the last round of changes. Hope all is well, happy holidays :)
Zaia
Formilla.com
Comment #10
k_zoltan commentedThanks for your contribution.
All user accounts are for individuals. Accounts created for more than one user or those using anonymous mail services will be blocked when discovered (see Get a Drupal.org account).
Please note that organization accounts cannot be approved for git commit access. See https://drupal.org/node/1966218 and https://drupal.org/node/1863498 for details on what is/isn't allowed. Please update your user profile so that we don't have to assume that this is a group account.
Comment #11
zgilyana commentedHi k_zoltan,
Good point! Thanks for bringing that to my attention. I've modified the account name to "zgilyana" which is my personal userID and I've also added my own email address to the account as well.
Thanks for your review!
Zaia
Comment #12
k_zoltan commentedGood work Zaia, but in order to get through the review process we should put this ticket to "Needs review"
Comment #13
d2ev commentedClean your repository, it has more than one info file.
Install file : Didn't get the intent of writting hook_install here http://cgit.drupalcode.org/sandbox-formilla-2294765/tree/formillalivecha...
formillalivechat.module : Remove the blank functions if you are not using them.
formillalivechat_chatscript.js : Store our function as a property of Drupal.behaviors. https://www.drupal.org/node/171213
formillalivechat.api.php: I hope you should optimize logic for $formillalivechat_id
Comment #14
zgilyana commentedGreat feedback, thank you. Updates posted and responses below:
Clean your repository, it has more than one info file.
----> ZG: removed extra info files
Install file : Didn't get the intent of writting hook_install here http://cgit.drupalcode.org/sandbox-formilla-2294765/tree/formillalivecha...
---> ZG: oops; agreed unnecessary. Removed.
formillalivechat.module : Remove the blank functions if you are not using them.
---> ZG: Removed blank functions.
formillalivechat_chatscript.js : Store our function as a property of Drupal.behaviors. https://www.drupal.org/node/171213
---> ZG: Completed.
formillalivechat.api.php: I hope you should optimize logic for $formillalivechat_id
---> ZG: Good catch. Rewrote logic to eliminate multiple calls and removed need for api file as a result.
Comment #15
harings_rob commentedFirst of all. Your clone link is invalid, should be:
The following is not a release blocker, but its always nice to have a clean automated review:
http://pareview.sh/pareview/httpgitdrupalorgsandboxformilla2294765git
Manual review:
1.
In your formillalivechat.admin.inc file, change the #required to FALSE instead of 0 for readability.
2.
In your installation file, you use the wrong function comments.
hook_requirements and hook_install are missing.
See: https://www.drupal.org/node/1354
3.
Same for your .module file, please use the correct documentation standards.
https://www.drupal.org/node/1354
4.
It's not a requirement, but to make your code easyer to maintain, I would place your js file in a js directory.
Comment #16
zgilyana commentedHi harings_rob, others,
Thank you for your feedback. Think it's possible to take this to the finish line at this point given it has been a year in the cycle? The recursive cycle between reviews, re-test, and re-review has taken some time, and I think we're at a point it's minor stuff better to be grouped with other changes in the future as opposed to keeping this queued up for much longer. Heck if I was really good with PHP I'd contribute some reviews myself to speed things up, although I'd likely do more damage than good! I did just update the clone link per your comments, thx for catching that!
Please let me know your thoughts.
Cheers,
Zaia
Comment #17
zgilyana commentedComment #18
fabian.fernandes_30 commentedhi zgilyana,
i would like to say good work,
i seems to work perfectly functionally.
there are some issue still pending in
http://pareview.sh/pareview/httpgitdrupalorgsandboxformilla2294765git
Submitted by Anonymous (not verified) on Sat, 07/26/2014 - 21:41
Review of the 7.x-1.x branch (commit eade26a):
http://pareview.sh/pareview/httpgitdrupalorgsandboxformilla2294765git
Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
ESLint has found some issues with your code (please check the JavaScript coding standards).
formillalivechat_chatscript.js: line 6, col 2, Error - Use the function form of "use strict". (strict)
formillalivechat_chatscript.js: line 11, col 47, Error - Expected '===' and instead saw '=='. (eqeqeq)
formillalivechat_chatscript.js: line 16, col 65, Error - Expected '===' and instead saw '=='. (eqeqeq)
formillalivechat_chatscript.js: line 16, col 96, Error - Expected '===' and instead saw '=='. (eqeqeq)
formillalivechat_chatscript.js: line 17, col 15, Error - Expected indentation of 12 characters but found 14. (indent)
formillalivechat_chatscript.js: line 18, col 15, Error - Expected indentation of 12 characters but found 14. (indent)
formillalivechat_chatscript.js: line 18, col 15, Error - "Formilla" is not defined. (no-undef)
formillalivechat_chatscript.js: line 19, col 17, Error - Expected indentation of 12 characters but found 16. (indent)
formillalivechat_chatscript.js: line 19, col 17, Error - "Formilla" is not defined. (no-undef)
formillalivechat_chatscript.js: line 20, col 16, Error - Expected indentation of 10 characters but found 14. (indent)
formillalivechat_chatscript.js: line 21, col 12, Error - Expected indentation of 8 characters but found 10. (indent)
formillalivechat_chatscript.js: line 22, col 8, Error - Expected indentation of 8 characters but found 7. (indent)
formillalivechat_chatscript.js: line 23, col 6, Error - Expected indentation of 6 characters but found 4. (indent)
13 problems
No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.
This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.
FILE: /var/www/drupal-7-pareview/pareview_temp/formillalivechat.module
---------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------
48 | ERROR | [x] Closing parenthesis of array declaration must be on a
| | new line
---------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------
FILE: /var/www/drupal-7-pareview/pareview_temp/formillalivechat.admin.inc
---------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------
27 | ERROR | [x] Closing parenthesis of array declaration must be on a
| | new line
---------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------
Time: 83ms; Memory: 5.25Mb
Source: http://pareview.sh/ - PAReview.sh online service
Comment #19
avpadernoI am changing status basing on the previous comment.
Comment #20
PA robot commentedClosing 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.