[D7] YouTube Video Transcript

Multiple Applications

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

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

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

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.

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.

Hi manimjs_drupal,

Thanks for your contribution.

I have found these issues in your module.

1. The @file block is missing in the file controle.js. Please add it.
2. The downloaded module shows the name '2401575' instead of Transcript.

Hi manimjs_drupal,
I appreciate effort you have take to create this module, i have gone through your module file please just have a look at below tips to improve this module.

1) This is related to youtube-video-transcript.tpl.php file, please add below code on the top of the youtube-video-transcript.tpl.php file

/**
 * @file
 * Default theme implementation to present .
 */
 

2) This is regarding to name of the function prepare_transcript(). whenever we create a custom function in drupal
we have to write a function name in this format nameofmodule_functionname() {}. please follow these practice.

3) This is regarding your controle.js file
this is good practice to write your javascript code insight jQuery(document).ready(function($) {here is your javascript code.});
writing javascript code insight this function means the js should load after the DOM has been ready...

4)Again this is related to youtube-video-transcript.tpl.php file, here i would like to share very small but good enough thing to use in our coding practice.
a) id="my_youtube_video"
b) class="video_transcript"
Try to avoid "_" in class name or in id, instead use "-".

Thanks,
Rahul.

my given issues are fixed by manimjs_drupal,

Thanks,
Rahul.

Hi manimjs_drupal,

I went through the module you have contributed, this is highly useful for Developers. Appreciate your effort. Keep up the good work and improve the features bit more.

Thanks
M.Dhanasekaran

Hi manimjs_drupal,
There some issues to be handle please follow this link:
http://pareview.sh/pareview/httpgitdrupalorgsandboxmanimjsdrupal2401575g...

Thanks,
Rahul

From the standard checklist:

1. Basic application checks
1.1 Ensure your application contains a repository and project page link. [Not OK: pareview link is broken.]
Use: http://pareview.sh/pareview/httpgitdrupalorgsandboxmanimjsdrupal2401575g...

1.2 Ensure your project is not a duplication. [OK]

2. Basic repository checks
2.1 Ensure the repository actually contains code. [OK]

3. Security Review
3.1 Ensure the project does not contain any security issues. []

4. Licensing checks
4.1 Ensure the repository does not contain a ‘LICENSE.txt’ file. [OK]

4.2 Ensure the repository does not contain any 3rd party (non-GPL) code. [OK]

5. Documentation checks
5.1 Ensure the project page contains detailed information. [Not OK: present but insufficiently detailed.]

5.2 Ensure the repository contains a detailed README.txt. [OK]

5.3 Ensure the code contains a well-balanced amount of inline-comments. [OK]

6. Coding standards and style
6.1 Run an automated review and ensure there are no major issues. [OK -- but]
[Some issues found: 3 warnings about 'Implements']
[Would suggest also adding some unit tests.]
[There are several issues with the coding style: See separate post.]

7. API and best practices Review
7.1 Ensure you are using Drupals API correctly. [API use looks reasonable]

I mentioned in the checklist there were issues with the code. These are:

- The javascript file 'controle.js:

+ is written with the functions and variables at the global level, and so 'pollutes' the namespace. I am not a JS programmer but the standard pattern is use the "Drupal.behaviors" JS global with all code wrapped inside it. This also enables Drupal to correctly manage ajax tabs and similar. Even if not using Drupal.behaviors, it would be best to hide all but the external call point.

+ With the JS code using jQuery or 'Zepto' (haven't seen that) it would be helpful to mention this in the project page and indicate if there are version restrictions (e.g. jQ 1.8, 1.9? ...). Especially indicate which jQ version(s) you have tested the JS with.

+ There is a typo: the English word 'start' has no 'e' on the end (starteTranscript), and I would like to see the two numbers (100, 4000) documented and preferably pulled out as named constants.

+ Why is line 27 event.target.playVideo() commented out? Could it be deleted and if so could the whole function be deleted? If not, please include a comment as to why line 27 might be needed in the future.

- The CSS file transcript.css:

+ Looks fine, except that the CSS class names are too generic, especially 'tracks'.

- The file transcript.info:

+ The 'name' here could be improved by prefixing with "YouTube": "YouTube Video Transcript"

- The PHP file transcript.module

+ The configuration menu is put in admin/config, while it might be preferable to put it in admin/config/media to cooperate with other similar modules.

+ There is some sort of key or ID in the source: kswE_n0FNXk which I feel is inappropriate. If a default value is needed, include it as a named constant (define(...)) and indicate in the source what it refers to. Preferably however leave the default blank and also handle appropriately the case where there are invalid inputs (e.g. check that iframe_width/height/video_id are not what you would expect.

+ The function comment for transcript_get_tell_friend is not useful. Please rewrite the comment indicating why the function exists / what overall task it performs.

+ In transcript_get_tell_friend there are two calls to drupal_get_path. Better to assign the result to a local variable and use that. Doing so will also reduce the overlong line length.

+ The transcript_get_tell_friend call includes an error message that is not good English and also not wrapped in t() so is untranslateable. Finally, it would be beneficial to include a or

tag around the text to enable this message to be targetted by CSS.

+ transcript_fetch forms a URL using the php concat (.) operator without checking that the ID or lang variables are the sort of variables expected. It wouldn't hurt to call 'check_plain()' more here.

+ A call is made to 'simplexml_load_string()' -- it would be sensible to record that the simplexml extension to php is required.

+ Just wondering: is it necessary to use 'curl' or would drupal_http_request() be usable? drupal_http_request() is more 'portable' in the sense there is an established way to override it, and it is expected in Drupal php code that this is the way to do http requests. What benefit is use of curl providing here?

+ Line 128 has the expression: "$sec % 3600 / 60", which may not have the result you expect: I suggest you put brackets in to ensure the order of operation and make it clear, and if you actually mean "$sec % (3600 / 60)" then why not write "$sec % / 60" which is the same.

+ Extension opportunity: include a hook_requirements() check that the module is configured properly.

This looks like a great module and great idea. Thanks for your contribution.

Hi manimjs_drupal,

Great that you found my review useful and thanks for implementing so much of the work.

I see you've updated this issue description in useful ways but the project (sandbox) page is still quite plain. Check out other modules and/or various blog posts as to what is helpful on the project page. For example: https://www.drupal.org/node/2190239, https://www.drupal.org/node/997024

I am also surprised that jquery_update is required; if that is because you need JQ 1.5+ and the default for base D7 is 1.4, then I'd just indicate that jQuery 1.5 or later is needed. jquery_update module is just a way to get that update IIRC.

Ruth

Hi again. Many thanks for all the work you've put in. I am setting this to RTBC now, though there are two things that could still be improved before you issue a stable release:

- Please add a similar modules section to the project page with links to the actual projects. Even if the modules are indeed different, someone finding your module and deciding it isn't quite what they are after can use the list for additional options, which helps us all.

- Several of the functions in the .module file has comments like /* Implementation of admin configuration */. The formats Drupal prefers are:

/* Implementation of hook_help(). */

and a short sentence describing why the function exists, for example (but not necessarily):

/* The administration menu configuration form callback function. */

Personally, I feel the current 'Implementation of admin configuration' sort-of-suggests it's a hook when it isn't, which is bad.

This point could be considered a personal one and I suggest you seek other views if you disagree.

@ramkrk could explain why it's back to needs review?

Switching back to original status.

Thanks for your contribution!

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.