[D7] Taxonomy Mappings UI

Please fix the git error:

http://pareview.sh/pareview/httpgitdrupalorgsandboxharish1372654004git

Git errors:

Git default branch is not set, see the documentation on setting a default branch.
Review of the 7.x-1.x branch (commit ecbe4bb):

./taxonomy_mapping.module: all functions should be prefixed with your module/theme name to avoid name clashes. See https://www.drupal.org/node/318#naming

function _get_taxonomy_vocabulary($term_ref) {
function _get_vocabulary_name($vid) {
function _get_taxonomy_vocabulary_list() {
function _get_vocabulary_machine_name($vid) {
./Includes/taxonomy_mapping.admin.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See https://www.drupal.org/node/318#naming

function system_mappings_page_form($form, &$form_state) {
function system_mappings_page_form_submit($form, &$form_state) {
function ajax_callback_vocabulary_fields($form, &$form_state) {
Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
DrupalPractice has found some issues with your code, but could be false positives.
FILE: ...w/drupal-7-pareview/pareview_temp/Includes/taxonomy_mapping.admin.inc
---------------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
---------------------------------------------------------------------------
30 | WARNING | #description values usually have to run through t() for
| | translation
85 | WARNING | Unused variable $details.
106 | WARNING | Messages are user facing text and must run through t()
| | for translation
---------------------------------------------------------------------------

Time: 46ms; Memory: 4.25Mb

Codespell has found some spelling errors in your code.

./taxonomy_mapping.module:51: Returnd ==> Returned

updated issue summary

updated title

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 harish b

I've installed your module. I really liked the interface. Seemed to be intuitive and allowed many terms to be created quick and easy.

I did have problems when creating a term in an image field. When choosing an image file and clicking upload, I get the following:

Notice: Undefined index: fields_field_image in file_ajax_upload() (line 271 of /home/dnelson/sites/commerce/web/modules/file/file.module).
Notice: Undefined index: #suffix in file_ajax_upload() (line 280 of /home/dnelson/sites/commerce/web/modules/file/file.module).

All other field types I tested worked for aside from the image field. I'm using the Commerce Kickstart install profile if that makes a difference.

Though I didn't dig deep into the issue with the image field, I saw no issues with the project's code.

Hi harish b,

I have not installed your module yet, anyway i found this problems:

- taxonomy_mappings_menu() line 10

 'file' => '/Includes/taxonomy_mappings.admin.inc',

should be

 'file' => 'includes/taxonomy_mappings.admin.inc',

And better if pathname is lowercase

- taxonomy_mappings_get_vocabulary_name, taxonomy_mappings_get_taxonomy_vocabulary, taxonomy_mappings_get_vocabulary_machine_name
Why you don't use drupal api to do that instead of mysql queries?

- taxonomy_mappings.admin.inc line 74
Description should respect server limit for upload size you set a fixed 2M value

- taxonomy_mappings.admin.inc line 94,95
You are getting values from the form submission without any security check, and using them to build the term.
I could do this :

$vid = 'some text';
$name = FALSE;

When you assign this values you should check if $vid is int and if exist as vocabulary.
Then you should check if $name is not empty and sanitize the value.

Hey Harish,

Please remove version from "info" file.

FILE: /var/www/drupal-7-pareview/pareview_temp/taxonomy_mappings.info
---------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
---------------------------------------------------------------------------
1 | WARNING | Remove "version" form the info file, it will be added by
| | drupal.org packaging automatically
---------------------------------------------------------------------------

Hello harish b,

I think you add this file "includes/taxonomy_mappings.admin.inc~" by mistake, you need to remove it, and you need add one empty line between each function in taxonomy_mappings.module.

Hi harish_b,

I saw your reviews for other projects:

- https://www.drupal.org/node/2682243#comment-10971003
i don't see your manual review here.

- https://www.drupal.org/node/2687067#comment-10974891
this one is just an automated review did with pareview and codesniffer

- https://www.drupal.org/node/2689105
this one seems a project already approved, anyway not in the main issue queue.

Follow this guideline to get the review bonus https://www.drupal.org/node/1975228

Do at least 3 manual reviews of separate project applications in the main issue queue. While not mandatory, there is a template that you can use to make sure you cover all of the necessary points in your review.

Right, removing review bonus tag.

Hi Harish,

Please fix below issue:

FILE: .../drupal-7-pareview/pareview_temp/includes/taxonomy_mappings.admin.inc
---------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------
53 | ERROR | [x] Whitespace found at end of line
---------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------

Hi harish,

you should place your admin page admin/taxonomy/mappings under admin/structure/taxonomy/mappings and change the menu item type to MENU_NORMAL_ITEM to make the page reachable without going over the modules page and searching for your module.

function taxonomy_mappings_menu() {
  $items = array();
  $items['admin/structure/taxonomy/mappings'] = array(
    'title' => 'Taxonomy Mappings',
    'description' => 'System taxonomy mappings of all types of categories.',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('taxonomy_mappings_page_form'),
    'access arguments' => array('access mappings pages'),
    'file' => 'includes/taxonomy_mappings.admin.inc',
    'file path' => drupal_get_path('module', 'taxonomy_mappings'),
    'type' => MENU_NORMAL_ITEM,
  );
  return $items;
}

You should not commit files like taxonomy_mappings.admin.inc~ to the repository.

A little bit more info about how your module can be used, would be nice (module description, README). Its not very clear for me what your module is doing except providing an other UI for taxonomy term creation.

Best regards

Thank 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). Can you go back to that applications and set the status accordingly? Thanks!

manual review:

1. what is the use case of this module? What problem does it solve? The project page only contains one sentence: "Let your users create taxonomy individually based on vocabulary check." which does not explain much. What vocabulary check? So every user creates their own taxonomy terms for their user accounts? Or is this about taxonomy admins that create new vocabularies or terms? Can you give an example? See also https://www.drupal.org/node/997024
2. taxonomy_mappings_page_form(): doc block is wrong, this is not hook_menu(). See https://www.drupal.org/coding-standards/docs#forms
3. taxonomy_mappings_page_form(): do not call drupal_add_css() in form building function, use #attached instead. See https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h...
4. "drupal_set_message(t("Created new term(") . $name . ").");": do not concatenate variables to translatable strings like that. Use placeholders with t() instead, see https://api.drupal.org/api/drupal/includes!bootstrap.inc/function/t/7.x . This important to prevent XSS security issues. In your particular case I think the admin user could only XSS themselves, so this is probably not a direct security vulnerability in your module. It should be fixed in any case.

The t() variable usage is a blocker right now. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

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

I tried out the module using simplytest.me (entering 2654004 as the module key allows us to test sandboxes quickly).

Like Klausi, I also could not figure out from the project description or the project name what it is trying to do or solve. But the keywords sounded interesting enough for me to look into it. Turns out 'mapping' doesn't mean anything like I thought it would mean in this context! What does the word 'mapping' refer to here?

This module is a UI utility that helps editors add a number of taxonomy terms, one after the other, and choose which vocabulary they are assigned to on the fly

The existing taxonomy edit forms and processes currently expect you to first choose a vocabulary, and then choose to add a number of terms to it.
This module allows you to choose to add a term, and define which vocabulary it is in, in the same place.

I could figure all that out just by looking at the admin form I got (but had no idea from the text), so I think that a screenshot on your project page would be worth all the words that are currently on it.

Now that I understand that this is what the module does, I'm not convinced that this is a problem that needs solving, as an editor that hasn't figured out which vocab to be adding lots of terms to before they start adding terms - is acting pretty wooly-headed. However, I have seen some pretty weird workflows in my time, so I guess this is an itch that someone had to scratch.

To find the utility, I got there directly from the modules page (good).
After that however, I can't find it in the menu anywhere. I expected to see it as a tab or an action from the /admin/structure/taxonomy page. It's not there. This was reported earlier in #21

In order to try it out, I needed more than one vocab. So I added a new sample vocab, and gave it a few fields to play with.
Visiting /admin/structure/taxonomy/mappings after creating my vocab, I was presented with the same error 3x :

Notice: Undefined offset: 1 in taxonomy_mappings_page_form() (line 57 of /home/r09t6/www/sites/default/modules/2654004/includes/taxonomy_mappings.admin.inc).

I looked at line 57 http://cgit.drupalcode.org/sandbox-harish137-2654004/tree/includes/taxon...
and due to
* the lack of comments or explanation,
* and the scary nested cascade of ternery operators,
* and the mix of assignments and comparisons =/== in the same line

... I can't even suggest what was being attempted and failed there to generate the warning. This line of code (at least) is unmaintainable and deserves a slap.

Further down those lines of code, I see the reasons that made me wonder when I read your description about which fields were and were not being supported (why should this be an issue if you are using field_ui?).
:-(

Code there (through a layer of elsifs, not even a switch) seems to be attempting to replicate taxonomy_form_term() incompletely, by hand.

There are a number of problems with this, starting with the issues of the attempt to remake field_ui one field type at a time. Making a new type of form for term editing means that other modules who expect to do alters or enhancements to term edit forms can't see it. Most scary is that you are now bypassing the core term validation and submit handlers, and making your own (which do not support language translation, among other problems - like field permissions)

Dude, this is a bunch of problems here. Maybe it's hacky enough for a local one-off private project, but it's not written in a way that would co-exist with other modules in the Drupal ecosystem.

If you wanted to make this UI enhancement work, the thing to do would have been to create a form harness - just like you have with the vocab selector so far ...
... but then to nest, re-use, or wrap, the core term edit form, and load that by AJAX, with maybe a few clean-ups.
This would mean that most of the hard work (and another dozen hidden tasks) would continue to use the built-in field management UI and other triggers.

Synergy and re-use and compatibility with other Drupal modules would then happen.
You would then end up *not* even having to worry about supporting field types or unpredicted term schemas at all.

I'm not going to be able to give any +1s to this at the moment.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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