This module creates a specified number of blocks. The administrative panel has an interface to configure data blocks. Each block has fields for title, name of the town, as well as the contents of the block. Each block has the ability to add an unlimited number of "cities". For each city indicated its title and content. Once a user visits the site, the module defines its city through free services API for Geolocation ip. Based on the findings of the city, in a block, this information is issued for the city. If such information is not present, then the value is displayed by default, and if it is not present, then the block does not display. Also in the settings you can enable options to change the city. One way - there is a strip at the top of the site, where you can change the city. Method 2 - Set custom code anywhere in the page, which will be inserted code to change the city.

Project URL: http://drupal.org/sandbox/Niklan/1785956
GIT: http://git.drupal.org/sandbox/Niklan/1785956.git
For Drupal 7.

CommentFileSizeAuthor
#16 1.jpg47.72 KBNiklan
#10 gb1.png17.58 KBNiklan
#10 gb2.png5.45 KBNiklan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

klausi’s picture

Status: Needs review » Needs work

Welcome,

please get a review bonus first. Then try to fix issues raised by automated review tools: http://ventral.org/pareview/httpgitdrupalorgsandboxniklan1785956git

Niklan’s picture

I apologize, but I can not participate in the bonus program, at least because I myself just do the first module and not experienced enough to check other people's work.

As for the on-line test. There, most of the error - a problem with spaces, which are not there, the service is accordingly not objective enough to test the code. Coder did not find any problems.

Niklan’s picture

Status: Needs work » Needs review

Hello again, klausi.

Fix all errors and warnings.
http://ventral.org/pareview/httpgitdrupalorgsandboxniklan1785956git

Is there a similar service for local checks are not using git? Coder and other modules do not analyze much detail.

gnucifer’s picture

@Niklan: http://drupal.org/project/pareviewsh (Remember to use the 2.x-branch of coder).

hussainweb’s picture

I will jump straight to the issues:

  1. Based on the functionality, it would seem that this module would do better to add on the functionality to existing block system, rather than create a new parallel block system.
  2. gb_settings_form.inc:141 - You are calling an undefined function - krumo(). This does not allow me to save the module settings form to test further.
  3. Some of the files are set as executable. Please remove the executable bit.
  4. gb_settings_form.inc:75 - The description is not clear.
  5. The module seems to store a lot of variables - 3 per block and 4 per city per block. A database is a better option.
  6. I get a warning when I go to Configuration > User Interface > Geo Block - Warning: Invalid argument supplied for foreach() in theme_table() (line 1989 of /home/hw/www/test/gbtest/includes/theme.inc).

I would emphasize mostly on using the existing block system and add on the overrides depending on the city, and also to use the database to store the settings. However, if you tie into the existing block system, you would not have to use so many variables.

hussainweb’s picture

Status: Needs review » Needs work

Forgot to set it back to 'needs work'.

Niklan’s picture

Status: Needs work » Needs review

Hi.

All requests are made. The module is now implemented in the block system Drupal.

How to:
1. Visit block management admin/structure/block
2. Click the "Add geo block" and fill in all required fields.

Configuring the module is available: admin/structure/block/gb_tsonfigure

PAReview: http://ventral.org/pareview/httpgitdrupalorgsandboxniklan1785956git

hussainweb’s picture

Status: Needs review » Needs work

I will jump straight to the issues again:

  • On a new site with no blocks defined, there is a notice: Notice: Undefined variable: blocks in gb_page_build() (line 222 of /home/hw/www/test/gbtest/sites/all/modules/geo_block/gb.module). Looking at the code in that section, I see that you are transferring the query result to an array and then looping over it. I would recommend either using something like fetchAllAssoc or directly looping with the database result using relevant methods to get the count. I think the first method would be more flexible based on your logic.
  • Your configuration path does not show up anywhere in Drupal menus. I would recommend putting that back under any section in admin/config.
  • Your hook_form_alter can as well be the specific to the particular form ID. See hook_form_FORM_ID_alter
  • I see my earlier request to put it under block system was slightly misunderstood. What I meant was to implement this functionality as something like visibility settings for existing blocks. That way, users can use the Geo features immediately for existing custom blocks or blocks from views or other modules. I am not sure if you have any reasons to separate the block system but I would prefer this to be a part of visibility settings. I can't think of another module that does this right now but I think mobile_switch_blocks is close.
