Link to sandbox page:

https://www.drupal.org/sandbox/mosewrite/2427607

Git clone command:

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/mosewrite/2427607.git

Initial Manual reviews of other projects

https://www.drupal.org/node/2443865#comment-9675165
https://www.drupal.org/node/2432439#comment-9662483
https://www.drupal.org/node/2328637#comment-9675241

Current Manual Reviews

https://www.drupal.org/node/2444455#comment-9699353
https://www.drupal.org/node/2448653#comment-9702811
https://www.drupal.org/node/2362563#comment-9709875

Description

Send to Kindle lets a site's users send Kindle-supported documents to their Kindle devices. This only works on field type 'file'. It creates a custom field formatter, which must be specified for the file field in question, when creating or editing the content type.

This module requires the mail system and mime mail modules:

After installation each user profile will have a new field called 'Send-to-Kindle E-Mail Address'. This is the email address of the Kindle device to which the user wants documents sent. The user *MUST* go into their Amazon account and configure their Kindle device to receive emails from 'kindlemail@domain_name', where 'domain_name' is the domain of the Drupal site. The Send-to-Kindle user profile field has a description that instructs the user to do this.

When the file is displayed, that display will be identical to the 'Generic file' formatter, except there will be a button directly after it that reads 'Send to Kindle'. The button only appears if the file is a supported file type for Kindle. Click the button, and the document is sent to the user's Kindle.

For customization purposes, the module includes a template file called 'send_to_kindle_kindlebutton.tpl.php'. Copy this file to your theme directory, and you can theme and modify the the file link as you wish. The template has a variable in it, $params, which contains all the file information. Modify it, and you can modify how the email is sent.

This module is similar to 'Amazon Send to Kindle'. It differs from that project in that 'Send File to Kindle' is restricted to (unsurprisingly) sending files and only sends files that are compatible with a Kindle. 'Amazon Send to Kindle' saves and sends all types of content to a given Kindle. 'Send File to Kindle' is much more single-purpose, much more focused.

One of the review commenters asked how this project differs from 'Readability Button'. It differs significantly because 'Readability Button' integrates a given page of content into the Readability service, which allows a page to be read offline or on another device, or both. That has little to do with sending a document file to a Kindle device for reading on that device.

Because the module operates by sending an email with an attachment to an email address for a given Kindle device, you may want to evaluate the module by specifying a different email address -- such as your own -- to which to send the email and attached file. By default the module does not allow a non-Kindle email address to be saved in the user profile. You can change this and allow the module to save a non-Kindle email address by going to the function below, which is in send_file_to_kindle.module and changing the line that reads '$is_good = FALSE'; to '$is_good = TRUE;'

function send_file_to_kindle_form_validate($form, &$form_state) {
  $submitted_email = $form_state['values']['send_file_to_kindle'][LANGUAGE_NONE][0]['value'];
  $is_good = TRUE;
  // Get the domain string and convert to lower case.
  $var = explode('@', $submitted_email);
  $domain = drupal_strtolower(array_pop($var));
  // If it's not empty and not a valid email or the domain isn't kindle.com,
  // fail.
  if ($submitted_email != '') {
    if (!valid_email_address($submitted_email) || ($domain != 'kindle.com')) {
      // Change the line below to TRUE to test with non-Kindle email addresses.
      $is_good = FALSE;
    }
  }
  if (!$is_good) {
    form_set_error('send_file_to_kindle', t('Your Kindle email address is invalid. It must be a valid email and be for the domain kindle.com. You do not have to provide an email address.'));
  }
}
CommentFileSizeAuthor
#24 coder-results.txt2.33 KBklausi

Comments

nabil.sadki’s picture

nabil.sadki commented

Automated review :

EDIT: remove long pareview.sh dump, see http://pareview.sh/pareview/httpgitdrupalorgsandboxmosewrite2427607git

PA robot’s picture

PA robot commented
Status: Needs review » Needs work

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

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

mlmoseley’s picture

mlmoseley commented

