CVS edit link for obojan

My name is Bojan Opacic from the Jefferson Institute. Our goal is to bring simple visualization
capabilities to Drupal. We worked together as a team to initiate six modules which implement
Google Map, Earth and Visualization APIs.

1. Timeline Map
2. Tagmap
3. GVS module
4. ct_gearth
5. importer

The Jefferson Institute team is building these modules under a Knight Foundation News
Challenge grant. The main idea was to visualize data from database tables imported with
TableWizard or other importers. We also made a CSV, excel2003 and files importer to import
data in a database and to link the data with files. After that we, used the TableWizard module to
make a view.

6. vidi

vidi is latest module, it serves as a guide to choose adequate visualizations from Timeline, Tagmap and GVS modules for specific data, and helps in process of preparing data and visualizations. It provides easy to use wizard for Timeline, Tagmap and GVS modules with visualizations preview.

CommentFileSizeAuthor
#16 vidi.zip972.86 KBobojan
#14 vidi.zip971.93 KBobojan
#11 vidi.zip971.89 KBobojan
#10 vidi.zip971.89 KBobojan
#8 vidi.zip971.32 KBobojan
#3 vidi.zip974.34 KBobojan

Comments

obojan’s picture

obojan commented
Status: Postponed (maintainer needs more info) » Needs review
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Postponed (maintainer needs more info)

Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review the code, pointing out what it needs to be changed.

We review just a module / theme per applicant, which must be attached in a comment here. Without any code to review, the application cannot proceed.

obojan’s picture

obojan commented
Status: Postponed (maintainer needs more info) » Needs review
StatusFileSize
new vidi.zip974.34 KB

Hello, thank you for answering, and I'm sorry I was not able to reply earlier to your post. To clarify my previous post, I'm applying with only one module "vidi" which is an add-on (wizard) for previously listed. I'm uploading module code again in attachment. Thank you for your time.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Issue tags: +Module review

Thank you for your reply.

obojan’s picture

obojan commented

Hello, I just wanted to know how review is going? I much appreciate your time and effort, and if you need any additional information regarding "vidi" module, I'll be happy to reply. Thank you.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Needs work
  • This is only a partial review.
  • The points reported in this review are not in order of importance / relevance.
  • Most of the times I report the code that present an issue. In such cases, the same error can be present in other parts of the code; the fact I don't report the same issue more than once doesn't mean the same issue is not present in different places.
  • Not all the reported points are application blockers; some of the points I report are simple suggestions to who applies for a CVS account. For a list of what is considered a blocker for the application approval, see CVS applications review, what to expect. Keep in mind the list is still under construction, and can be changed to adapt it to what has been found out during code review, or to make the list clearer to who applies for a CVS account.
  1. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how the code should be formatted.
  2. Hook implementation comments should be like the following one: 
    /**
 * Implements hook_menu().
 */

  3.   $items['visualization-access-denied'] = array(
    'title' => 'Visualization',
    'page callback' => 'vidi_visualization_access_denied',
    'access arguments' => array(
      'visualize preview'
    ),
    'type' => MENU_CALLBACK,
    'file' => 'vidi_tagmap_form.inc'
  );

    What is the purpose of a menu callback that just returns a string about an access denied? What is the purpose of using access arguments for such menu callback?

  4. Strings used in the user interface should be translated. 
  5. function vidi_start_form_submit($form, &$form_state) {
  drupal_goto('vidi/chose_visualization_type', 'tablename=' . $form_state['values']['existing_tables']);
}

    Form submission handlers don't use drupal_goto(). 

  6.     $results = db_query("SELECT * FROM  {$tablename} LIMIT 5");

    For such cases, there is db_query_range(). 

  7.       $rows[] = array(array('data' => t('No entries in table !table', array('!table' => $tablename)), 'colspan' => count($fields)));

    If the content of $tablename is plain text, then it should be passed through check_plain(); that means the placeholder is not the correct one. 

  8.   else{
    $output = '<div class="vidi-preview-no-table error">' . t('The selected table doesn\'t exists in the tables that VIDI module is allowed to handle.') . '</div>';
  }

    Avoid to escape a string delimiter inside a string, especially if the string is passed to t().

obojan’s picture

obojan commented

Thank you for your time and review. I'm working on resolving all points you reported, and I'll upload fixed code as soon as I finish.

obojan’s picture

obojan commented
Status: Needs work » Needs review
StatusFileSize
new vidi.zip971.32 KB

I have fixed issues you reported:

  1. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how the code should be formatted.

    Fixed indenting and whitespace, converted files to Unix line nedings, and cleaned up commented out code

  2. Hook implementation comments should be like the following one

    Fixed.

  3. What is the purpose of a menu callback that just returns a string about an access denied? What is the purpose of using access arguments for such menu callback?

    Fixed. Removed callback, rewrote code that used it.

  4. Strings used in the user interface should be translated.

    Fixed. I have left some strings that explain usage in HTML tags in order to avoid text being to fragmented and hard to translate. If that is not recommend, I'll put only strings i t() function

  5. Form submission handlers don't use drupal_goto().

    Fixed. Rewrote code.

  6. For such cases, there is db_query_range().

    Fixed.

  7. If the content of $tablename is plain text, then it should be passed through check_plain(); that means the placeholder is not the correct one.

    Fixed. t("No entries in table @$tablename", array('@$tablename' => $tablename))

  8. Avoid to escape a string delimiter inside a string, especially if the string is passed to t().

    Fixed. Used alternative character