hussainweb’s picture

I just spotted another issue on gb.install. You don't need to drop tables you create via hook_schema. Uninstall removes those tables. See hook_schema.

Niklan’s picture

Status: Needs work » Needs review
FileSize
5.45 KB
17.58 KB

Hi, hussainweb, and thank you for helping me.

All the items are made.

Now setting sewn directly into system block, but only available for standard block (and user-created with standart method) that it did not cause any problems with the blocks of third-party modules.

geo block settings
I found that the section of visibility to be a great place for geo settings. This preserves the compact shape and still has to do with visibility.

gb config
Attach files with the screen, including a reference to the module configuration. I've got it as it was, and remains.

PAReview: http://ventral.org/pareview/httpgitdrupalorgsandboxniklan1785956git

asgorobets’s picture

Status: Needs review » Needs work

Hi Niklan,

Thank you for your contribution to this project. I have some recommendations you can consider:

1. It looks like the only purpose of gb_module_settings_form is to set settings variables. You can use system_settings_form() for this purpose. Please take a look at system_settings_form example. You can even use '#tree' => TRUE on your fieldset to save fieldset values as a serialized array.

2. Line 290: Looks like you forgot to clean up this:

<?php
  return $data;
  return $block;
?>

3. I reviewed your gb_city_select.js file and it looks like you are not using Drupal behaviors. It's a recommandation in Drupal community.
You can check out this section of Javascript API for more info.

4. Line 264 says:
Implements hook_form_block_custom_block_delete_alter_submit().
It's not actually a hook, but a submit callback. And in general naming convention of forms and related callbacks are confusing. You can't have a submit for a form_alter. Usually it's FORMID_submit() or FORMID_something_submit().

Cheers!

Niklan’s picture

Status: Needs work » Needs review

Hi asgorobets,

All done.

monymirza’s picture

Did you miss hook_uninstall ?

Niklan’s picture

Hi monymirza,

Okay, that was my mistake to remove it. I thought that if we keep the value of a system function, it is as well as to the base controls remote data delete module. Now checked, nothing is removed by itself. Add all that is necessary to remove. Now all works fine.

logan.H’s picture

Hi Niklan,

1) Can you Add "type" to the menu $items['admin/structure/block/gb_configure'] to make the admin configuration menu available in the block configuration page or in site configuration page, right now its not available anywhere.

2) I tried install the module and got the following notices

Notice: Undefined index: ip in gb_get_user_city_ipgeobase() (line 376 of C:\xampp\htdocs\drupal7_test\sites\all\modules\contrib\gb\gb.module).
Notice: Trying to get property of non-object in gb_get_user_city_ipgeobase() (line 376 of C:\xampp\htdocs\drupal7_test\sites\all\modules\contrib\gb\gb.module).
Look like if the ip is not retrieved form ip_address we get this notice.

3) It would be better if curl_error condition is handled and log the error. It will help in case if the service is
not available for some reason.

Niklan’s picture

FileSize
47.72 KB

Hi logan.H,

1) link avialable. Look screenshot http://drupal.org/files/1_54.jpg
2) I have no such problems and I do not understand where it came from you. Are you sure you use the latest version of the module?

cloudbull’s picture

This is good idea of geoIP feature. Really helpful.

Just a little bit comment.

1) I cannot find admin/structure/block/gb_configure easily without directly enter the path.
I know we can find it at admin/index, but if we are site admin, we wont click into it, we will usually go admin/structure/block or admin/structure/block or /admin/config

2) It will be great if blocks like main menu / user menu also supported

3) at block geo setting section, it is great to have geo-autocomplete or map etc. Since I don't know if my input of the city name will be exactly match with the one API suggested. Or we can put it as geoInformation instead of name.

What do you think ?

Keith

Niklan’s picture

Hey.

1) I do not think this is an important point, because these settings are not used often enough. Essentially you are setting them once and all. Also, from the admin menu is very easy to get into the settings.

2) The module is designed for text information. At the moment it is quite difficult to implement. Maybe in the future I will modify that opportunity. The idea is interesting.