I have fixed every issue and the module now passes all code review completely. I will look at writing some unit tests for it, but that is a learning curve for me and will probably be a few weeks at least. In the meantime, please feel free to examine the module.

--Marshall

mlmoseley’s picture

mlmoseley commented
Issue summary: View changes
mlmoseley’s picture

mlmoseley commented
Issue tags: +PAreview: review bonus
mlmoseley’s picture

mlmoseley commented
Issue summary: View changes
mlmoseley’s picture

mlmoseley commented

This project is not showing up in the issues queue. Can anyone tell me why?

mlmoseley’s picture

mlmoseley commented
Status: Needs work » Needs review
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Issue summary: View changes
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Needs work
Issue tags: -PAreview: review bonus

Review of the 7.x-1.x branch (commit 65c30a5):

  • Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
    FILE: /home/klausi/pareview_temp/send_file_to_kindle.module
--------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------
  54 | ERROR | [x] Doc comment long description must end with a full stop
 207 | ERROR | [x] Doc comment long description must end with a full stop
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------

FILE: /home/klausi/pareview_temp/send_file_to_kindle.install
-------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------
 5 | ERROR | [x] Doc comment short description must be on the first line
-------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------
  • 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.

manual review:

  1. Project page is too short, how does it work? Only on file fields? See https://www.drupal.org/node/997024
  2. what are the differences to https://www.drupal.org/project/readability_button ? I assume your module works on file fields and that one on the content? Please add that to the project page.
  3. send_file_to_kindle_form_alter(): if you are targeting only one form you should use hook_form_FORM_ID_alter() instead.
  4. send_file_to_kindle_form_validate(): doc block is wrong, the first line only repeats the function name? See https://www.drupal.org/coding-standards/docs#forms
  5. send_file_to_kindle_supported_extension(): same here, the first line of a doc comment should be the short description. See https://www.drupal.org/coding-standards/docs#functions
  6. send_file_to_kindle_message_mail(): hook_message_mail() does not exist. Did you mean hook_mail()?
  7. "$message['params']['plaintext'] = 'Attached is your document';": all user facing text must run through t() for translation.
  8. send_file_to_kindle_button_form(): doc block is wrong, this is not hook_form()? See https://www.drupal.org/coding-standards/docs#forms
  9. " '#value' => 'Send to Kindle',": all user facing text must run through t() for translation.
  10. send_file_to_kindle_kindlebutton.tpl.php: ideally a template should not call such API functions, you should only be dealing with variables and if statements. You should prepare all necessary variables before calling theme() or do it in a preprocess function.
  11. send_file_to_kindle_button(): do not call drupal_render() here, rendering should be done as late as possible, could be done in the template with render().
  12. send_file_to_kindle_disable(): deleting data should only happen on uninstall hooks, otherwise you have data loss when the module is disabled.
  13. send_file_to_kindle_disable(): do not use "print" here, use drupal_set_message() with t() and appropriate placeholders.
  14. "t('Note: using your Amazon account, you must configure your Kindle device to accept email from&nbsp') . $from_email . t(' otherwise the document will not show up on your Kindle.')": do not concatenate variables into translatable strings like that, use placeholders with t() instead. See https://api.drupal.org/api/drupal/includes!bootstrap.inc/function/t/7

So the project page and the data loss issue are blockers right now. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

mlmoseley’s picture

mlmoseley commented

Thank. I'll get to work.

mlmoseley’s picture

mlmoseley commented
Issue summary: View changes
mlmoseley’s picture

mlmoseley commented

1. send_file_to_kindle_form_alter(): if you are targeting only one form you should use hook_form_FORM_ID_alter() instead.

Done.
2. send_file_to_kindle_form_validate(): doc block is wrong, the first line only repeats the function name? See https://www.drupal.org/coding-standards/docs#forms

Done.

3. send_file_to_kindle_supported_extension(): same here, the first line of a doc comment should be the short description. See https://www.drupal.org/coding-standards/docs#functions

Done.

4. send_file_to_kindle_message_mail(): hook_message_mail() does not exist. Did you mean hook_mail()?

