[D7] Views Floorplan

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxlaboratorymike2359995git

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.

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.

The automated review has highlighted a few problems: http://pareview.sh/pareview/httpgitdrupalorgsandboxlaboratorymike2359995git

• ./views_floorplan.module: all functions should be prefixed with your module/theme name to avoid name clashes. See https://www.drupal.org/node/318#naming function _array_insert($array, $position, $insert) {
• 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.

I can do a manual review in the meantime

Initial code review:

• views_floorplan_views_post_render() - Consider moving variable_get_value() inside if ($view->style_plugin->plugin_name === 'floorplan') {, to prevent variable_get_value() being called on all views
• Consider using behaviours in js/floorplan.js instead of jQuery(document).ready() - see https://www.drupal.org/node/756722
• I think the README.txt needs to be updated with the new helper tool path - Currently says to go to /views_floorplan_point_tool

Will now try out the module functionality

Hi,

The module file contains an unused variable $message at line no.181.Delete it.
The drop down values in the views_floorplan_plugin_style_floorplan.inc file are not translatable.
also in your form alter function you have not checked the form_id.If you need to alter a particular form use hook_form_id_alter instead.

Its a nice module!

• Consider adding @param and @return for all your callbacks/your own functions as suggested in the documentation. see: https://www.drupal.org/coding-standards/docs
• You should pass the form options in t() in views_floorplan_plugin_style_floorplan.inc .
• Also consider fixing other tiny issues shown in http://pareview.sh/pareview/httpgitdrupalorgsandboxlaboratorymike2359995git

@laboratory.mike, I'm intrigued with this module. It's something I can see being extremely handy in some of the projects I support so I'll be happy to help out finalizing and reviewing it, if I can.

PAREVIEW

Since your latest commits PAReview is showing quite a number of formatting errors. I think many of the formatting errors were always there but the PAReview tool has been updated and as a result it is just that much more picky about spacing and indenting in comments. It's pretty simple stuff to fix, if only a bit tedious.

PAReview is also still complaining about the $message variable that isn't being used. That'll need to be used or purged from the code unless it is being used somewhere.

SETUP ISSUES

Now getting down to business... Initially I couldn't seem to get this thing to work properly and it's because I didn't set up my content types and fields before configuring the module so I think the instructions need to be updated to reflect the need to set up the content types then configure the module. Something like the following:

Basic Module Setup (instruction)
Once the module is enabled, create two content types.

• A 'floorplan' content type with a field for the image that represents the floorplan
• A 'floorplan point' content type that represents each point on the floorplan.
• The 'floorplan point' content type will have two 'decimal' text fields representing X and Y coordinates, plus an entity reference field with its Entity Selection Mode set to "Simple" and its Bundle set to the 'floorplan' content type that was created earlier.

Once the content types have beed created clear all caches and then navigate to the Views Floorplan Settings page (admin/config/user-interface/views_floorplan) to configure the module to use the fields and content types that you have created.

Note: If you do not see settings for the fields that you've created in your content types, clear all caches again and visit the Views Floorplan settings page again.

Once you have set the appropriate fields in the Views Floorplan Settings page, you should be able to create a Floorplan node with an image of the Floorplan you choose and then add points to it by creating Floorplan Points nodes. When select the Floorplan via the Entity Reference Field within a Floorplan Point, the Foorplan jQuery tool will pop up allowing you to set where the point will be positioned on the Floorplan image.

VIEWS ISSUES

Now that I know how to install and set up the content types and configure the module first time, every time, I was struggling with creating the view.

The README and setup instructions don't really tell the user what type of view they are creating and they don't mention that in the view Advanced Settings we need to add a relationship or a contextual filter to make it work. I suggest adding instructions to create a view.

Basic View Setup (instruction)
Once you have a Floorplan and Floorplan Points that reference the Floorplan, you can create a view of the Floorplan Points content type that will reference the Foorplan and overlay the points above the Floorplan image.

Note: The floorplan view display format allows you to decide what color the points will be displayed with and you can either set a single colour as determined by a 'color' text field on the 'Floorplan' content type or a variety of colors that can be set via a 'color' text field on the 'Floorplan Point' content type. Whichever method you choose, you will want to create a 'color' text field on one of the content types as set it with HEX codes representing the color of choice.

• Add a new View, Show: Content, of type: Floorplan Point, sorted by: unsorted
• Display format: Floorplan, of: fields
• Items to display: 0 (unlimited), uncheck Pager checkbox

A basic view will be set up with the title field exposed.

In the View editing tools, in the Advanced section add a Relationship of the type Entity Reference and choose the one that is of "A bridge to the Content entity that is referenced via the entity reference field that you are using in your Floorplan Point content type to reference the Floorplan.

Add the following fields and set them to "Exclude from display":

• X and Y coordinates
• The Floorplan image using the relationship provided via the entity reference field
• The color field mentioned in the note above. It will either be referenced from the Floorplan using the relationship or it will be from the Floorplan Point

Note: The Floorplan image will need to be configured to "rewrite the output" so that it provides the FID and not render the image. You can do that via the Rewrite Results option and selecting the Replacement Pattern for the 'Raw fid'

Next, navigate to the FORMAT -> Floorplan -> Settings in the View editing interface and set the Image FID, X-distance Field, Y-Distance Field and the Color Field. The Image FID field will likely have a very long name due to the entity reference relationship that eventually resolves to the Floorplan Image you set up. The rest should be rpetty straight forward.

Once the Floorplan Settings have beed net, you should see the Floorplan background image and the points that you have created to overlay over it. They may not be positioned as you expect hoever, once you look at the view within the site, it should be exactly as expected.

FEATURE REQUEST

To really make this module effective, it might be a good idea to have it create a Floorplan node type and a Floorplan Point type with a basic view. That would certainly speed up usage. I don't expect it would be a deal breaker providing the instructions are concise enough but it sure would make it easy to get up and running with. I'm happy to lend a hand with this module if you need it. I think it's really quite useful and I've yet to see a duplicate.

Cheers,
Andrew

Hi Mike,
Good plan regarding the Floorplan Example sub module. I've been playing around with it as time permits and once you know what you're doing, it's pretty simple to work with. I particularly like the way you can colour-key different points on a Floorplan by adding a colour field to your Floorplan Point nodes and then assigning the color attribute to it in the View display.

On a side-note
I've got a project percolating on the side for one of my clients that I think I'll be using with this and I think I can do everything I need to tailor it for my needs via overriding the JS and CSS files but I have a question regarding making it mobile friendly. Since it uses pixel coordinates for placing the points, it's not likely that it'll be easy to make a mobile friendly Floorplan view. Have you any ideas about how one would present a Floorplan for the small screen?

@laboratory.mike, I've been away on holidays and have to head out of town again tonight again but thought I would check in.

I'll look into the responsive ideas when I'm back. I thought about using !important in a custom stylesheet as well. I'll give it some more thought and if I come up with anything interesting, I'll let you know.

Regarding the PAReview, I've looked at the PAReview report and reviewed the changes you made. I'm not sure that anything can be done regarding the variable $translatables because if I'm not mistaken that will be used by views so you can't simply remove it but PAReview can't see where it will be used so it is throwing it up as an error.

Regarding the errors in formatting, I wonder if someone else can weigh in regarding the conflict between what Views needs in naming conventions and what PAReview demands for a clean review.

Hi laboratory.mike,

Some errors reported by the automated code review tool: http://pareview.sh/pareview/httpgitdrupalorgsandboxlaboratorymike2359995git

you should add a module configure link in *.info file, so that a user can quickly reach at module configuration page from modules list.
configure = admin/config/user-interface/views_floorplan

Waiting for review since October 2015.
Bumping to critical as per Review process for Full Project Applications: What to Expect

Critical: Applications with a status of needs review that have been awaiting response from a reviewer for 4+ weeks.

This looks like it's been pretty thoroughly reviewed, with many rounds of revision, so hopefully it's actually close to RTBC now

@laboratory.mike - if you could review some other projects to pick up the PAReview bonus while we get more reviewer eyeballs on this - hopefully we can get this approved quickly for you.

Info on the review bonus project is here: https://www.drupal.org/node/1975228

I'd like to see this get pushed forward too. It's an amazing concept and module. I reviewed it over 9 months ago so I'll give it another review and hopefully we can get this one pushed through to acceptance.

Hi laboratory.mike,

Interesting module !

Automated review showed some errors and warnings, please correct where possible.
http://pareview.sh/pareview/httpgitdrupalorgsandboxlaboratorymike2359995git

Manual review:
- worth considering moving custom markup into separate theme functions
- get rid of dpm() function (this was highlighted just as a warning in automated review, but might cause a fatal error if devel module is not enabled)
- you might want to consider using '#attached' to add the js to the element (where possible) instead of drupal_add_js

hello!

your JS files are not properly wrapped

(function ($) {
'use strict';
Drupal.behaviors.myname = {
  attach: function (context, settings) {
 // do stuff
  }
};
})(jQuery, Drupal);

in your views_floorplan_node_ajax() function. it would be nicer to render your image through a theme function: theme_image().

you do need to check over your dependencies (.info) -- you are using node_load(),

Hi laboratory.mike,

you should consider to implement theme functions for generating the markup that is currentls generated in views_floorplan_form_alter(), views_floorplan_node_ajax() and your views style plugin. This would avoid code dublication and make the code more readable and extendable.

You should move the views_floorplan_plugin_style_floorplan.inc file to a views/plugins subdirectory of your module directory.

Best regards

I think my suggestions are not application blockes.

Sorry for the delay. Make sure to review more project applications and get a review bonus and this will get finished faster.

manual review:

1. views_floorplan_form_alter(): do not call drupal_add_js() in a form alter hook, use #attached instead. See https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h...
2. views_floorplan_node_ajax(): "Enter a floorplan map with a valid image to begin.": all user facing text must run through t() for translation. Please check all your strings.
3. There is a dpm() call in teh code, so this will throw a fatal error on production sites.
4. ViewsFloorplanPluginStyleFloorplan::createLegend(): looks vulnerable to XSS exploits. $label is directly going to HTML markup without being sanitized. I tried to test this on simplytest.me, but of course the dpm() call threw a fatal error and I could not verify the vulnerability. Can you fix the dpm() call, then read https://www.drupal.org/node/28984 and test that all output is already sanitized when printed to HTML?

Hello laboratory.mike,

My findings for your modue as follows:

views_floorplan.module

Line 179: Use "elseif" in place of "else if"

else if (module_exists('entityreference_prepopulate') && isset($_GET[$eref_field])) {

Thank you,

Interesting module!

/**
 * Implements hook_variable_info().
 */
function views_floorplan_variable_info($options) {

This should go in an inc file.

+1 for declaring these.

Though... I think a lot of these should be moved to the Views style plugin. Eg, the fields.

  $variable['views_floorplan_css'] = array(
    'title' => t('Path to CSS for floorplan maps', array(), $options),
    'description' => t('If you would like to use custom CSS for your floorplan map, enter the internal Drupal path here. The default is @module/css/floorplan-style.css', array('@module' => drupal_get_path('module', 'views_floorplan')), $options),
    'type' => 'string',
    'default' => drupal_get_path('module', 'views_floorplan') . '/css/floorplan-style.css',
  );

This is overkill. Just provide your CSS in the module. Site builders can override it in their theme if they want to.

      $legend .= '<table class="floorplan-legend ' .
      $view_options['optional_fields']['legend_category_attach'] . '">
      <tr>
      <th class="floorplan-legend-color">Color</th>
      <th class="floorplan-legend-category">Category</th>
      </tr>';

All HTML code should be in theme templates or functions. Use theme_table() here maybe?

        dpm($color);

Stray debug.

    'description' => ' Administer views floorplan settings <em>Give to trusted roles only; this permission has security implications</em>',

There's a way to do that warning automatically in the hook.

function views_floorplan_views_post_render(&$view, &$output, &$cache) {
  $js = variable_get_value('views_floorplan_js');
  $css = variable_get_value('views_floorplan_css');
  if (isset($view->style_plugin) && $view->style_plugin->plugin_name === 'floorplan') {

You control the plugin! No need for this -- do it in the plugin code.

  if (count(array_intersect($field_array, array_keys($form))) === count($field_array) && !in_array('none', $field_array)) {

If you're detecting the presence of fields, there are better ways.

Also, I hope you've not hardcoded to nodes... ;)

      $node = node_load($eref_val);

Oh you have :( On D7 it's best to do things for generic entities, not just nodes, if at all reasonable.

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.