Thank you for review.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Needs work
  1. Coding standards also reports about the names to use for Drupal variables, constants, global variables, and functions defined from the module; Drupal variables don't respect the namespace.
  2. The module doesn't implement hook_uninstall(), or doesn't implement it to remove the Drupal variables it defines. 
  3.     $form['import_new_data'] =array(
      '#type' => 'item',
      '#value' => 'or ' . l('insert new data', 'importer', array('query' => 'return_link=vidi/chose_visualization_type'))
    );

    or is not translated.
    The correct is not 

        $form['import_new_data'] =array(
      '#type' => 'item',
      '#value' => t('or ') . l('insert new data', 'importer', array('query' => 'return_link=vidi/chose_visualization_type'))
    );


  4. function vidi_start_form_submit($form, &$form_state) {
  drupal_goto('vidi/chose_visualization_type', 'tablename=' . $form_state['values']['existing_tables']);
}

    A form submission handler is still using drupal_goto(). 

  5.     $results = db_query("show columns from {$tablename}");

    That query is not understood from all the database engines supported by Drupal (AFAIK).
    Reserved SQL keywords are written in uppercase. 

  6.       if ($result->Key == 'PRI')
        $header[] = array('data' => $result->Field . " (Primary Key)<BR/>Type: " . $result->Type);
      else
        $header[] = array('data' => $result->Field . "<BR/>Type: " . $result->Type);

    The coding standards describe how to write a control structure.
    The strings used as headers are not translated.
    The correct code is not

          if ($result->Key == 'PRI') {
        $header[] = array('data' => $result->Field . t(" (Primary Key)<BR/>Type: ") . $result->Type);
      }
      else {
        $header[] = array('data' => $result->Field . t("<BR/>Type: ") . $result->Type);
     }

    (Except for the curly parentheses, which are required by the coding standards.) 

  7.   if (module_exists('importer'))
    if (isset($datasource))
      $output .= '<div class="vidi-importer">' . l('Import new table', 'importer', array('query' => array('return_link' =>  $visualization, 'datasource' => $datasource))) . '</div>';
    else
      $output .= '<div class="vidi-importer">' . l('Import new table', 'importer', array('query' => array('return_link' =>  $visualization))) . '</div>';
  $output .= '<div class="vidi-preview-wrapper"></div>';

    What is wrong, here? 

  8.   if ($db_type == 'mysql' || $db_type == 'mysqli') {
    $sql = "SHOW TABLES";
  }
  elseif ($db_type == 'pgsql') {
    $sql = "SELECT tablename FROM {pg_tables} ORDER BY tablename";
  }
  else {
    drupal_set_message(t('Unrecognized database type %dbtype', array('%dbtype' => $db_type)));
    return;
  }

    If the module doesn't support all the database engines supported by Drupal, then it should implement hook_requirements() to avoid it is installed on a Drupal-powered site that is not using MySQL, or PostgreSQL. So far, those are the only supported database engines; checking for a third type is probably not necessary (at least for Drupal versions before Drupal 7). 

  9.     drupal_set_message('Enter Google maps key in Gmap settings', 'warning');

    The message strings should end with a period. 

  10.   if (isset($_GET['fid'])) {
    $sql = "SELECT * FROM {gvs_embeds} WHERE fid=" . $_GET['fid'];
    $embedingno=db_query_range($sql, 0, 1);
    $row_obj = db_fetch_object($embedingno);
    $defaults=unserialize($row_obj->form_state_storage);
    $sql1 = "SELECT * FROM {files} WHERE fid=" . $_GET['fid'];
    $embedingno1=db_query_range($sql1, 0, 1);
    $row_obj1 = db_fetch_object($embedingno1);
    $filename=$row_obj1->filename;
    $update=1;
  }

    Use placeholders, instead of injecting values into an SQL query. 

  11.   if (isset($_GET['datasource']))$requiredviz=$_GET['datasource'];else $requiredviz=2;
  $str='google.load("visualization", "1", {packages:["annotatedtimeline", "areachart", "barchart", "piechart", "columnchart", "linechart", "scatterchart", "imageareachart", "gauge", "motionchart", "geomap", "intensitymap", "map"]});

    function changefields(typeid) {
      switch (typeid) {
        case "0":
          $("#edit-xaxis1-wrapper").hide();
          $("#edit-xaxis-wrapper").show();

          $("#edit-yaxis11-wrapper").hide();
          $("#edit-yaxis1-wrapper").show();
          $("#edit-yaxis2-wrapper").hide();
          $("#edit-yaxis3-wrapper").hide();
          $("#edit-yaxis4-wrapper").hide();
          $("#edit-yaxis5-wrapper").hide();

          $("#edit-zaxis1-wrapper").hide();
          $("#edit-zaxis2-wrapper").hide();
          $("#edit-zaxis3-wrapper").hide();
          $("#edit-zaxis4-wrapper").hide();
          $("#edit-zaxis5-wrapper").hide();

          $(".zzaxis").hide();

          $("#edit-mcdateformat-wrapper").hide();
          $("#edit-title-wrapper").show();
          $("#edit-enableTooltip-wrapper").hide();
          $("#edit-isStacked-wrapper").hide();
          $("#edit-is3D-wrapper").show();
          $("#edit-linlog-wrapper").hide();
          $("#edit-legend-wrapper").show();
          $(".gauge").hide();
          $(".map1").hide();
          $(".map2").hide();
          $(".map3").hide();
          $("#edit-geomapstring-wrapper").hide();
          $("#edit-maptype-wrapper").hide();
          $(".atl").hide();
          $(".motion").hide();
          $(".labels").hide();
          $("#edit-color-wrapper").show();
          break;
        case "1":
          $("#edit-xaxis1-wrapper").hide();
          $("#edit-xaxis-wrapper").show();

          $("#edit-yaxis11-wrapper").hide();
          $("#edit-yaxis1-wrapper").show();
          $("#edit-yaxis2-wrapper").hide();
          $("#edit-yaxis3-wrapper").hide();
          $("#edit-yaxis4-wrapper").hide();
          $("#edit-yaxis5-wrapper").hide();

          $("#edit-zaxis1-wrapper").hide();
          $("#edit-zaxis2-wrapper").hide();
          $("#edit-zaxis3-wrapper").hide();
          $("#edit-zaxis4-wrapper").hide();
          $("#edit-zaxis5-wrapper").hide();

          $(".zzaxis").hide();

          $("#edit-mcdateformat-wrapper").show();
          $("#edit-enableTooltip-wrapper").hide();
          $("#edit-isStacked-wrapper").hide();
          $("#edit-is3D-wrapper").hide();
          $("#edit-linlog-wrapper").hide();
          $("#edit-legend-wrapper").hide();
          $(".gauge").show();
          $(".map1").hide();
          $(".map2").hide();
          $(".map3").hide();
          $("#edit-geomapstring-wrapper").hide();
          $("#edit-maptype-wrapper").hide();

          $(".atl").hide();
          $(".motion").hide();
          $(".labels").hide();
          $("#edit-color-wrapper").hide();
          $("#edit-title-wrapper").show();
          $("#edit-mcdateformat-wrapper").hide();
          break;
        case "2":
          $("#edit-xaxis1-wrapper").hide();
          $("#edit-xaxis-wrapper").show();

          $("#edit-yaxis11-wrapper").hide();
          $("#edit-yaxis1-wrapper").show();
          $("#edit-yaxis2-wrapper").show();
          $("#edit-yaxis3-wrapper").show();
          $("#edit-yaxis4-wrapper").show();
          $("#edit-yaxis5-wrapper").show();

          $("#edit-zaxis1-wrapper").hide();
          $("#edit-zaxis2-wrapper").hide();
          $("#edit-zaxis3-wrapper").hide();
          $("#edit-zaxis4-wrapper").hide();
          $("#edit-zaxis5-wrapper").hide();

          $("#edit-enableTooltip-wrapper").hide();
          $("#edit-isStacked-wrapper").show();
          $("#edit-is3D-wrapper").show();
          $("#edit-linlog-wrapper").show();
          $("#edit-legend-wrapper").show();
          $(".gauge").hide();
          $(".map1").hide();
          $(".map2").hide();
          $(".map3").hide();
          $("#edit-geomapstring-wrapper").hide();
          $("#edit-maptype-wrapper").hide();

          $(".zzaxis").hide();
          $(".atl").hide();
          $(".motion").hide();
          $(".labels").hide();
          $("#edit-color-wrapper").show();
          $("#edit-title-wrapper").show();
          $("#edit-mcdateformat-wrapper").hide();
          break;
        case "3":
          $("#edit-xaxis1-wrapper").hide();
          $("#edit-xaxis-wrapper").show();

          $("#edit-yaxis11-wrapper").hide();
          $("#edit-yaxis1-wrapper").show();
          $("#edit-yaxis2-wrapper").show();
          $("#edit-yaxis3-wrapper").show();
          $("#edit-yaxis4-wrapper").show();
          $("#edit-yaxis5-wrapper").show();

          $("#edit-zaxis1-wrapper").hide();
          $("#edit-zaxis2-wrapper").hide();
          $("#edit-zaxis3-wrapper").hide();
          $("#edit-zaxis4-wrapper").hide();
          $("#edit-zaxis5-wrapper").hide();

          $("#edit-enableTooltip-wrapper").hide();
          $("#edit-isStacked-wrapper").show();
          $("#edit-is3D-wrapper").hide();
          $("#edit-linlog-wrapper").show();
          $("#edit-legend-wrapper").show();
          $(".gauge").hide();
          $(".map1").hide();
          $(".map2").hide();
          $(".map3").hide();
          $("#edit-geomapstring-wrapper").hide();
          $("#edit-maptype-wrapper").hide();

          $(".zzaxis").hide();
          $(".atl").hide();
          $(".motion").hide();
          $(".labels").hide();
          $("#edit-color-wrapper").show();
          $("#edit-title-wrapper").show();
          $("#edit-mcdateformat-wrapper").hide();
          break;
        case "4":
          $("#edit-xaxis1-wrapper").hide();
          $("#edit-xaxis-wrapper").show();

          $("#edit-yaxis11-wrapper").hide();
          $("#edit-yaxis1-wrapper").show();
          $("#edit-yaxis2-wrapper").show();
          $("#edit-yaxis3-wrapper").show();
          $("#edit-yaxis4-wrapper").show();
          $("#edit-yaxis5-wrapper").show();

          $("#edit-zaxis1-wrapper").hide();
          $("#edit-zaxis2-wrapper").hide();
          $("#edit-zaxis3-wrapper").hide();
          $("#edit-zaxis4-wrapper").hide();
          $("#edit-zaxis5-wrapper").hide();

          $("#edit-enableTooltip-wrapper").hide();
          $("#edit-isStacked-wrapper").show();
          $("#edit-is3D-wrapper").show();
          $("#edit-linlog-wrapper").show();
          $("#edit-legend-wrapper").show();
          $(".gauge").hide();
          $(".map1").hide();
          $(".map2").hide();
          $(".map3").hide();
          $("#edit-geomapstring-wrapper").hide();
          $("#edit-maptype-wrapper").hide();

          $(".zzaxis").hide();
          $(".atl").hide();
          $(".motion").hide();
          $(".labels").hide();
          $("#edit-title-wrapper").show();
          $("#edit-mcdateformat-wrapper").hide();
          break;
        case "5":
          $("#edit-xaxis1-wrapper").hide();
          $("#edit-xaxis-wrapper").show();

          $("#edit-yaxis11-wrapper").hide();
          $("#edit-yaxis1-wrapper").show();
          $("#edit-yaxis2-wrapper").show();
          $("#edit-yaxis3-wrapper").show();
          $("#edit-yaxis4-wrapper").show();
          $("#edit-yaxis5-wrapper").show();

          $("#edit-zaxis1-wrapper").hide();
          $("#edit-zaxis2-wrapper").hide();
          $("#edit-zaxis3-wrapper").hide();
          $("#edit-zaxis4-wrapper").hide();
          $("#edit-zaxis5-wrapper").hide();

          $(".zzaxis").hide();

          $("#edit-enableTooltip-wrapper").hide();
          $("#edit-isStacked-wrapper").hide();
          $("#edit-is3D-wrapper").hide();
          $("#edit-linlog-wrapper").show();
          $("#edit-legend-wrapper").show();
          $(".gauge").hide();
          $(".map1").hide();
          $(".map2").hide();
          $(".map3").hide();
          $("#edit-geomapstring-wrapper").hide();
          $("#edit-maptype-wrapper").hide();

          $(".atl").hide();
          $(".motion").hide();
          $(".labels").hide();
          $("#edit-color-wrapper").show();
          $("#edit-title-wrapper").show();
          $("#edit-mcdateformat-wrapper").hide();
          break;
        case "6":
          $("#edit-xaxis-wrapper").hide();
          $("#edit-xaxis1-wrapper").show();

          $("#edit-yaxis11-wrapper").hide();
          $("#edit-yaxis1-wrapper").show();
          $("#edit-yaxis2-wrapper").show();
          $("#edit-yaxis3-wrapper").show();
          $("#edit-yaxis4-wrapper").show();
          $("#edit-yaxis5-wrapper").show();

          $("#edit-zaxis1-wrapper").hide();
          $("#edit-zaxis2-wrapper").hide();
          $("#edit-zaxis3-wrapper").hide();
          $("#edit-zaxis4-wrapper").hide();
          $("#edit-zaxis5-wrapper").hide();

          $("#edit-enableTooltip-wrapper").hide();
          $("#edit-isStacked-wrapper").hide();
          $("#edit-is3D-wrapper").hide();
          $("#edit-linlog-wrapper").show();
          $("#edit-legend-wrapper").show();
          $(".gauge").hide();
          $(".map1").hide();
          $(".map3").hide();
          $(".map2").hide();
          $("#edit-geomapstring-wrapper").hide();
          $("#edit-maptype-wrapper").hide();

          $(".zzaxis").hide();
          $(".atl").hide();
          $(".motion").hide();
          $(".labels").hide();
          $("#edit-color-wrapper").show();
          $("#edit-title-wrapper").show();
          $("#edit-mcdateformat-wrapper").hide();
          break;
        case "7":
          $("#edit-xaxis1-wrapper").hide();
          $("#edit-xaxis-wrapper").show();

          $("#edit-yaxis11-wrapper").hide();
          $("#edit-yaxis1-wrapper").show();
          $("#edit-yaxis2-wrapper").hide();
          $("#edit-yaxis3-wrapper").hide();
          $("#edit-yaxis4-wrapper").hide();
          $("#edit-yaxis5-wrapper").hide();

          $("#edit-zaxis1-wrapper").hide();
          $("#edit-zaxis2-wrapper").hide();
          $("#edit-zaxis3-wrapper").hide();
          $("#edit-zaxis4-wrapper").hide();
          $("#edit-zaxis5-wrapper").hide();

          $("#edit-enableTooltip-wrapper").hide();
          $("#edit-isStacked-wrapper").hide();
          $("#edit-is3D-wrapper").hide();
          $("#edit-linlog-wrapper").hide();
          $("#edit-legend-wrapper").hide();
          $(".gauge").hide();

          $(".map1").show();
          if ($("#edit-geomaptype").val()=="regions") {
            $("#edit-geomaplat-wrapper").hide();
            $("#edit-geomaplon-wrapper").hide();
          }
          else {
            $("#edit-geomapla-wrappert").show();
            $("#edit-geomaplon-wrapper").show();
          }

          $(".map2").hide();
          $(".map3").hide();
          $("#edit-geomapstring-wrapper").show();
          $("#edit-maptype-wrapper").hide();

          $(".zzaxis").hide();
          $(".atl").hide();
          $(".motion").hide();
          $(".labels").hide();
          $("#edit-color-wrapper").show();
          $("#edit-title-wrapper").show();
          $("#edit-mcdateformat-wrapper").hide();
          break;
        case "8":
          $("#edit-xaxis1-wrapper").hide();
          $("#edit-xaxis-wrapper").show();

          $("#edit-yaxis11-wrapper").hide();
          $("#edit-yaxis1-wrapper").show();
          $("#edit-yaxis2-wrapper").show();
          $("#edit-yaxis3-wrapper").show();
          $("#edit-yaxis4-wrapper").show();
          $("#edit-yaxis5-wrapper").show();

          $("#edit-zaxis1-wrapper").hide();
          $("#edit-zaxis2-wrapper").hide();
          $("#edit-zaxis3-wrapper").hide();
          $("#edit-zaxis4-wrapper").hide();
          $("#edit-zaxis5-wrapper").hide();

          $("#edit-enableTooltip-wrapper").hide();
          $("#edit-isStacked-wrapper").hide();
          $("#edit-is3D-wrapper").hide();
          $("#edit-linlog-wrapper").hide();
          $("#edit-legend-wrapper").hide();
          $(".gauge").hide();
          $(".map1").hide();
          $(".map2").hide();
          $(".map3").show();
          $("#edit-maptype-wrapper").hide();
          $("#edit-geomapstring-wrapper").hide();

          $(".zzaxis").hide();
          $(".atl").hide();
          $(".motion").hide();
          $(".labels").hide();
          $("#edit-color-wrapper").hide();
          $("#edit-title-wrapper").show();
          $("#edit-mcdateformat-wrapper").hide();
          break;
        case "9":
          $("#edit-xaxis1-wrapper").hide();
          $("#edit-xaxis-wrapper").show();

          $("#edit-yaxis11-wrapper").show();
          $("#edit-yaxis1-wrapper").hide();
          $("#edit-yaxis2-wrapper").show();
          $("#edit-yaxis3-wrapper").show();
          $("#edit-yaxis4-wrapper").show();
          $("#edit-yaxis5-wrapper").show();

          $("#edit-zaxis1-wrapper").show();
          $("#edit-zaxis2-wrapper").show();
          $("#edit-zaxis3-wrapper").show();
          $("#edit-zaxis4-wrapper").show();
          $("#edit-zaxis5-wrapper").show();

          $("#edit-enableTooltip-wrapper").hide();
          $("#edit-isStacked-wrapper").hide();
          $("#edit-is3D-wrapper").hide();
          $("#edit-linlog-wrapper").hide();
          $("#edit-legend-wrapper").hide();
          $(".gauge").hide();
          $(".map1").hide();
          $(".map2").hide();
          $(".map3").hide();
          $("#edit-geomapstring-wrapper").hide();
          $("#edit-maptype-wrapper").hide();

          $(".zzaxis").hide();
          $(".atl").hide();
          $(".motion").show();
          $(".labels").hide();
          $("#edit-color-wrapper").hide();
          $("#edit-title-wrapper").show();
          $("#edit-mcdateformat-wrapper").show();
          break;
        case "10":
          $("#edit-xaxis1-wrapper").hide();
          $("#edit-xaxis-wrapper").hide();

          $("#edit-yaxis11-wrapper").show();
          $("#edit-yaxis1-wrapper").show();
          $("#edit-yaxis2-wrapper").show();
          $("#edit-yaxis3-wrapper").show();
          $("#edit-yaxis4-wrapper").show();
          $("#edit-yaxis5-wrapper").show();

          $("#edit-zaxis1-wrapper").show();
          $("#edit-zaxis2-wrapper").show();
          $("#edit-zaxis3-wrapper").show();
          $("#edit-zaxis4-wrapper").show();
          $("#edit-zaxis5-wrapper").show();

          $("#edit-enableTooltip-wrapper").hide();
          $("#edit-isStacked-wrapper").hide();
          $("#edit-is3D-wrapper").hide();
          $("#edit-linlog-wrapper").hide();
          $("#edit-legend-wrapper").hide();
          $(".gauge").hide();
          $(".map1").hide();
          $(".map2").hide();
          $(".map3").hide();
          $("#edit-geomapstring-wrapper").hide();
          $("#edit-maptype-wrapper").hide();

          $(".zzaxis").show();
          $(".atl").show();
          $(".motion").hide();
          $(".labels").hide();
          $("#edit-color-wrapper").show();
          $("#edit-title-wrapper").show();
          $("#edit-mcdateformat-wrapper").hide();
          break;
        case "11":
          $("#edit-xaxis1-wrapper").hide();
          $("#edit-xaxis-wrapper").show();

          $("#edit-yaxis11-wrapper").hide();
          $("#edit-yaxis1-wrapper").show();
          $("#edit-yaxis2-wrapper").hide();
          $("#edit-yaxis3-wrapper").hide();
          $("#edit-yaxis4-wrapper").hide();
          $("#edit-yaxis5-wrapper").hide();

          $("#edit-zaxis1-wrapper").hide();
          $("#edit-zaxis2-wrapper").hide();
          $("#edit-zaxis3-wrapper").hide();
          $("#edit-zaxis4-wrapper").hide();
          $("#edit-zaxis5-wrapper").hide();

          $("#edit-enableTooltip-wrapper").hide();
          $("#edit-isStacked-wrapper").hide();
          $("#edit-is3D-wrapper").hide();
          $("#edit-linlog-wrapper").hide();
          $("#edit-legend-wrapper").hide();
          $(".gauge").hide();
          $(".map1").hide();
          $(".map2").show();
          $("#edit-geomapstring-wrapper").show();
          $("#edit-maptype-wrapper").show();

          $(".map3").hide();
          $(".zzaxis").hide();
          $(".atl").hide();
          $(".motion").hide();
          $(".labels").hide();
          $("#edit-color-wrapper").hide();
          $("#edit-title-wrapper").show();
          $("#edit-mcdateformat-wrapper").hide();
          break;
        };
    }

    function resend() {
      $.ajax({
        url: "' . $nekiurl . '",
        data: "table=' . $tablename . '&type="+$("#edit-type").val()+"&title="+$("#edit-title").val()+"&xaxis1="+$("#edit-xaxis1").val()+"&xaxis="+$("#edit-xaxis").val()+"&yaxis11="+$("#edit-yaxis11").val()+"&yaxis1="+$("#edit-yaxis1").val()+"&yaxis2="+$("#edit-yaxis2").val()+"&yaxis3="+$("#edit-yaxis3").val()+"&yaxis4="+$("#edit-yaxis4").val()+"&yaxis5="+$("#edit-yaxis5").val()
            +"&zaxis2="+$("#edit-zaxis2").val()+"&zaxis3="+$("#edit-zaxis3").val()+"&zaxis4="+$("#edit-zaxis4").val()+"&zaxis5="+$("#edit-zaxis5").val()
            +"&zaxis1="+$("#edit-zaxis1").val()+"&zzaxis1="+$("#edit-zzaxis1").val()+"&zzaxis2="+$("#edit-zzaxis2").val()+"&zzaxis3="+$("#edit-zzaxis3").val()+"&zzaxis4="+$("#edit-zzaxis4").val()+"&zzaxis5="+$("#edit-zzaxis5").val()
            +"&geomaptype="+$("#edit-geomaptype").val()+"&geomaplat="+$("#edit-geomaplat").val()+"&geomaplon="+$("#edit-geomaplon").val()+"&geomaplat1="+$("#edit-geomaplat1").val()+"&geomaplon1="+$("#edit-geomaplon1").val()
            +"&geomapstring="+$("#edit-geomapstring").val()+"&maptype="+$("#edit-maptype").val()+"&isStacked="+$("#edit-isStacked").is(":checked")+"&is3D="+$("#edit-is3D:checkbox").is(":checked")+"&linlog="+$("#edit-linlog").val()+"&legend="+$("#edit-legend").val()
            +"&region1="+$("#edit-region1").val()+"&region="+$("#edit-region").val()+"&color="+$("#edit-color").val()+"&mcdateformat="+$("#edit-mcdateformat").val()+"&clickedembed=0&update=' . $update . '&filename=' . $filename . '",
          success: function(data) {
            $("#chart_div").show();
            $("#embedinstructons").show();
            $("#embedlink").show();
            $("#back-button").show();
            $("#addscrip").html(data);
            drawChart();
          }
      });
    }
    ';

    All that JavaScript code should go in a separated file. 

  12.   if ($_GET['new_data'] == 'TRUE')
    $link = l('<img src = "' . base_path() . path_to_theme() . '/images/try_another_type_s.jpg" />', 'vidi/chose_visualization_type', array('html' => TRUE, 'query' => 'tablename=' . $form_state['storage']['tablename']));
  else
    $link = l('<img src = "' . base_path() . path_to_theme() . '/images/try_another_type_s.jpg" />', 'vidi/chose_visualization_type', array('html' => TRUE));

    There is a theme function for images.
    The code should verify if those images are really present in the directory of the currently set theme. 

  13.   $form['data']['geomaplon'] = array(
    '#title' => t('Longitude'),
    '#description' => t('Must be number.'),
    '#type' => 'select',
    '#weight' => 8,
    '#default_value' => isset($defaults['geomaplon'])?$defaults['geomaplon']:'NULL',
    '#options' => $numeric_fields,
    '#prefix' => '<div class="map1">',
    '#suffix' => '</div>',
  );

    The description should say something more that the input must be a number. 

  14.     $form['submit'] = array(
      '#weight' => 101,
      '#type' => 'submit',
      '#value' => 'Save>>',
      '#prefix' => '<INPUT id="previewbt" type="button" value="Preview"/>'
    );

    Form fields are generated using the form API.