Yes. Done.

5. "$message['params']['plaintext'] = 'Attached is your document';": all user facing text must run through t() for translation.

Done.

6. send_file_to_kindle_button_form(): doc block is wrong, this is not hook_form()? See https://www.drupal.org/coding-standards/docs#forms

Done. Changed to form constructor doc block.

7. " '#value' => 'Send to Kindle',": all user facing text must run through t() for translation.

Done.

8. send_file_to_kindle_kindlebutton.tpl.php: ideally a template should not call such API functions, you should only be dealing with variables and if statements. You should prepare all necessary variables before calling theme() or do it in a preprocess function.

Done. All templatized, and the themer can now modify the form array in the template to modify how the email is sent. I've also included the file object in case he/she wants to build their own link.

9. send_file_to_kindle_button(): do not call drupal_render() here, rendering should be done as late as possible, could be done in the template with render().

Done. Render is now in the template.

10. send_file_to_kindle_disable(): deleting data should only happen on uninstall hooks, otherwise you have data loss when the module is disabled.

This is a problem. When I simply disable the module using hook_disable, the user profile field remains visible in the user profile. Even if I was able to hide it on display, it remains visible and editable on the user profile edit form. There seems to be nothing I can do under hook_disable to hide the profile field but reptant he data (the email address). All the solutions I've discovered recommend using the field permissions module, but I think creating a dependency just so the module can be disabled is too much to ask, and inelegant.

On the other hand, if when the module is disabled and then re-enabled, all the user has to do is enter his/her Kindle email in the profile again...that's not a big burden. So unless someone can offer me a solution, I'm going to leave this as it is.

11. send_file_to_kindle_disable(): do not use "print" here, use drupal_set_message() with t() and appropriate placeholders.

Done.

12. "t('Note: using your Amazon account, you must configure your Kindle device to accept email from&nbsp') . $from_email . t(' otherwise the document will not show up on your Kindle.')": do not concatenate variables into translatable strings like that, use placeholders with t() instead. See https://api.drupal.org/api/drupal/includes!bootstrap.inc/function/t/7

Done

mlmoseley’s picture

mlmoseley commented
Status: Needs work » Needs review

Addressed all of the most recent issues.

mlmoseley’s picture

mlmoseley commented
Issue summary: View changes
mlmoseley’s picture

mlmoseley commented
Issue summary: View changes
davidsheart02’s picture

davidsheart02 commented

Don't include the LICENSE.txt file: https://www.drupal.org/node/1587704

Your git clone command is missing the renaming of your project and could lead to a bad test. May want to update that.

mlmoseley’s picture

mlmoseley commented
Issue summary: View changes
mlmoseley’s picture

mlmoseley commented
Issue summary: View changes
mlmoseley’s picture

mlmoseley commented
Issue summary: View changes
mlmoseley’s picture

mlmoseley commented

I@davidsheart02. have removed the LICENSE.txt file. I don't understand this comment: "Your git clone command is missing the renaming of your project and could lead to a bad test. May want to update that." To what renaming are you referring?

mlmoseley’s picture

mlmoseley commented
Issue tags: +PAreview: review bonus

I have completed three more reviews and added back in the 'PAReview: review' bonus tag.

mlmoseley’s picture

mlmoseley commented
Issue summary: View changes
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Assigned: Unassigned » cweagans
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new coder-results.txt2.33 KB

Review of the 7.x-1.x branch (commit e74bc25):

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.

manual review:

But otherwise looks good to me.

Assigning to cweagans as he might have time to take a final look at this.

mlmoseley’s picture

mlmoseley commented

I have fixed the issues in issue 24, and the project now passes Code Sniffer and phpcs with flying colors. How do I take this to RTBC?

cweagans’s picture

cweagans
He/Him
Primary language English
Location Boise, ID, USA
commented

Thanks for your contribution, mlmoseley!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a ""full"" project.

Here are some recommended readings to help with excellent maintainership:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Reviewed & tested by the community » Fixed

Marking as fixed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.