[D7] Media Prezi

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://www.drupal.org/node/2288977

Project 2: https://www.drupal.org/node/2222137

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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

Here is my review of the code

Coding standards

A few minor issues:

• media_prezi.module: Remove the blank comment line in the @file block (line 5)
• includes/MediaPreziBrowser.inc: Refactor the comments in lines 18/19 such that the line length is not over 80 characters

Documentation

• README.txt is incomplete and needs to be substantially fleshed out.
• There is no link to the git repository in this request.

Licensing/Third Party

I was concerned at first about your inclusion of the prezi logo, but, from my reading of the Prezi terms of use I think this is OK.

dbcollies wrote:

I was concerned at first about your inclusion of the prezi logo, but, from my reading of the Prezi terms of use I think this is OK.

From the TOU:

(v) any website that uses Prezi Content must include prominent text to the effect that “this website is not affiliated with or sponsored or endorsed by Prezi.”

In what way is this term of the TOU fulfilled by the project?

@gisle You are correct. I didn't read through the entire text.

@blobsmith The only options you have, as I see it, would be to:

1. Find a way to add the text required by the TOU wherever the logo is used, or
2. Remove the logo completely, either replacing it with another image which you have the rights to, or use no image at all.

blobsmith said:

And so, what do you recommend?

Currently, I recommend nothing.

And I am not even sure if the license terms are only referring to the logo. Isn't the project designed specifically for using "Prezi content"?

The TOU speaks about "Prezi content", so I need to be able to install and start to use the module in order to see how "Prezi content" is used by the module. I haven't got that far yet. Both the project page and the README.txt is too sparse to get me started on a review.

My preliminary notes below:

Project page

The project page doesn't even explain what "Prezi" is, or how one can learn more about it. I hate having to google in order to review a module. It this case, it even looks like I have to sign up for some external service just to be able to review. Why is there no information about how to get started with "Prezi content" on the project page?

Please take a moment to make your project page follow tips for a great project page. In particular, you need to say what external service this module is for, how one can get access to Prezi content for test purposes.

It should also list up-front all non-core modules that the module depends on, and what versions to install when full releases do not exist. It currently says that it "Work only with Media version 7.x-2.x", but it doesn't say that there is no full release and that one must use the dev version. Ditto with the File Entity module.

README.txt

The REQUIREMENTS section in README.txt is wrong, see above. You also need to check the text for spelling errors. Also see guidelines for in-project documentation and the README.txt Template.

@blobsmith, it's a little bit complex module and need more time to review complete code of your module. But as I was reviewing your code found one issue in following code, there is no return type for the case if your 'if' condition fails but this hook functions must return an array.

/**
 * Implements hook_media_browser_plugin_view().
 */
function media_prezi_media_browser_plugin_view_alter($plugin_name, $params) {
  if ($plugin_name == 'remote_file') {
    if (remote_stream_wrapper_media_browser_plugin_access()) {
      $params += array('types' => array());
      $form = drupal_get_form('remote_stream_wrapper_file_add_form', $params);
      return array(
        '#title' => t('Remote URL'),
        'form' => array($form),
      );
    }
  }
}

@blobsmith, please manually review for this scenario when if condition fails and share with us what happened in that case.

Thanks for your contribution and patience!

@blobsmith coder sniffer is still showing some points which need to be fixed.
http://pareview.sh/pareview/httpgitdrupalorgsandboxogilbert2288159git

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  
  FILE: /var/www/drupal-7-pareview/pareview_temp/README.txt
  --------------------------------------------------------------------------------
  FOUND 1 ERROR AFFECTING 1 LINE
  --------------------------------------------------------------------------------
   56 | ERROR | Files must end in a single new line character
  --------------------------------------------------------------------------------
  
  
  FILE: /var/www/drupal-7-pareview/pareview_temp/media_prezi.module
  --------------------------------------------------------------------------------
  FOUND 1 ERROR AFFECTING 1 LINE
  --------------------------------------------------------------------------------
   98 | ERROR | Files must end in a single new line character
  --------------------------------------------------------------------------------
  
  Time: 212ms; Memory: 10Mb
  

  These two errors should not stop anyone from doing a real, manual review.

1) PAReview errors mentioned above still exist
2) Line 50 MediaPreziStreamWrapper.inc - don't mix single and double quotes
3) Functionally fine, works as expected.

I don't see any show stoppers - promoting to Reviewed and Tested.

Thanks you for your reviews. When finishing your review comment also set the issue status either to "needs work" (you found some problems with the project) or "reviewed & tested by the community" (you found no major flaws).

The project looks good to me after a manual review.

Thanks for your contribution, blobsmith!

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.

One more thing: The Git commits are not connected to your user account. You need to specify an email address. See https://www.drupal.org/node/1022156 and https://www.drupal.org/node/1051722