3) It is impossible, for one simple reason. All use different geo-services that have different requirements. Also, all entering town in their own language rather than in English. For example, I am from Russia and I enter the name of their cities in Cyrillic. Same thing with the other countries. I think autocomplete cities with the service and the country will be enough heavyweight addition to the module. But again, back down and say that everything is possible, but not at the time of release of the module.

Thank you very much for attention and ideas. I already thought that this check was lost in the other, and never will recover.

heddn’s picture

Automated Review

FILE: /var/www/drupal-7-pareview/pareview_temp/gb_city_select.js
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
84 | ERROR | Functions must not contain multiple empty lines in a row; found 2
| | empty lines

Manual review

gb_city_select.js

  • Why is the js not using jquery.cookie.js?
  • Why does the js use (jQuery, Drupal, this, this.document) notation for its initial function definition? Can't it just pass in (jQuery)? See http://drupal.org/node/171213

gb.install

  • Instead of implementing your own table, have you considered using block_custom? Or at least use the schema for it as the base for gb.module's block implementation? It looks almost identical, except there isn't a means in your block schema to set a fitler format and it uses info instead of delta to make the table row unique.

gb.info

  • I feel the description should be shorter as it scrolls off the side of my screen. Perhaps "Geo location blocks"

gb.admin.inc

  • Line 24: nit - Better wording: "What geo-service will be used to determine the location of the visitor?"
  • Line 36: The description doesn't make sense to me. Is it intended to read: "This section displays various options to change the city for a user. It is necessary for cases when the city was not defined correctly.
  • Line 45: Is this intended to read: "Toolbar on the top of the site."
  • Line 59: Is this intended to read: "If checked, the cookie will be set after the close of the selection box and it won't display again."
  • Line 65: Perhaps insert color code through $args so it doesn't get passed through translation.
  • Line 72-74: I'm not an expert with these things but I'm not sure that embedded html in a t() call is a good idea. Perhaps pull these out and insert using $args array().

gb.module

  • What about moving the hook menu to settings instead of structure? Considering the title of the menu item, that would make more sense.
  • Line 49-50: Perhaps use '#attributes' => array( 'class' => 'cities-fieldset-wrapper',), or '#attributes' => array( 'id' => 'cities-fieldset-wrapper',),instead of #prefix/#suffix.
  • Line 108: Use db_query instead of db_select. It has better performance.
  • Line 114: You can probably just check for FALSE since the return from fetchObject is false if no results found.
  • Line 139-140: See comment earlier about #prefix/#suffix.
  • Line 217: Can you move the rebuild inside of the if statement? Is there any need to rebuild the form if == 1?
  • Line 224: Use db_query.
  • Line 237: Check for !FALSE instead of empty is appropriate given the interface of fetchObject().
  • Line 367-372: Is there any reason why you can't use drupal_http_request()?
  • Line 387-392: Is there any reason why you can't use drupal_http_request()?
  • Line 404: Can't you use db_query()?
  • Line 426-433: Can you use #attached to add these on the blocks rather than implementing hook_page_build()? See http://api.drupal.org/api/drupal/developer!topics!forms_api_reference.ht...
vladimir-m’s picture

Status: Needs review » Needs work

Hello Niklan,

Thank you for great module.

1. In file: gb.module at line: 58, 148 insted check_plain("City {$i}") you can use t() with @placeholder http://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/t/7
2. In function gb_form_block_admin_configure_alter_submit() instead db_insert and db_update you can use drupal_write_record() http://api.drupal.org/api/drupal/includes%21common.inc/function/drupal_write_record/7 and you get schema validation for free.
3. Please provide in info file configure = admin/structure/block/gb_configure
4. Please provide more information (in README file and/or using hook_help().) how to configure this module, how o access configuration page.

Niklan’s picture

Status: Needs work » Needs review

Hi heddn and vladimir-m.
Thank you for your interest. Most of your requirements are met.

heddn’s picture

@Niklan: can you summarize the changes you performed in #21 and which ones you didn't perform? That would help with further reviews since there are almost certainly things you didn't perform for various reasons and I don't want to rehash the same feedback when there is good reason for those omissions.

