Taxonomy Tools

This is the result from the automated testing tool - http://ventral.org/pareview/httpgitdrupalorgsandboxmartinsbertins1406398git. You can use this link for future reviews too.

Hello, I am a fellow applicant doing a review of your project application.

Automated Review

http://ventral.org/pareview/httpgitdrupalorgsandboxmartinsbertins1406398git

Came back clean.

Manual Review

I cloned the D7 branch into a fresh Drupal 7 localhost install and used drush to enable taxonomy_tools and taxonomy_tools_role_access. I used devel generate to make a bunch of dummy terms in the tags vocabulary. I then configured Taxonomy Role Access to be setup on the Tags vocabulary and control the anonymous and authenticated user roles. At this point it took me a while to catch on that I needed to go to the 'Overview' tab on the Tags vocabulary edit form. I kept looking inside the edit form for individual terms/tags (this is where I expected the controls to be).

From here I ran into a problem. When I clicked on the enable/disable ajax controls the images wouldn't render properly. My localhost install is running at http://localhost/drupal-7.12 and all of the icon images on the 'Overview' tab are looking for their image source in http://localhost. I took a look inside your code and I see a few spots where you are building a path to an image by using '/' instead of base_path(), please call the base_path() so the image source will render properly no matter where the drupal install lives. (line 120 in taxonomy_tools_role_access.module, 92, 104, 117 and 171 in taxonomy_tools.admin.inc should use base_path() instead of '/').

After making those changes in the code, I was able to get the enable/disable ajax buttons/icons to work. However, the anonymous user column gets stuck on disabled and I am not able to get the enabled icon back.

At this point I was able to test 'access for authenitcated user' on a few terms (tags). Visiting one of those term's pages directly as an authenticated user, it responded as expected when the permission was enabled/disabled. The access denied page appeared when it was supposed to, and the ability to view the term page worked when it was needed. I didn't test the anonymous user permissions because the enable/disable is broken for that role. I did however watch the 'Network' tab in Firebug to see if I could watch the ajax call/response and see if anything was going on. I couldn't notice anything unusual as to why the 'access for anonymous user' ajax enable/disable buttons aren't working.

Next I tried disabling access to a particular term for authenticated users (like before), but this time I removed 'authenticated user' from the 'select user roles which will be controlled by Taxonomy Role Access'. When visiting the term page, it behaved as expected and the authenticated user was able to view the page.

To be able to test any further it seems necessary that the 'access for anonymous user permission' column bug would need be fixed up. This module is quite far a long for a sandbox project, keep up the good work!

- In hook_menu, wrap your descriptions in t()
- I think it's recommended that you create your own permission instead of using 'access administration pages'
- PAreview has more errors - http://ventral.org/pareview/httpgitdrupalorgsandboxmartinsbertins1406398git

Here's a quick review regarding functionality:
1. If module is setup after terms have been created, if shows that all terms are published, when in fact thei are not.
2. If Taxonomy Tools module alone is enabled, it could provide it's features (reordering, deleting) for all Vocabularies in Overview tab. Also there should a short description in readme about these features.

1. Code Review reports

   taxonomy_tools.admin.inc

   Line 295: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   $link = 'taxonomy-role-access/nojs/' . $vocabulary->machine_name . '/' . $term->tid . '/' . $rid . '/' . $opposite;

   and

   taxonomy_tools_role_access.module

   Line 158: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   $link = 'taxonomy-role-access/nojs/' . $vocabulary->machine_name . '/' . $tid . '/' . $rid . '/' . $opposite;
   Line 191: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   $link = 'taxonomy-role-access/nojs/' . $vocabulary->machine_name . '/' . $tid . '/' . $derived->rid . '/' . $opposite;

2. Should the number of redirect links be limited to 1? (lines 86-95 of taxonomy_tools_redirect.admin.inc)

3. This taxonomy term is set to redirect to: taxonomy/term/294
   Updated term 1100-1500.
   This taxonomy term is set to redirect to: localhost

   Seems to be a leftover message? Only one redirect can be true?

4. One term can redirect to a term which can redirect itself back i.e. a circular loop. I'd happily say this is "user error", but was wondering if this could be trapped?

I'm not a great coder, just trying to help.
Hope this helps, I really like this module. We have a site full of terms and a tool to redirect typos would be really handy.

reviewing....

Autmatic review:

see attached file bellow

Manual Review:

1. I like multi delete option, will be definitely appreciated by anybody who need to delete more than couple of terms at once.

There is a small usability issue with multi hierarhical taxonomies, user may not be aware that there may be sub terms just by looking at overview page, so some explanation at the top and + sign or similar would be good to indicate that there are sub terms. Also, if you drill down in to sub terms there is no obvious way to go back up.

2 TAXONOMY PUBLISHER
Please add to Readme files where to configure it, it is not obvious that you need enable it for each vocabulary in
admin/config/taxonomy-tools/publisher

However when I did enable if for my taxonomies, I could than not see publish option anywhere, I expected it to be in edit term page.

3. Taxonomy redirect, same as above, please update readme

Redirect did work normal user, but for admin did not , instead I got this message at the top of taxonomy page:

"This taxonomy term is set to redirect to: node/23"

If this is a feature please document it

4. Taxonomy role access
Please document how to change access, i.e click on green icon in Overview peae, it is not obvious at first
Restriction did work.

This is much needed module for any site that makes serious use of taxonomies, so hurry up :)

automated review

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732
Review of the 7.x-1.x branch:

• Remove the translations folder, translations are done on http://localize.drupal.org
• Remove all old CVS $Id tags, they are not needed anymore.
  

  translations/lv.po:1:# $Id$
  

• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). 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.

manual review:

1. taxonomy_tools_enable(): do not juggle with module weights. If you want any specific hook to run before/after another module use hook_module_implements_alter() to change the order.
2. taxonomy_tools_menu_alter(): you don't need $break, just use "break 2;". See http://php.net/manual/en/control-structures.break.php
3. "drupal_alter('term_copy', $term);": all hooks that your module provides should be prefixed with your module's name to avoid name collisions.
4. "$form['field_taxonomy_term_status']['und']['#default_value'] = 1;": do not use "und", use LANGUAGE_NONE. Also elsewhere.

But that are just minor issues, otherwise I think this is RTBC. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Thanks for your contribution, martins.bertins!

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:

• 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 again, and good luck! :)