Hi,

I made a drupal 6 port of that module. I based my work on the latest dev version of october 29th 2008.

It is just a basic port ATM, cause maintainers wanted to proceed step by step. I plan to bring some other stuff, but it will be added later via patches. However I bring some verrrry little changes, that can be applied to 5.x too easily, so, here is the changelog :

  • Drupal 6 port ! (this is the main change)
  • Separate all administration functions in a uc_discounts_admin.inc file (new d6 menu allow to declare a separate file, I used it, don't remember if d6 allow it)
  • When you delete a discount, it delete all related actions and conditions (ATM, the d5 version only delete the discount, letting conditions and actions polluate the database)
  • Added uc_discounts_user submodule in the package (found here : http://www.ubercart.org/contrib/143#comment-20625) and correct a condition check problem
  • Added description to condition quantity field for uc_discounts_product
  • I think that all for this package...

I would add that I test it using uc_discounts_product and uc_discounts_user submodules, I didn't make some tests on others submodules cause I don't use them and I don't have time for a full testing of all submodules. As submodules don't include menu, form, or others things that need attention on a drupal 6 port, it should work as the 5.x version. I let the community gives feedback about that.

So, the next step for me is to make some improvements and add some features into that release via patches. Here is my battleplan :

  • Make uc_discounts_product submodules working with attributes, and bring the necessary changes into hook_nodeapi of uc_discounts module
  • Allow submodules to add informations in a node when a potential discount is found. Example, you set a condition that make 10% discount for a product if quantity = 3. When the user view the product page, the discount module, actually, show nothing, because it don't know if the user will buy 3 quantity of that product, so it cant alter the display price. The idea is to display, somewhere in the node, a sentence like "Buy 3 quantity of that product, and get a 10% discount". So users will exactly know what's the deal, and it will be done automatically.

Can maintainers review and, if correct, submit the 6.x branch for that module ? It would allow users to know that it's in progress, and allow me to choose 6.x when I create an issue to submit a patch. I would add that all patch I will submit wil be for 6.x branch and will need to be backported if you want to see that in 5.x.

zmove

CommentFileSizeAuthor
#5 uc_discounts.install.patch619 byteszmove
#5 uc_discounts.install.txt3.1 KBzmove
#3 uc_discounts.zip33.91 KBzmove

Comments

zmove’s picture

zmove commented

I forget to add one thing for my battleplan :

  • Change the item_id varchar(256) in the databse by a TEXT type, cause some modules, like multi-products save serialized arrays in that database field, and a varchar 256 for serialized array is too limited
zmove’s picture

zmove commented

Lol, I speak, I speak and I forget to post the package !

zmove’s picture

zmove commented
StatusFileSize
new uc_discounts.zip33.91 KB

Wrong package uploaded, I'm a noob ^^

here is the good one

psynaptic’s picture

psynaptic commented

I've had a good look through this and it's looking pretty clean.

I've listed all the changes that were not specifically related to the port to 6.x.

uc_discounts.module

  1. uc_discounts_discount_form() - changes to #value elements - backported to 5.x
  2. uc_discounts_discount_form_submit() - moved variables - moved variable creation to match 6.x port
  3. uc_discounts_discount_delete_form_submit() - added delete of conditions and actions - backported to 5.x
  4. uc_discounts_condition_form() - removal of $amount_field and $form['amount'] - see comment below
  5. uc_discounts_condition_form_submit() - addition of two drupal_set_message() calls to confirm when conditions and actions are added - backported to 5.x
  6. uc_discounts_condition_delete() - addition of drupal_set_message()</del> - backported to 5.x
  7. uc_discounts_actions_table() - change Item to Items - reverted to Item
  8. uc_discounts_action_delete() - addition of drupal_set_message() - backported to 5.x
  9. moved all admin functions out into uc_discounts_admin.inc - releated to port to 6.x

uc_discounts_product.module

  1. uc_discounts_product_discounts_condition() - changed compare_type to text - backported to 5.x
  2. uc_discounts_product_discounts_action() - changed has_qty_field to false - backported to 5.x
  3. uc_discounts_product_item_field() - added '#required' => true - backported to 5.x
  4. uc_discounts_product_value_field() - added '#description' => t('How many products does the user have to buy to get this discount?') - backported to 5.x but changed to "The number of products the user must to buy to get this discount."

uc_discounts_taxonomy.module

  1. uc_discounts_product_apply() - removed all debug code and some comments - backported to 5.x

I'm pretty happy to go ahead and commit, at least now we have a list to work with and can track down all stuff we may want to backport to 5.x.

Anyone want to take a peek before I commit?

zmove’s picture

zmove commented
StatusFileSize
new uc_discounts.install.txt3.1 KB
new uc_discounts.install.patch619 bytes

Hi,

Wow, nice review ! I made this port by removing some addition and functionnalities I made in my experimental d6 version of that module that support attributes and some others things, Item to Items is a good example of a thing I forget to remove to be like the original module. ^^'

I would add some things to your review :

  • The step logic change are related to drupal 6 as drupal 6 form step management has changed, this is the way to go with d6. So, even if you like the idea, I don't think it could be backported to 5.x. And it would be useless as it does exactly the same thing.
  • Deletion of conditions and actions are necessary for the 5.x release, I think you should backport that
  • The query submit function has changed because drupal don't support the db_last_insert_id() function, so I have to find another way to insert and update query
  • For the compare type of uc_discounts_products, change it to text allow only != or = instead of >=, <= etc... I don't think it's usefull to see >= and <= for a product name comparaison (even if, in fact, this is the id of the product that is saved). I would add that a global submodules review is necessary to put the good compare type, cause the uc_discounts_product module is not the only one to have non-sense comparaison type.
  • for the #required attribute, it don't change things a lot as the select list don't have an empty option, so you cannot don't choose product, but it's more logical to put it as #required

I would add a last things. I suggest you to change the item_id field in uc_discounts_conditions and uc_discounts_actions table in the database into a TEXT type. Actually a varchar(256) is too limit and it can cause error to submodules that save informations in a serialized array like the multiproduct module.

I attach a patch and the original modified .install file for the 6.x branch. I think a copy/paste can be enough to backport to 5.x as I believe the update function hasn't changed.

(Put .txt to the .install file to make drupal accept the upload)

psynaptic’s picture

psynaptic commented

I have committed this with a few minor changes to the DRUPAL-6--1 branch. I have made a new release and it will appear on the project page at about midnight tonight.

Not marking as fixed just yet since I need to work through the points in the review above.

4. Amount field

The creation of the $amount_field array was in step 1 of the multi-step form. This stopped the amount field on the form from showing. I'm removing it for now, if and when we decide we need it we can always put it back.

<?php
    $amount_field = array(
      '#type' => 'textfield',
      '#title' => t('Dollar Amount'),
      '#description' => t('If set, condition will match only products whose sell price matches this amount.'),
      '#maxlength' => 12,
      '#size' => 6,
    );
    $form['amount'] = $amount_field;
    $form['amount']['#default_value'] = $condition->amount;
?>

I also just converted tabs to spaces and removed all trailing spaces from the DRUPAL-6--1 branch. zmove, please set your editor to use soft tabs (2 spaces) rather than actual tabs.

psynaptic’s picture

psynaptic commented
Status: Active » Fixed

Marking as fixed now since I've just backported everything relevant to 5.x.

psynaptic’s picture

psynaptic commented
Assigned: Unassigned » psynaptic
Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

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