[D7] Ensemble Video

Hi,

first of all there are quite a few issues to sort out such as indentation, whitespace.

You can find them all here:
http://ventral.org/pareview/httpgitdrupalorgsandboxjmpease1834810git

Here you can check source code whether it meets drupal coding standards or not, and advise you what to change in your code. You can repeat review after your commits, and can fix those errors. And then change your status again.

Hi,

The code looks well written to me. One suggestion to consider is mentioning the use of dataTables jquery plugin on your project page. Sites might already be using the dataTables project. I don't know if version differences could cause conflicts or just that folks might want to be informed ahead of time what external plugins are being used. Perhaps simply adding this to your README file would suffice.

You may want to use drupal_json_decode instead of json_decode. It is Drupal's wrapper for json_decode in D7.

The only other minor issue which is no big deal that coder found is:
ensemble_video.proxy.inc

Line 30: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
$api_url = preg_replace('/http(s)?:\/\//', 'http$1://' . $username . ':' . $password . '@', $api_url);

Overall it looks very good.

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

Automated review:
PAreview.sh on ventral.org: http://ventral.org/pareview/httpgitdrupalorgsandboxjmpease1834810git gives multiple warnings/errors in multiple files ... in the 3rd party scripts
Coder module: Coder found 1 projects, 7 files, 3 normal warnings,... in the 3rd party scripts

Manual review:
It's only a tip (i read your notes in description): The js-files throwing these errors in the automated review scripts seem to be from a third party, should they directly included in the module package? Also the images like ensemblevideo-logo are included. Please be sure that the drupal rules for using thrid party codes is ok for your module in this case.
But therefore all looks very nice.

In a manual code-review i found no issues, but while testing i was not able to find any testing/demo "Ensemble Video Base URL" that worked for me.
Is there an "Ensemble Video Base URL" for testing the configuration? Please provide if there is one...

Added link to EV website for reference.

revert issue status.

Automatic Review

Quite a few errors found by ventral.org at http://ventral.org/pareview/httpgitdrupalorgsandboxjmpease1834810git (I take it these are from the 3rd party library)

Manual Review

• I normally put my CSS file in a folder labelled "css" but that's just the way I do it, like you have for "js"
• Should the "ensemble_video.js" file go in the "js" folder? Or are you keeping this for 3rd party JS files? Maybe there is a way to add these 3rd party files using the Libraries API module?
• You may want to add a hook_uninstall() function to your .install to clean up variables set e.g "ensemble_video_whitelist"

So no issues found for me other than maybe looking into including the 3rd party JS library using the Libraries API module.

ev-script.js: appears to be 3rd party code. 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms. The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.

Hi,

I only assumed it was 3rd party because one of the scripts was minified. If it is your own script then maybe don't minify it as Drupal will aggregate JS files anyway.

Cheers.

Again only a suggestion:
I think it's a really good idea to use the Libaries API to include these scripts, when the copyright notations must stay in there. I think when these scripts are also downloadable at github, it could be used well for other platforms to interact with ensemblevideo (if thats the idea of ensemblevideo then the Libaries API would be the solution to stay the "owner" these scripts). Perhaps you find a way to get an exception here (Infos about 3rd party libraries on Drupal.org) if the cases applies to you: http://drupal.org/node/422996 .

Added configuration instructions to description for testing.

Hi,

During manual test I came to the step when I should add a video through field to node (for example). I click sign "+", enter u:hasp p:hasp
and begin to watch "loading gif". After some time I receive an alert:
Could not find requested resource. This is likely a problem with the configured Ensemble Video base url.
I configured base url as: https://cloud-test.ensemblevideo.com
So I could not confirm module working as it should be.
ev-script.js third party file are still in the current branch.

Thank you.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

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

3rd party code was removed. You have a couple of errors reported here: http://pareview.sh/pareview/httpgitdrupalorgsandboxjmpease1834810git. You should remove the .gitignore file. It would still be nice if you would use the Libraries API to load the required javascript libraries. I don't think there are any remaining blocking issues though.

----
Top Shelf Modules - Crafted, Curated, Contributed.

I'll look at this now in the Project applications sprint

Sorry for the delay, but you have not listed any reviews of other project applications in your issue summary as strongly recommended in the application documentation.

manual review:

1. I think it would be wise to integrate with the media module: https://drupal.org/project/media
2. ensemble_video_validate_number(): use element_validate_integer as #element_validate property in the form instead, then you can remove this function.
3. ensemble_video_add_scripts(): I is recommended to use the #attached property in render arrays instead of calling the drupal_add_js/css functions.
4. ensemble_video_field_widget_form(): no need to call filter_xss() on #default_value in forms, that gets sanitized automatically. See https://drupal.org/node/28984
5. "variable_get('ensemble_video_whitelist', '');": all variables defined by your module need to be removed in hook_uninstall().

But that are not critical blockers, so ...

Thanks for your contribution, jmpease!

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.