I've been tinkering with this module, working on my Quotes by Item count and Countries version, and found several general things that can use fixing. Forgive me for not providing a patch, I'm unfamiliar with creating them.

1. The admin settings validation checks that Max is not smaller than Min, but the actual submit function only submits the data if Max is greater than Min. For quotes where Max equals Min (which I obviously tried for my item count method), success is reported but no action is done.

Change validation test from:

$form_state['values']['uc_tablequote_maximum'] < $form_state['values']['uc_tablequote_minimum']

To:

$form_state['values']['uc_tablequote_maximum'] && $form_state['values']['uc_tablequote_maximum'] <= $form_state['values']['uc_tablequote_minimum']

It requires the extra check (that Max has value), otherwise the default 0 and 0 (indicating no item is to be added) fails validation.

* * *

2. The module supports 0 shipping rates and even parses it to "Free" on the settings page. But in the hook_shipping_method callback function, if the final rate is 0, the method returns no suitable shipping methods.

Change uc_tablequote_quote like this (minus marks remove, plus marks add):

  $rate = 0;
+  $found = FALSE;
...
  while ($r = db_fetch_object($result)) {
    if (($total >= $r->min) && ($total < $r->max)) {
      $rate = $r->rate;
+      $found = TRUE;
    }
  }
...
-  if($rate > 0) 
+  if ($found) {
    $quotes[] = array('rate' => $rate, 'format' => uc_currency_format($rate), 'option_label' => $method['tablequote']['quote']['accessorials'][0]);
  }

* * *

3. In admin settings, when marking something for delete and adding new quote in the same go, the validation for overlap should skip checking the items that are about to be deleted. Didn't have time to implement this yet.

* * *

4. Not exactly bug, but very redundant DB call in theme_uc_tablequote_admin_settings: no need to count rows separately from preparing the table.

Remove this:

  $num_rows = db_result(db_query('SELECT COUNT(*) FROM {uc_tablequote} ORDER BY min'));
  if ($num_rows == 0) {
    $rows[] = array(array('data' => t('No rates found.'), 'colspan' => 4));
  }

And instead use:

  $result = db_query("SELECT * FROM {uc_tablequote} ORDER BY min");

  while ($r = db_fetch_object($result)) {
    $row = array();
    $qid = $r->qid;
    $r->rate > 0 ? $rate = $r->rate : $rate = 'Free';
    $row[] = drupal_render($form['rates'][$qid]['delete']);
    $row[] = $r->min;
    $row[] = $r->max;
    $row[] = $rate;
    $rows[] = $row;
  }
  
  if (count($rows)) {
    $rows[] = array(array('data' => t('Shipping quotes based on: ').variable_get('uc_tablequote_type','weight'), 'colspan' => 4));
  }
  else {
    $rows[] = array(array('data' => t('No rates found.'), 'colspan' => 4));
  }

* * *

5. The uc_tablequote_quote function doesn't take into account shippability of items, and also uses variable_get a dozen times. Cleaning it up a little:

function uc_tablequote_quote($products, $details){
  $total = 0;
  $type = variable_get('uc_tablequote_type','weight');

  foreach ($products as $product) {
    if($product->shippable) {

      switch($type) {
        
        case 'weight':
          $total += $product->qty * $product->weight * uc_weight_conversion($product->weight_units, variable_get('uc_weight_unit', 'lb'));
          break;
        case 'order':
          $total += $product->price * $product->qty;
          break;
        case 'items':
          $total += $product->qty;
          break;
      }
    }
  }
  ...

And that's it for now. Thoughts, anyone? (Patch, anyone? :))

CommentFileSizeAuthor
#19 new_patch.patch16.81 KBvalante
#17 patch.patch17.46 KBvalante
#11 last_patch.patch4.76 KBvalante
#10 patch.patch21.29 KBvalante
#10 uc_tablequote_new.txt10.59 KBvalante
#8 another_fix.patch21.37 KBvalante
#8 all_fixes.patch21.29 KBvalante
#7 fixed_fixes.patch21.03 KBvalante
#5 fixes.patch3.58 KBvalante

Comments

anrikun’s picture

anrikun commented

I've released a dev version of the module yesterday.
Please can you apply your changes to this dev version instead of 6.x-2.0
I will review them then and commit them.
Thank you for your contribution!

valante’s picture

valante commented

Hi anrikun, good to know this module is alive and kicking. I've only started working with it yesterday, took an entire day to debug and to modify it to support countries, and am running it now on my live site.

I would love to submit patches if I knew how to make them. On Linux, easy (or so I've heard). On Windows--any idea?

Otherwise the best I can do is post code here.

anrikun’s picture

anrikun commented

Please apply your changes to "6.x-2.x-dev",
Be sure that your changes don't break previous fixes (read http://drupal.org/node/743050#comment-2836530)
then post the code or attach a zip :-)

valante’s picture

valante commented
Status: Active » Needs review

OMG, look! I've made a patch! (for the record, I /hate/ cgywin.)

valante’s picture

valante commented
StatusFileSize
new fixes.patch3.58 KB

It will help if the attached file shows up...

anrikun’s picture

anrikun commented

Nice, thank you :-)!!!

But... :-(
There's a syntax error in uc_tablequote_quote($products, $details)

+        case 'weight:

A single quote is missing.

Please make sure the dev version + your changes is working correctly.

Instead of using cygwin, you should download the GnuWin32 version of patch, so easier to use:
http://gnuwin32.sourceforge.net/packages/patch.htm
Read this too: http://drupal.org/node/99903
I'm sure you'll love to make patches then :-)

valante’s picture

valante commented
StatusFileSize
new fixed_fixes.patch21.03 KB

Oops, the excitement!

