Ubercart Global Quote

Well, this module really helped in my project,
thanks to the author of this module that has geared up my project.

Project looks to have had bugs fixed and git repository is in good use. Looks pretty much like it's ready after a quick cleanup. Perhaps patching the master branch with a README to explain where to get the real code, ie branch 6.x-1.x or 7.x-1.x

Tips for ensuring a smooth review
Your code comes with a license file, or contains comments that report the license used for the code.
All the projects hosted on Drupal.org are licensed under the same license used from Drupal, which is GPL.

LICENSE.txt is automatically added by the Drupal.org packaging system on every project, since everything on Drupal.org has the same license. It should not be included in your repository.

needs documentation of install and configuration Tips for a great project page

missing doxygen comments in .install hook_install/hook_uninstall .. no space between comment and function declaration. ditto in uc_shipping_zones. Update function is missing it took which is important because without that, the update description when running update.php is empty!

missing @file block on most files

no EOL on README

"TODO: Clean that" .. also has commented out code sitting around (that's what a VCS is for - delete it)

comment on uc_global_quote_quote shouldn't that be 'Implements hook_quote().' ? (blank line)

line: 110 of uc_global_quote.module - you can use single quotes on strings that don't need expansion since it speeds up parsing and execution. Same thing in sub-module.

No need to use t() with l() l(t('edit'), 'admin/store/settings/quotes/methods/zones/' . $zid);There are a few other occurrences as well. You also use string concatenation with t() which is not good for translating - use parameters instead.

In uc_shipping_zones_regions_ajax, strange SQL You're doing a very long OR zone_country_id = %d add on when you could just use WHERE zone_country_id IN (N, N, N). Also means the db_query can be cleaned up to only use parameters instead of string concatenation.

long lines A lot of the super long long lines in uc_global_quote_admin.inc can be wrapped to make them easier to see the structure and what they're doing. Sorry .. that's just be me being pedantic.

array indexes need guards on not existing you use array indexes in uc_global_quote_uc_shipping_method without ever checking the index exists when it does have the chance of not existing.

D7
.install Remove unused hook_uninstall(). Add a simple drupal_set_message() 'Global Quote installed ... ' message to hook_install() so that the user knows it all went well.

commented out code lines

in uc_global_quote_quote You should pull the variable_get to outside the loop so it isn't called on each loop. More TODO. More commented out code lines. Using dprint_r in watchdog for EVERY quote?

Parameterised db_query should perhaps be converted to use db_select

As I said at the start, apart from those few things, it looks pretty good. I haven't run the code but considering it's published offsite, in use already, and includes patches, I'd assume that it's working for people.

Looks good. I should probably have clarified that the empty line between the doxygen block and function was not desired, instead of the rather ambiguous 'no space between comment and function declaration' which can be taken either way!

In future when doing commits, it's often better to split things into logical blocks that can be adequately described instead of everything in one big blob. That way someone looking at the commit log can more clearly see the specific logical change, especially if there are many related changes over many files.

eg. In this last case, it could potentially be divided into some of the following, all of which would be relatively small and related commits.

• adding doxygen blocks
• fixing SQL
• code cleanup of unused code
• etc.

On the other hand if you decided to add some major feature that required changes to almost every file, it wouldn't be split into multiple commits, it would be the one large logical commit that added that feature in one go. Well, actually, you'd probably branch off the current state, do lots and lots of changes and commits (potentially switch back to your original branch to fix something else and merging that in when you switch back), and then when the feature is done squish the commits together (there's a git command for that) and then commit a larger patch/merge back to the main branch. All the while you have kept your commit history on the side branch which you might not even push, but the main branch hasn't been polluted with a zillion one liners but instead a well documented large commit.

Anyway, I think you have met the requirements so I'm setting it to RTBC. No point having this one sitting here. Let's hope someone more powerful than I agrees. Continue to fix up stuff and work on it (D7 is still in dev?)

I created an issue at #1301472: various code cleanup issues for uc global quote so it can be tracked. It has several suggestions, but none of those items are blockers for a project application.

Thanks for your contribution, joanpc! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.