This module allows users to insert videos from Aol into their posts, based on categories, search terms, keywords and others. It supports several WYSIWYG editors, as well as the default Drupal editor. Videos are loaded directly from Aol with the Aol video player, so users will only need an SID from Aol, and they can insert videos into the posts. There is no similar plugin, so this one is better than all because it exists.

Project page: http://drupal.org/sandbox/aolplugin/1918004
Git clone: git clone --recursive --branch 7.x-1.x http://git.drupal.org/sandbox/aolplugin/1918004.git aol_video_plugin
Drupal version: 7.x
Drupalcode tree: http://drupalcode.org/sandbox/aolplugin/1918004.git/tree

Manual Reviews:
http://drupal.org/node/1881052#comment-7079070
http://drupal.org/node/1904782#comment-7079006
http://drupal.org/node/1909588#comment-7079138

Round 2:
http://drupal.org/node/1784544#comment-7115178
http://drupal.org/node/1927760#comment-7114796
http://drupal.org/node/1913916#comment-7114956

Note: When testing, please set the filtering of the article to "Full HTML", as the widget adds an <img> tag, which ultimately gets converted into a <script> tag.

Comments

klausi’s picture

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

Welcome,

please get a review bonus first. Then try to fix issues raised by automated review tools: http://ventral.org/pareview/httpgitdrupalorgsandboxaolplugin1918004git

AolPlugin’s picture

AolPlugin commented
Status: Needs work » Needs review
Issue tags: +PAreview: review bonus

Hello,

Thank you for the suggestions. I've fixed the errors and warnings reported by the automated tool, and updated the issue with my reviews. Also updated the git clone command for anonymous use.

Adding the "PAReview: review bonus" tag.

AolPlugin’s picture

AolPlugin commented
Issue summary: View changes

added reviews

joseffriedrich’s picture

joseffriedrich commented

Hallo!

The code is very well formated.

My manual reviews:

  1. There is a empty aolplugin.pages.inc file in the git repository.
  2. Replace XXX with the module name in aolplugin.install file (line)
/**
 * @file
 * Install, update, and uninstall functions for the XXX module.
 */
AolPlugin’s picture

AolPlugin commented

Hello,

Thank you (danke) for the review. It seems these small things have slipped. They are now fixed and awaiting more reviews.

klausi’s picture

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

manual review:

  1. project page is too short, see http://drupal.org/node/997024
  2. Same for README.txt, see http://drupal.org/node/447604
  3. aolplugin_install(): no need to set variables on module installation as you can make use of default values with variable_get() anyway. So I would suggest to remove that function altogether.
  4. "'access arguments' => array('administer aolplugin settings'),": that permission does not exist? Did you forget to implement hook_permission()?
  5. "return t('<div id="fivemin-plugin">Please configure the plugin first</div>');": HTML tags should be outside of t() where possible.
  6. aolplugin_contents(): this should be a theme function so that themes can override the markup.
  7. aolplugin_node_view(): This is hard-coded to nodes and the body field, which is bad. I think you rather want to have a content filter, see hook_filter_info() and friends. That would then work for any text format that has the filter configured, regardless of field and entity.
  8. aolplugin_admin_validate(): use element_validate_integer_positive() as #element_validate in aolplugin_admin(), then you don't need this function.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

klausi’s picture

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

added testing clarification

AolPlugin’s picture

AolPlugin commented
Status: Needs work » Needs review

Hello klausi,

Thank you for the thorough review. We've addressed all items on your list and are awaiting for more feedback, if any (we hope there's none :) ).

Updated the issue with the new reviews done; the bonus tag was never removed so didn't change that.

Also, note on number 7 on your list, in addition to adding a content filter, we've also added a formatter that uses that filter. The code is taken from the Drupal core and adapted. Hope that's not an issue.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -PAreview: review bonus

manual review:

  1. aolplugin.js: you should use Drupal.behaviors instead of jQuery(document).ready(), see http://drupal.org/node/756722
  2. "define('AOLPLUGIN_DEFAULT_SID', 281);": what is this for? Please add a comment.
  3. aolplugin_preprocess_node(): shouldn't the CSS be added when the filter is processed?
  4. aolplugin_filter_info(): why do you have a list of allowed HTML tags? I thought you would only convert certain video tags with your filter?
  5. Please note: All user accounts are for individuals. Accounts created for more than one user will be blocked when discovered. Your account looks a bit suspicious, like a group account.

But that are not application blockers, so I guess this is RTBC. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

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

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Assigned: Unassigned » stborchert

Now actually assigning.

AolPlugin’s picture

AolPlugin commented

Hey,

Fixed 1 and 2. Regarding 3, I'm not sure what you mean.

Regarding 4, I wanted to replicate the default "Filtered HTML" filter as much as possible, since most people use this, so using the "Aol filter" shouldn't change this in any way for most users. Limiting all HTML to just an <img> tag would mean limiting the HTML for users who don't know how to edit the settings. Besides, there are settings, so users who know how, can add/remove HTML tags from the filter.

5. is a bit more sensitive and I don't want to get into detail publicly, but I can assure you that the account itself is used by one person only. I realize I was saying "we" in most comments, but I was referring to the multiple developers working on this module.

stborchert’s picture

stborchert
he/him
Primary language German
commented
Status: Reviewed & tested by the community » Fixed

Thanks for your contribution, Eduard!

Regarding the 3rd note in klausis last comment: your css is loaded every time a node is viewed. It would be much better if the css is loaded only if your filter applies to the node.

I updated your account to let you 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 get 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.

stborchert’s picture

stborchert
he/him
Primary language German
commented
Assigned: stborchert » Unassigned

* removed assignment

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

added more reviews