[D7] CloudSMS

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxsignalfire2101413git

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.

Hi,

I manually reviewed your code and tested your module. The following are few items that needs to be taken care.

1) Please use the Drupal API t() function for all the user visible texts. lots of titles and descriptions from cloudsms_admin_menu() and many files in the inc folder are missing the t().

2) I am getting the following notice and warning when I try to click the Import Subscribers.

Notice: Undefined index: cloudsms_admin_subscriber_import_form in drupal_retrieve_form() (line 763 of /Applications/MAMP/htdocs/theme/includes/form.inc).
Warning: call_user_func_array() expects parameter 1 to be a valid callback, function 'cloudsms_admin_subscriber_import_form' not found or invalid function name in drupal_retrieve_form() (line 798 of /Applications/MAMP/htdocs/theme/includes/form.inc).

Thanks,
Ramesh

Hi,

I think the t() is not necessary for the cloudsms_admin_menu(), because Drupal will automatically take care of this. So please take care of the other places.

Thanks,
Ramesh

Hi,
manual review of the module.

It looks like the main module is not doing anything (except adding a help) but there are 2 optional modules that the user can enable. the API module is required by the admin module, so ultimately to get this working, user has to enable the main module and the API module. I'd suggest to use the API module as the base module and make the admin module optional.

cloudsms.install
Generally we delete the variables created by the module in hook_uninstall step (<D8). When the user disabled a module, that means data is not removed by hooks are not getting executed so the module functionality is off. If the user wants to completely remove the module, he'd use the uninstall tab to do so.

Line 58 of cloudsms_api.module.
The comment says array chunks have 500 each, but it's 50 in the code.

Update:
Imagine someone going through your code, in a train, with a tablet, and surrounded by crying babies (that's me). I'll write a proper one later :)

I installed and tested it and everything looks good to me. Sent out my SMS perfectly.

First of all thanks for effort to promote this module in drupal.org. I hope this will help others. I just did a quick review and here are my findings:

1. Still there are very few coding standard issues here http://pareview.sh/pareview/httpgitdrupalorgsandboxsignalfire2101413git
2. Please update non-maintainer git clone URL at summary.
3.a. I don't find any standard around the file & dir name of include files, but in core, commonly we can see 'includes' as a directory name (if there are more than 1 files to include) and the filename stays same as module name if it is single. So your case, instead of inc/functions.inc, we can keep it in cloudsms.inc in the module root. or includes/cloudsms.inc, if you like to keep it under includes folder.
3.b -

module_load_include('inc', 'cloudsms', 'inc/functions');

(Different places of module) is not a recommended practice. It's going to be loaded on all request(when this module enabled). Is there a way to move it under any function level where we really need? or if it is just two helper function we can move them to .module and remove the .inc?
4. check_markup(file_get_contents(dirname(__FILE__) . "/README.txt")); on hook_help is not recommended. Can we have HTML help description please?
5. Seems cloudsms_api_key is a very important variable that we need to have to send mails on cloudsms module. however the admin interface to enter this key is available only on cloudsms_admin module
5.1. Can we mention that in README or INSTALL that this is something we need to set in settings.php or somewhere, if we don't want cloudsms_admin module?
5.2 If the setting this key is part of config file, is it really worth throwing error on hook_enable of cloudsms_admin? I would try to throw this error on every admin pages where we try to CURD messages and cloudsms_admin settings page.

From my understanding we are trying to CURD some details on our own schema and have a list/edit/add/delete/export/import options. we can do this with few lines of code + Ctoll export plugin. Here are some resource, if you want to give a try:
https://drupal.org/node/928026
http://www.sthlmconnection.se/en/blog/exportable-configuration-ctools-re...

Closing due to lack of activity. Feel free to reopen if you are still working on this application (see also the project application workflow).

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