Merge Discount Date and Usage into Commerce Discount

First stab. I somehow managed to do this patch just as mglaman was working on his vertical tabs stuff, so I had to rework it a bit. The only change required was to undo a part of the alter function he added that blew away the additional_settings changes. Otherwise, this patch seems to work OK testing against the latest Commerce Kickstart 2.x.

This patch also adds an update hook which disables the modules, but it likely needs to best tested thoroughly. It disables the modules in the update but might very well break because not all of the functions were renamed, so that is some clobbering that might happen. I'm thinking that what might need to happen is just emptying out the existing modules and leaving a .module and a .info so that the parent module can disable them but there won't be any functions laying around. Then again, this is my first time to try something like this and I'm not sure what's the best way to approach it. Caveat utilitor and all that.

As much as possible the individual modules' logic and such were put into the main commerce_discount module. If there is something missing, it likely got left out in the mess that the git pull did. Oh well.

Scratch that. I swapped my diff params!

Maybe I should say "please?"

Maybe I should say "please?"

Your patch doesn't delete the submodule, so the test class exists twice, giving error.

Fixed the URLs in the test suites. Delete the test modules and comment them out in the .info files.
Also, the patch in #2557247: Add vertical tabs to the discount configuration form blows away anything in $form['commerce_discount_fields']['additional_settings'], so I had to change it a bit so that it doesn't remove anything:

  // Vertical tabs container.
  $form['commerce_discount_fields']['additional_settings']['#type'] = 'vertical_tabs';
  $form['commerce_discount_fields']['additional_settings']['#weight'] = 99;
  $form['commerce_discount_fields']['additional_settings']['commerce_discount_compatibility'] = array(
    '#type' => 'fieldset',
    '#title' => t('Compatibility with other discounts'),
    '#collapsible' => TRUE,
  );
  $form['commerce_discount_fields']['#pre_render'][] = 'commerce_discount_vertical_tabs_pre_render';

Patched against latest HEAD.

Sorry I made a change that I should have mentioned earlier to you.
It got mixed about in this patch.

1. +++ b/README.md
   @@ -14,8 +14,8 @@ This module requires the following modules:
   -To contribute CSS fixes/improvements to this module please do so in the `sass`
   +To contribute CSS fixes/improvements to this module please do so in the `scss`
   ...
   -```sass --watch sass:css  --style expanded```
   +```sass --watch scss:css  --style expanded```
   

   don't change.

2. +++ b/scss/_mixins.scss
   --- /dev/null
   +++ b/scss/commerce_discount.scss
   

   sass not scss folder.

We should consider #1838350: Make usage transactional when working on this. Or at least discuss it.

@joelpittet, No worries. I've been rebasing before re-rolling and it has been doing the hard work of fixing the patch. ;)
@mglaman, Re #14, That's a feature request and is outside the scope of a patch like this.

Adding fieldset summaries.

Meh. Fixed some missing newlines.

Fixing the test.

Tagging for manual testing. It'd be great to find some sites using Discount heavily and running this patch and upgrade process on a dev server.

I've manually tested this and it updated without a hitch. I'm looking for at least one more manual test on an existing site (hopefully in dev environment) to try this.

I have been clicking through the patched version, but it seems to be working fine.

Update went smooth and all pages and discounts apply as expected.

The only thing I somewhat dislike is the remains of the submodules which are now marked incompatible with D7. I suppose this is done to prevent function name conflicts as explained in #2. And under no circumstance we want the old function in the submodule and the new function in the core module to be enabled at the same time. I suppose we'd need to get rid of them now or have them stick around forever.

This made me wonder what would happen if I would just delete the submodules. So I enabled the core and submodules, patched them, removed the folder containing the submodules and requested another page. Then I ran update.php.
This worked fine, so I suggest removing the submodules completely and not to leave them hang around as an incompatible module.

Apart from the issue above I'd say this is RTBC.

The problem is the record left in the system table. Commerce Kickstart 2 actually has a function for this we could copy and run in the upgrade

After patching the module, the info files (and submodules) remained in place, but the 'core' property has been removed from the info files. So I doubt it is the system table. In fact I think it persists in the system table because module_rebuild_cache() is still detecting an info file.

I believe the patch should delete the old module files and then we can start thinking about deleting the records in the system table.

Removed the module stubs.
Updated the update hook to forcibly remove the old sub-modules.
Tested in latest Commerce Kickstart... works like a charm.

Interdiff looks ok, will try this in dev tomorrow.

This looks better to me @nvahalik. Just a little nitpick.

1. +++ b/commerce_discount.install
   @@ -1104,23 +1104,15 @@ function commerce_discount_update_7110() {
   +  db_update('system')
   +    ->fields(array('status' => 0))
   +    ->condition('name', $disable_modules)
   +    ->condition('type', 'module')
   +    ->execute();
   

   Good idea, then it won't try to delete the fields and run the uninstall hooks and field deletions

2. +++ b/commerce_discount.install
   @@ -928,3 +1078,41 @@ function commerce_discount_update_7109() {
   +  foreach (entity_load('commerce_discount') as $discount) {
   +    $wrapper = entity_metadata_wrapper('commerce_discount', $discount);
   +    if (!isset($wrapper->discount_usage_limit) || $wrapper->discount_usage_limit->value() == FALSE
   +      && isset($wrapper->commerce_discount_max_uses) && $wrapper->commerce_discount_max_uses->value()) {
   +      $wrapper->discount_usage_limit = $wrapper->commerce_discount_max_uses->value();
   +      entity_save('commerce_discount', $discount);
   +    }
   +  }
   

   Can you add the comment about this being "Migrate from commerce_discount_max_uses to discount_usage_limit." So that it's clear what this is doing?

I'm going to RTBC ahead of nvahalik addressing nitpic in #29. Just tested this on a site with 35+ discounts using usages and dates. No integrity issues!

Ok great, fixed nit on commit.