obojan’s picture

obojan commented
Status: Needs work » Needs review
StatusFileSize
new vidi.zip971.89 KB
  1. Coding standards also reports about the names to use for Drupal variables, constants, global variables, and functions defined from the module; Drupal variables don't respect the namespace.

    Fixed. Changed isStacked to is_stacked and
    returning_gvs to vidi_returning_gvs

  2. The module doesn't implement hook_uninstall(), or doesn't implement it to remove the Drupal variables it defines.

    There is vidi_uninstall() which removes dbtable “vidi_visualization_tables”. Other than that, module does not define any variables to dbtable “variable”. Is there something additional I should clean up on uninstall? 

  3.       <?php
          $form['import_new_data'] =array(
            '#type' => 'item',
            '#value' => 'or ' . l('insert new data', 'importer', array('query' => 'return_link=vidi/chose_visualization_type'))
          );
      ?>

    or is not translated.
    The correct is not

          <?php
          $form['import_new_data'] =array(
            '#type' => 'item',
            '#value' => t('or ') . l('insert new data', 'importer', array('query' => 'return_link=vidi/chose_visualization_type'))
          );
      ?>

    Fixed.

  4.       <?php
      function vidi_start_form_submit($form, &$form_state) {
        drupal_goto('vidi/chose_visualization_type', 'tablename=' . $form_state['values']['existing_tables']);
      }
      ?>

    A form submission handler is still using drupal_goto().

    Fixed. I’m sorry I missed it.

  5.       <?php
          $results = db_query("show columns from {$tablename}");
      ?>

    That query is not understood from all the database engines supported by Drupal (AFAIK).
    Reserved SQL keywords are written in uppercase.

    Fixed. 

          <?php


 if ($db_type == 'mysql' || $db_type == 'mysqli') {
        $results = db_query("SHOW COLUMNS FROM {$tablename}");
        while ($result = db_fetch_object($results)) {
          if ($result->Type == "float" || substr($result->Type, 0, 3) == "int") {
            $options['numerical'][$result->Field] = $result->Field;
          }
          else {
            $options['string'][$result->Field] = $result->Field;
          }
        }
      }
      else if ($db_type == 'pgsql') {
        $results = db_query("SELECT COLUMN_NAME, DATA_TYPE FROM information_schema.columns WHERE table_name = {$tablename}");
        while ($result = db_fetch_object($results)) {
          if ($result->data_type == "float" || substr($result->data_type, 0, 3) == "int") {
            $options['numerical'][$result->column_name] = $result->column_name;
          }
          else {
            $options['string'][$result->column_name] = $result->column_name;
          }
        }
      }
      ?>
	
  6.       <?php
            if ($result->Key == 'PRI')
              $header[] = array('data' => $result->Field . " (Primary Key)<BR/>Type: " . $result->Type);
            else
              $header[] = array('data' => $result->Field . "<BR/>Type: " . $result->Type);
      ?>

    The coding standards describe how to write a control structure.
    The strings used as headers are not translated.
    The correct code is not

          <?php
            if ($result->Key == 'PRI') {
              $header[] = array('data' => $result->Field . t(" (Primary Key)<BR/>Type: ") . $result->Type);
            }
            else {
              $header[] = array('data' => $result->Field . t("<BR/>Type: ") . $result->Type);
           }
      ?>

    (Except for the curly parentheses, which are required by the coding standards.)

    Fixed.

  7.       <?php
        if (module_exists('importer'))
          if (isset($datasource))
            $output .= '<div class="vidi-importer">' . l('Import new table', 'importer', array('query' => array('return_link' =>  $visualization, 'datasource' => $datasource))) . '</div>';
          else
            $output .= '<div class="vidi-importer">' . l('Import new table', 'importer', array('query' => array('return_link' =>  $visualization))) . '</div>';
        $output .= '<div class="vidi-preview-wrapper"></div>';
      ?>

    What is wrong, here?

    Fixed. I added curly braces as coding standard for control structures describes. Line $output .= '<div class="vidi-preview-wrapper"></div>'; adds div where ajax callback later inserts data for display. 

  8.       <?php
        if ($db_type == 'mysql' || $db_type == 'mysqli') {
          $sql = "SHOW TABLES";
        }
        elseif ($db_type == 'pgsql') {
          $sql = "SELECT tablename FROM {pg_tables} ORDER BY tablename";
        }
        else {
          drupal_set_message(t('Unrecognized database type %dbtype', array('%dbtype' => $db_type)));
          return;
        }
      ?>

    If the module doesnt support all the database engines supported by Drupal, then it should implement hook_requirements() to avoid it is installed on a Drupal-powered site that is not using MySQL, or PostgreSQL. So far, those are the only supported database engines; checking for a third type is probably not necessary (at least for Drupal versions before Drupal 7).

    Fixed. Erased checking for a third type.

  9.       <?php
          drupal_set_message('Enter Google maps key in Gmap settings', 'warning');
      ?>

    The message strings should end with a period.

    Fixed.

  10.       <?php
        if (isset($_GET['fid'])) {
          $sql = "SELECT * FROM {gvs_embeds} WHERE fid=" . $_GET['fid'];
          $embedingno=db_query_range($sql, 0, 1);
          $row_obj = db_fetch_object($embedingno);
          $defaults=unserialize($row_obj->form_state_storage);
          $sql1 = "SELECT * FROM {files} WHERE fid=" . $_GET['fid'];
          $embedingno1=db_query_range($sql1, 0, 1);
          $row_obj1 = db_fetch_object($embedingno1);
          $filename=$row_obj1->filename;
          $update=1;
        }
      ?>

    Use placeholders, instead of injecting values into an SQL query.

    Fixed.

  11.       <?php
        if (isset($_GET['datasource']))$requiredviz=$_GET['datasource'];else $requiredviz=2;
        $str='google.load...';
      ?>

    All that JavaScript code should go in a separated file.

    Fixed. This code is needed inside head tags in order to function properly with Google maps API.

  12.       <?php
        if ($_GET['new_data'] == 'TRUE')
          $link = l('<img src = "' . base_path() . path_to_theme() . '/images/try_another_type_s.jpg" />', 'vidi/chose_visualization_type', array('html' => TRUE, 'query' => 'tablename=' . $form_state['storage']['tablename']));
        else
          $link = l('<img src = "' . base_path() . path_to_theme() . '/images/try_another_type_s.jpg" />', 'vidi/chose_visualization_type', array('html' => TRUE));
      ?>

    There is a theme function for images.
    The code should verify if those images are really present in the directory of the currently set theme.

    Fixed. 

    $image_path = base_path() . path_to_theme() . '/images/try_another_type_s.jpg';
  if(file_exists($image_path)) {
    $link_argument = theme_image("$image_path");
  }
  else {
    $link_argument = "Try another type";
  }
  
  if ($_GET['new_data'] == 'TRUE') {
    $link = l($link_argument, 'vidi/chose_visualization_type', array('html' => TRUE, 'query' => 'tablename=' . $form_state['storage']['tablename']));
  }
  else {
    $link = l($link_argument, 'vidi/chose_visualization_type', array('html' => TRUE));
  }

  13.       <?php
        $form['data']['geomaplon'] = array(
          '#title' => t('Longitude'),
          '#description' => t('Must be number.'),
          '#type' => 'select',
          '#weight' => 8,
          '#default_value' => isset($defaults['geomaplon'])?$defaults['geomaplon']:'NULL',
          '#options' => $numeric_fields,
          '#prefix' => '<div class="map1">',
          '#suffix' => '</div>',
        );
      ?>

    The description should say something more that the input must be a number.

    Fixed.

  14.   
      <?php
          $form['submit'] = array(
            '#weight' => 101,
            '#type' => 'submit',
            '#value' => 'Save>>',
            '#prefix' => '<INPUT id="previewbt" type="button" value="Preview"/>'
          );
      ?>

    Form fields are generated using the form API.

    Fixed.