Niklan’s picture

  1. gb_city_select.js
    • - I see no reason to use an additional library for cookies.
    • +
  2. gb.install
    • - This is just in case the module will need a new functionality.
  3. gb.info
    • +
  4. gb.admin.inc
    • +
    • +
    • +
    • +
    • +
    • - Many modules do just that, if this principle, it is possible to make and arguments.
  5. gb.module
    • - If I bring the item in the settings. That link also appears in the toolbar, which is not good.
    • +
    • +
    • - I did not realize that it returns FALSE. Ie I'm in a variable makes a request, and if he does not return values​​, the variable receives FALSE?
    • +
    • - This function is called only on click, and therefore it should perform rebuild. Why did they put them in a condition?
    • +
    • - I tried the service returns empty results.
    • -
    • +
    • - This refers to the load block was created?

For comment #20

  1. +
  2. +
  3. +
  4. +
heddn’s picture

Status: Needs review » Needs work
  • gb_city_select.js & jquery.cookie.js: Why write a new solution when one is already built into Drupal and other developers are familiar with it? Using your own solution makes it more difficult for others to also contribute to the module.
  • gb.install - good reasoning
  • It took me a while to find it, but t() shouldn't include html tags - http://drupal.org/node/322774
  • gb.module - hook_menu: This really should be in the settings section of the site. Please reconsider moving it. With the correct configuration it should not exposed in the toolbar.
  • gb.module - Line 211: Using that logic about click, then why even have the if statement?
  • gb.module - Use drupal_http_request(). It should work and not every site has curl available to them.
  • gb.module - Using #attached on the block is the preferred approach. Maybe add it in gb_block_generate_data().
Niklan’s picture

Status: Needs work » Needs review
  • done
  •  
  • done
  • I do not understand exactly in which menu you would like me to add a link?
  • done
  • done
  • As I understand it, this is used for forms. What method I will use it in a moment of generate block. Further, what is the difference from the current method? The fact that the page can be generated in two blocks, and you will be added to the method of double-js files and transfer parameters.
heddn’s picture

  • gb.module - hook_menu: You'll need to add the intermediary menu item, if it doesn't exist. That will hide it from showing up directly under config.
  • gb.module - #attached: Instead of form, think of it as a render array - https://drupal.org/node/930760
Niklan’s picture

Added to the menu admin/structure/gb_configure
As for #attached. I never understood the point. Now to generate a page samples and collection of all cities. If all this is to move in a moment of the block, in the presence of two or more units on a page will be made the next generation that will increase the download time.

dimitrov.adrian’s picture

Status: Needs review » Needs work

I think that the styling have to be exported to external CSS file instead of styling with a JS from the js file. Will be more flexible if you add css file, so themers can override it.

Niklan’s picture

Status: Needs work » Needs review

done

mnico’s picture

hello

Manual Review
It is recommended that persistent variables have the module prefix to avoid collision
I see this persistent variables:

  • gb_geo_service
  • top_toolbar_bg_color
  • top_toolbar_enabled
  • top_toolbar_use_cookie
  • placeholder_enabled
sreynen’s picture

Title: Geo Block » [D7] Geo Block
Niklan’s picture

Six months later, his ever check?

kscheirer’s picture

Status: Needs review » Needs work

The name of your module and the shortname should match - so if the name is geo_block, then all your module files should be geo_block.module, geo_block.info, etc. This affects all your function names as well - geo_block_schema(), geo_block_uninstall(), etc.

Your variable names should use the same namespace as well - these should all start with geo_block.

  variable_del('gb_geo_service');
  variable_del('top_toolbar_bg_color');
  variable_del('top_toolbar_enabled');
  variable_del('top_toolbar_use_cookie');
  variable_del('placeholder_enabled');

What is this default value? function gb_get_user_city($geo_service = '', $ip = '46.146.113.162') {? That should probably be 127.0.0.1 or 0.0.0.0. Looks like somewhere in Perm (http://iplocation.truevue.org/46.146.113.62.html)? Also, by setting a default value for the argument, this block of code will never get executed:

  if (empty($ip)) {
    $ip = ip_address();
  }

If you fix the module namespace issue you'll get an RTBC from me.

----
Top Shelf Modules - Enterprise modules from the community for the community.

Niklan’s picture

kscheirer’s picture

Status: Needs work » Reviewed & tested by the community

Thanks!

----
Top Shelf Modules - Enterprise modules from the community for the community.

Niklan’s picture

What i need to do next? I didnt see "promote" tab in the project :(

cweagans’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for your contribution, Niklan!

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:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

Status: Fixed » Closed (fixed)

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