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.
Comments
Comment #1
klausiWelcome,
please get a review bonus first. Then try to fix issues raised by automated review tools: http://ventral.org/pareview/httpgitdrupalorgsandboxniklan1785956git
Comment #2
NiklanI 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.
Comment #3
NiklanHello 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.
Comment #4
gnucifer CreditAttribution: gnucifer commented@Niklan: http://drupal.org/project/pareviewsh (Remember to use the 2.x-branch of coder).
Comment #5
hussainwebI will jump straight to the issues:
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.
Comment #6
hussainwebForgot to set it back to 'needs work'.
Comment #7
NiklanHi.
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
Comment #8
hussainwebI will jump straight to the issues again:
Comment #9
hussainwebI 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.
Comment #10
NiklanHi, 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.
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.
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
Comment #11
asgorobets CreditAttribution: asgorobets commentedHi 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:
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!
Comment #12
NiklanHi asgorobets,
All done.
Comment #13
monymirzaDid you miss
hook_uninstall
?Comment #14
NiklanHi 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.
Comment #15
logan.H CreditAttribution: logan.H commentedHi 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.
Comment #16
NiklanHi 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?
Comment #17
cloudbull CreditAttribution: cloudbull commentedThis 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
Comment #18
NiklanHey.
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.
Comment #19
heddnAutomated 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
(jQuery, Drupal, this, this.document)
notation for its initial function definition? Can't it just pass in(jQuery)
? See http://drupal.org/node/171213gb.install
gb.info
gb.admin.inc
gb.module
'#attributes' => array( 'class' => 'cities-fieldset-wrapper',),
or'#attributes' => array( 'id' => 'cities-fieldset-wrapper',),
instead of #prefix/#suffix.Comment #20
vladimir-m CreditAttribution: vladimir-m commentedHello 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/72. In function gb_form_block_admin_configure_alter_submit() instead
db_insert
anddb_update
you can usedrupal_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.
Comment #21
NiklanHi heddn and vladimir-m.
Thank you for your interest. Most of your requirements are met.
Comment #22
heddn@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.
Comment #23
NiklanFor comment #20
Comment #24
heddnComment #25
NiklanComment #26
heddnComment #27
NiklanAdded 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.
Comment #28
dimitrov.adrian CreditAttribution: dimitrov.adrian commentedI 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.
Comment #29
Niklandone
Comment #30
mnico CreditAttribution: mnico commentedhello
Manual Review
It is recommended that persistent variables have the module prefix to avoid collision
I see this persistent variables:
Comment #31
sreynen CreditAttribution: sreynen commentedComment #32
NiklanSix months later, his ever check?
Comment #33
kscheirerThe 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.
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 you fix the module namespace issue you'll get an RTBC from me.
----
Top Shelf Modules - Enterprise modules from the community for the community.
Comment #34
NiklanHello, kscheirer.
I do it.
http://ventral.org/pareview/httpgitdrupalorgsandboxniklan1785956git
Comment #35
kscheirerThanks!
----
Top Shelf Modules - Enterprise modules from the community for the community.
Comment #36
NiklanWhat i need to do next? I didnt see "promote" tab in the project :(
Comment #37
cweagansThanks 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.