Thank you for review.

obojan’s picture

obojan commented
StatusFileSize
new vidi.zip971.89 KB

I have fixed several code formatting warnings I got reported from coder module, and I have attached module again. Thank you for reviewing my application.

Scyther’s picture

Scyther commented
Status: Needs review » Needs work

1.

/**
 * Implements hook_menu().
 */
function vidi_menu() {
  $items[] = array();

the way you have write it means

  $items = array(array());

should be

  $items = array();

2.

function _vidi_tagmap_simple_sql($values) {
  $sql = "SELECT * FROM {" . $values['tablename'];
  return $sql;
}

Missing end } around the table name. Don't know if you use this function more, but else I would do like this:

Replace, line 52, 53 in vidi_tagmap_form.inc

  $sql = _vidi_tagmap_simple_sql($values);
  $results = db_query($sql);

with this

  $results = db_query("SELECT * FROM {'%s'}", $values['tablename']);

(think that '%s' will work there) and remove the function _vidi_tagmap_simple_sql($...)

3.

Still missing { } round if

  if (isset($width)) $width .= 'px';
  if (isset($height)) $height .= 'px';

  foreach ($sort_array as $key => $value) {
    if ($value['add'] == 0)
      unset($sort_array[$key]);
  }

and at alot more places.

4. More comments on functions and what they do would be handy.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented 
  $results = db_query("SELECT * FROM {'%s'}", $values['tablename']);

