[D7] - Flipping Book

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

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.

You should display the max upload size for the drupal installation on the flipping book upload form. Otherwise if a user tries to upload a flipping book larger than their post_max_size setting, the form fails silently. And it seems most of these flip books are pretty large?

Or configure a warning message if the flip book exceeds the Drupal sites max file upload size.

Second, could you provide an example of a flipping book zip file to test with? I'd rather not sign up for the flipping book site just to test this module... that could be why nobody has reviewed yet.

Other than that, the actual code looks pretty good.

Removing review bonus tag, you have not done any review of other project applications as requested on the review bonus page? It is nice that you contribute patches to other modules, but this program is about making progress with project applications. Feel free to review some and then add the review bonus again!

Your missing the git clone instructions on the project application.

description = Flipping Book module integration with Drupal. 
package = Bmeme

Description should be a sentence in the present tense; also don't use 'Drupal' in the UI. Eg, 'Provides integration with yada yada'.
Having a package named after yourself seems weird.

dependencies[] = transliteration

That seems a bit of an unrelated dependency.

function flipping_book_schema() {
  $schema['flipping_book'] = array(
    'description' => 'Flipping Book files.',

Would it not be possible to use the regular files table for these, specifying the type?

    'administer flipping_book' => array(
      'title' => t('Administer Flipping Book'),
    ),
    'manage flipping_book' => array(
      'title' => t('Manage Flipping Book'),

Administer and Manage are pretty much synonyms! How on earth is the poor admin user to know which permission means what?

/**
 * Implements hook_views_data().
 */
function flipping_book_views_data() {

This hook should go in a views.inc file to improve performance.

/**
 * Executes node deletion.
 *
 * @see node_delete_confirm()
 */
function flipping_book_delete_confirm_submit($form, &$form_state) {


/**
 * @file
 * Contains the basic 'node' field handler.
 */

/**
 * Field handler to provide simple renderer that allows linking to a node.


/**
  * Provide link to node option.

Not sure those docs are correct... ;)

Overall there seems to be good attention to detail, though I do wonder whether the whole project could be made much simpler if it used Drupal core's managed file table & possibly also file reference field.

> But we maintained two different permissions to logically separate administration of the module, which is usually performed by a site-admin, and the action to add one or more flipping book file(s), which is usually provided by a site-editor.

That makes sense. It's just the naming of the permissions that was confusing. Though:

    'upload flipping_book' => array(
      'title' => t('Upload Flipping Book'),
    ),

That should be plural, surely?

> Leaved transliteration as dependency. Tranliteration is very useful to sanitize uploaded filename and we need all filenames do not show spaces or special characters, otherwise the module will not work as expected.

It's useful, but is it essential? If I don't already have Transliteration module on my site, I don't want to be forced to install it just for this.

> We obviously could "copy&paste" this method in our code, but I think this is absolutely NOT the best way

Agreed! However, you could consider having a so-called 'soft dependency'. That is, use module_exists() and call functions from Transliteration module if the module exists.

Automated Review

Review of the 7.x-1.x branch:

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• DrupalPractice has found some issues with your code, but could be false positives, and may be duplicate results from Coder Sniffer. See attachment.

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.

Look these through. I think some are false positives, eg the views class names. Nothing looked too problematic, though.

Manual Review

Update the README to say transliteration is optional.

Why is the dir part of the PK on the schema? The unique key is also redundant as PK needs to be unique by definition. Also don't
see why the dir needs to be a key, is it used as a query condition?

define('FLIPPING_BOOK_REFERENCE_COLORBOX', module_exists('colorbox') && variable_get('colorbox_load', FALSE));

I don't know if either module_exists() or variable_get() is totally safe to use at the top level in all circumstances. I would turn this into
a proper variable via an admin form.

In the formatter view for 'flipping_book_reference_colorbox', pass the URL options via options instead of building yourself.

In _flipping_book_reference_options, is the html_entity_decode() to get around #1919338: Select widget (from the options module) prone to double encoding ?

Does flipping_book_reference_autocomplete() needs a drupal_exit()?

flipping_book_delete_confirm_submit() has a watchdog w/o an explicit level. I would set one.

flipping_book_management_form_submit() can use REQEUST_TIME instead of time().

User #attached instead of drupal_add_css() in flipping_book_management_form

flipping_book_delete() is missing the @return from the docblock

This is a very well written module with good use of comments that explain *why* something is being done and not just *what* is being done.
I also explicltly tested XSS in the titles and didn't get any alerts in the node view and the default view that is provided.

I see nothing to prevent RTBC here. As the module is fairly big, I am asking @heddn for a second look if he has time.

OK, now I can assign this to @heddn to take a look at this if he has time.

Automated Review

Location      file [flipping_book] - .../views/views_handler_field_flipping_book_link.inc
Problem synopsis      Unused parameter $values (at line 54)

Manual review

• Might want to use file_usage_add() when saving the contents of the zip file.
• No removal of the created directory on hook_uninstall.
  

  function flipping_book_install() {
    $directory = FLIPPING_BOOK_DIR;
    file_prepare_directory($directory, FILE_CREATE_DIRECTORY);
  }

• No mechanism to put the books into a private filesystem. Feature request might be to provide that as a configurable option.
• This returns all columns from the database although not all colums are used.
  

  flipping_book_load($fbid) {
    $flipping_books = &drupal_static(__FUNCTION__);
  
    if (!isset($flipping_books[$fbid])) {
      $flipping_book = db_select('flipping_book', 'f')
        ->fields('f')
        ->condition('f.fbid', $fbid, '=')
        ->execute()
        ->fetchObject();

• (*) The exported view uses a non-English language.
  

    $handler->display->display_options['exposed_form']['options']['reset_button_label'] = 'Ripristina';
    $handler->display->display_options['pager']['type'] = 'full';
    $handler->display->display_options['pager']['options']['items_per_page'] = '20';
    $handler->display->display_options['pager']['options']['offset'] = '0';
    $handler->display->display_options['pager']['options']['id'] = '0';
    $handler->display->display_options['pager']['options']['quantity'] = '9';
    $handler->display->display_options['pager']['options']['tags']['first'] = '« prima';
    $handler->display->display_options['pager']['options']['tags']['previous'] = '‹ precedente';
    $handler->display->display_options['pager']['options']['tags']['next'] = 'seguente ›';
    $handler->display->display_options['pager']['options']['tags']['last'] = 'ultima »';

• Many projects place sub-modules into a 'modules' folder then index the sub-modules by the sub-module name. eg. ./modules/flipping_book_reference/flipping_book_reference.info
• Should use decode_entities() rather than html_entity_decode() in _flipping_book_reference_options().

The starred items (+) are fairly big issues. The rest of the comments in the code walkthrough are recommendations.

None of these are release blockers. Thanks for your contribution, bmeme!

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:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

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.