Thanks for the GnuWin32 tip. I admit cgywin is not so bad either, once you have it figured out. Yes, I've been a Windows user all my life, guilty as charged. Shoot me later :)

valante’s picture

valante commented
StatusFileSize
new all_fixes.patch21.29 KB
new another_fix.patch21.37 KB

While working some more on it, discovered another major efficiency issue in the settings page. First, there's no need to count rows just to know if there's an existing type. Second, there's definitely no need to loop over every row and rewrite the type variable (with the same type, supposedly) over and over again.

Also:

* Where type already exists, I added the closing p tag in the Type of calculation line.
* Added a warning regarding more than 1 existing type. Should never happen, but. . . Is there something better than a warning to do here?

(See, I'm having fun.)

The "another fix" works against the previous patch. The "all fixes" works against the original dev version.

anrikun’s picture

anrikun commented

I still cannot apply the patch.
Please can you post the final+full uc_tablequote.module file?

valante’s picture

valante commented
StatusFileSize
new uc_tablequote_new.txt10.59 KB
new patch.patch21.29 KB

Hmm, that's exasperating. I've double-checked everything downloading a new dev version, applying each patch I made, and uploading the result to my test server for a functionality test.

I'm attaching the all-in-one patch again, from the current dev version, and the module file (changed extension to txt so I can upload it).

I'd appreciate if you could tell me what I've done wrong.

Got it. For some reason, whenever I open a module file with notepad it opens without line breaks. When I open it somewhere else, it does have line breaks, and when I save it it somehow changes the entire file. This new patch for example rewrites the entire module file, for line breaks and for my actual changes.

Any ideas on how to get around this?

Solved with Notepad++. Attaching new patch below (rolled against dev and tested on server . . . cross my fingers this will work).

valante’s picture

valante commented
StatusFileSize
new last_patch.patch4.76 KB

Last attempt. Please let me know if this works, and sorry for the multiple bother.

anrikun’s picture

anrikun commented

Thanks for your work Valante!
I succeeded in applying your patch after converting line breaks from LF (Unix style) to CR+LF (Windows style).
It's now committed :-)
You should use a text editor that respects file's original encoding and line breaks style.
I use EmEditor Professionnal. The free version (http://www.emeditor.com/modules/download2/rewrite/tc_5.html) is a bit outdated now but is still great.
Also, don't use tabs for indenting but spaces.

By the way, I've committed some changes too to uc_tablequote.install:
min, max and rate fields now use the "decimal(16,5)" format instead of "float" to ensure that decimal values are stored without unwanted roundings.
The way these values are displayed is not very pretty and they should be better formatted but it's not a major problem.

Please update your dev version with the new one and review it.
If there's no bug, I think we could then release a new recommanded version :-)

valante’s picture

valante commented

Hooray! Yes, Notepad++ does the trick as far as I can see. I'll keep a closer watch on that tab key :)

I'll update the new module on my test site and poke around.

I've been meaning to ask: for my private needs, I also have an "edit quote" option implemented. Would you like me to submit it? I know it's simple to re-enter the quote, but that might break the logical order of bands, and since there's no way to re-arrange them . . .

Edited also to add: why do I catch my spelling/grammar/usage errors *after* hitting save, no matter how many times I re-read everything *before*?

anrikun’s picture

anrikun commented

Yes a patch is always welcome!

By the way, is English your native language? If so, the module needs a better project page and a README.txt to explain how to install/configure it and you might be able to write it :-)

valante’s picture

valante commented

So, bad news: my entire test site has crashed and become messed up beyond repair. Good news: it had nothing to do with *this* module. :)

I'll need to reinstall the thing, then I'll run the tests. I'll also work on the edit patch, which should also be applicable toward multiple tables (hint hint).

English is not my native tongue, but I'm fluent in it, and I work as a freelance editor in English. Let me know what needs to be done and I'll do my best. I've taken a liking to this module.

More news/patch coming soon.

anrikun’s picture

anrikun commented

Too bad, sorry for your crash :-(

What needs to be done is documentation:
we should provide a project page and a README.txt describing the features of the module and explaining how to install it.

Good luck Valante!

valante’s picture

valante commented
StatusFileSize
new patch.patch17.46 KB

I abuse my test site so much that it's a wonder it hasn't downloaded itself into my local machine and killed me already. Anyway, rebuilt it and got back to work.

But oops, I kinda made some major changes by accident. In this patch:

1. Cleaned up some more redundant DB calls and loops
2. Implemented the no-conflict-if-deleted validation mechanism
3. Added an "edit" option for quotes
4. Formatted all numbers according to weight/currency
5. Formatted all input fields according to weight/currency
6. Cleaned up some tabs, rewrote some comments

So, yeah. :)

One thing I'm highly dissatisfied about is the code for formatting the fields. The same piece of code appears under the admin_settings and under the quote_edit functions, so the logic needs to be moved out to some generalized auxiliary function. I'm rather too tired to do it now.

Lemme know what you think!

~ Tal

anrikun’s picture

anrikun commented

Hi Valante,

I could not apply your patch :-(
Please redo it from last CVS version.

Thank you!

valante’s picture

valante commented
StatusFileSize
new new_patch.patch16.81 KB

Recreated against latest dev, applied back to it with success.

BTW, any news about the multiple shipping tables?

valante’s picture

valante commented

@anrikun: Did it work this time, or did you give up on me?

anrikun’s picture

anrikun commented

Hi Valante. No I didn't give up :-D !
I'm just too busy at the moment to review all your work.
I will do it this week ;-)

valante’s picture

valante commented

No worries Anrikun, just wonder if I botched it up again.

I'm now working on combining this version with the mutli-table one, and I have somewhere a project page . . . I'll have more to show you when I get back home (I'm out of town at the moment).