Placeholders are not thought for table names; the code should be written as 

  $results = db_query("SELECT * FROM {" . $values['tablename'] . "}");
obojan’s picture

obojan commented
Status: Needs work » Needs review
StatusFileSize
new vidi.zip971.93 KB
  1. should be 
    <?php
  $items = array();
?>

    Fixed.

  2. Fixed. Removed function _vidi_tagmap_simple_sql() and rewrote as

    <?php
  $results = db_query("SELECT * FROM {" . $values['tablename'] . "}");
?>
  3. Still missing { } round if ... and at alot more places.

    Fixed.

Thank you for review and all comments.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Needs work
  • This is a partial review.
  • The points reported in this review are not in order of importance / relevance.
  • Most of the times I report the code that present an issue. In such cases, the same error can be present in other parts of the code; the fact I don't report the same issue more than once doesn't mean the same issue is not present in different places.
  • Not all the reported points are application blockers; some of the points I report are simple suggestions to who applies for a CVS account. For a list of what is considered a blocker for the application approval, see CVS applications review, what to expect. Keep in mind the list is still under construction, and can be changed to adapt it to what has been found out during code review, or to make the list clearer to who applies for a CVS account. 
  1.       else {
        $rows[] = array($result->tablename . ' - ' . '<a title="' . $result->tablename . '" id="' . $result->tablename . '" class="vidi-table-ajax-callback" href="javascript: void(0);">preview</a>',
          date('M d, Y H:i:s', mktime((int)$hour, (int)$minute, (int)$second, (int)$month, (int)$day, (int)$year)),
          l('Visualize ' . $result->tablename, 'vidi/' . $visualization . '/' .  $result->tablename, array('query' => array('datasource' => $datasource))));
      }

    Strings used in the user interface should be translated. 

  2.   drupal_set_html_head('<style type="text/css">.form-item label{float:left;margin:0px 10px 0px 10px;}</style>');

    Why isn't the code using a CSS file, instead of calling drupal_set_html_head()?

  3. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how the code should be formatted. 
  4.     $form['#prefix'] = '<div id = "timelinemap-all-vidi-preview" class = "timelinemap-all">
                        <div class = "timelinecontainer" style = "width: '. $form_state['storage']['width'] . '; height:' . $form_state['storage']['tlheight'] . '">
                          <div id = "timeline-vidi-preview" class = "timeline"></div>
                        </div>
                        <div class = "mapcontainer" style = "width: '. $form_state['storage']['width'] . '; height:' . $form_state['storage']['mheight'] . '">
                          <div id = "map-vidi-preview" class = "map"></div>
                        </div>
                        </div>' . $embed_str . '<div id = "back-button">' . $link . '</div><div class = "clear"></div>';

    The code is outputting a lot of HTML without using a theme function. 

  5.   if ($cnt == 0) {
    tw_add_tables(array(schema_unprefix_table($visualisation_form['tablename']['#value'])), FALSE, FALSE);
  }

    Where is the function tw_add_tables() defined?

