Block visibility switcher

Semicolon is missing in block_visibility_switcher.module line number 13.

->condition('bid', $block_id, '=') 

change this to

->condition('bid', $block_id) 

Do not add version in .info file.

block_visibility_switcher.css must end in a single new line character. Similarly in js file.

It looks like you have files other than README.txt in the master branch, you should remove those.

Code sniffer only found one issue that you may want to fix. You have one function "toggle_block()" that should be renamed to include the name of your module.

http://ventral.org/pareview/httpgitdrupalorgsandboxtimusan1659156git

Cool idea and works as advertised.

Hi,
your module conatins only 4 functions and less then 120 lines of code (CSS and tpl files not counted). I don't think it will satisfy condition 2.3 on the project application checklist.

General issues:
- Please test your module after applying changes
- The close button conflicts with the contextual links button (at least in theme seven)

module file:
- Line 1: contains a "G" before the php tag, it gets printed on every page
- Line 40: You didn't update the callback function after the last changes
- I think this isn't a toogle function, you are just disabling the block, never enabeling, therefor you also don't need the switch variable, as it's always 0 anyway
- POST data is not getting sanitized at all, when changing your JS call to &switch=whatever it will write whatever to the DB. Very critical! I'm not 100% sure if Drupal automatically sanitzies data in dynamic queries (propbaly does), but I would wrap an intval() around the POST block_id anyway.
- Line 69: Check if the query was successful at all, otherwise empty entries end up in the DB (for example with JS call 'block_id=whatever')

js file:
- Line 8: dataType describes the data format that's being expected from the server. So far no data is being returned and jQuery detects the format automatically (but of course you can leave it if you really sent JSON data)
- Line 9: The success callback is wrong. You are missing an 's' and the call itself is wrong, it should look like this
success: function(){$(this).parents('div.block').fadeOut()},
What's happening now is that fadeOut() is being called no matter what, which is why it seems to work. You should pass a state back from the callback to the JS success function (e.g. with drupal_json_output) that actually reflects if the call did what it was supposed to, 'success' just tells you that the server didn't throw any errors.
- Line 10: I know you can pass POST data as strings, but I would highly recommend using a map like
data: {block_id: $(this).attr('id'), switch: 0}

Hi Timusan,

I checked the updates:

1. pareview shows 3 errors
2. produce JSON output anyway. Even if the query returns no result the callback itself is still considered to be successful, therefore the block will be hidden and reappear on next refresh. A way would be to exit, but that's not clean (drupal needs to shutdown), so add something like this:
   .module line 69

     $json = array('success' => false);
     if (!empty($result)) {
       .....
       $json['success'] = true;
     }
     return drupal_json_output($json);
   

   .js line 10

   success: function(data, textStatus) {
    if(data.success != undefined && data.success == true){
      block.parents('div.block').fadeOut()
    }
   },
   

3. Line 58: I would check if $_POST['block_id'] is set at all, intval() will return 0 anyway, but I don't think its a clean solution
4. Line 54: Load the user after line 70, its not needed before that and you might not need to load it at all if the block_id is wrong
5. In hook_init, check if the user is authenticated, right now you always add the resources, no matter what
6. I might even add a specific permission, then you can choose which role should be able to use this
7. I'm amazed Drupal coding style dosen't say anything about inline variables in strings, but I would call "$my_path/block_visibility_switcher.css" bad practise, but just my opinion
8. I would make a css sprite for the buttons

Hope this helps

Sorry, am always forgetting to switch the status.

Review of the 7.x-1.x branch:

• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  
  FILE: ...l/modules/pareview_temp/test_candidate/block_visibility_switcher.module
  --------------------------------------------------------------------------------
  FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   64 | ERROR | else must start on a new line
  --------------------------------------------------------------------------------
  FILE: .../modules/pareview_temp/test_candidate/block_visibility_switcher.tpl.php
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   4 | WARNING | Line exceeds 80 characters; contains 97 characters
  --------------------------------------------------------------------------------
  

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. There is no code to add the toggle link to block titles? How should this module work, or am I missing something?
2. block_visibility_switcher_toggle_block(): this should only be executed for logged in users. It will probably cause errors when triggered as anomyous user.
3. It would be nice to have a permission to indicate which role is allowed to use that functionality.

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

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

Added anther project I reviewed.