obojan’s picture

obojan commented
Status: Needs work » Needs review
StatusFileSize
new vidi.zip972.86 KB 
  1.    l('Visualize ' . $result->tablename...

    Fixed. Fixed here and in all link functions.

  2. <?php
  drupal_set_html_head('<style type="text/css">.form-item label{float:left;margin:0px 10px 0px 10px;}</style>');
?>

    I fixed this and also reorganized .css files better.

  3. Fixed all that coder module reported.

  4. <?php
  $prefix_str = theme('vidi_timelinemap_form_prefix', $form_state['storage'], $embed_str, $link);
  $form['#prefix'] = $prefix_str;  
?>

    Fixed. Moved to theme function.

  5. It is in table wizard (tw) module. Vidi module is depends on tw module, vidi.info: 

    ...
dependencies[] = tw

Thank you for reviewing and for all comments.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Fixed 
  1.     drupal_set_message(t('To use Google Visualizations, please download the gvs module from ') . l(t('here'), 'http://drupal.org/project/gvs', array('attributes' => array('target' => '_blank'))));

    l() should not be used together with t(). See the documentation for t(), where this code is reported to be wrong:

    $output .= t('<p>Go to the @contact-page.</p>', array('@contact-page' => l(t('contact page'), 'contact')));

    The correct code to use is the following:

    $output .= '<p>'. t('Go to the <a href="@contact-page">contact page</a>.', array('@contact-page' => url('contact'))) .'</p>';

  2.   $form['data']['geomaplat1'] = array(
    '#title' => t('Latitude'),
    '#description' => t('Must be number specifying location latitude. Precision beyond the 6 decimal places is ignored.'),
    '#type' => 'select',
    '#weight' => 9,
    '#options' => $numeric_fields,
    '#default_value' => isset($defaults['geomaplat1'])?$defaults['geomaplat1']:'NULL',
    '#prefix' => '<div class="map2">',
    '#suffix' => '</div>',
  );

    There should be a space before and after a binary operator. 

  3. function _vidi_getColor($values, $id) {
  switch ($values) {
    case 'city_funky':
      $colors_values = array('yellow', 'green', 'light blue', 'orange', 'purple');
      break;
    case 'pure_classic':
      $colors_values = array('blue');
      break;
    case 'ecology_green':
      $colors_values = array('green');
      break;
    case 'clear_blue':
      $colors_values = array('light blue');
      break;
    case 'lavender':
      $colors_values = array('purple');
      break;
    case 'burgundy_red':
      $colors_values = array('red');
      break;
    case 'orange_sun':
      $colors_values = array('orange');
      break;
    case 'vidi_special':
      $colors_values = array('yellow', 'green', 'light blue', 'orange');
  }

  if (count($colors_values) != 0) {
    $colors = array_values($colors_values);
    $tmp = $id;
    while ($tmp >= count($colors)) {
      $tmp = $tmp - count($colors);
    }
    return $colors[$tmp];
  }
  else return 'red';
}

    The coding standards report that a function name should use lowercase characters.
    The curly parentheses in the control statement are not optional as per Drupal coding standards; the last statement should be written

    else {
  return 'red';
}
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Assigned: Unassigned » avpaderno

Status: Fixed » Closed (fixed)
Issue tags: -Module review

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

Issue tags: +Module review

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Component: Miscellaneous » new project application
Issue summary: View changes