During some testing I have found several issues with the handling of role expiration in 6.x-2.0-rc6. I have attached a patch for review that addresses the issues listed below.

  • uc_roles.module
    • uc_roles_feature_settings
      • Issue: improper use of undefined variable $end_time to set variable uc_roles_default_end_expiration
      • Result: form always defaults to 'relative' even if 'absolute' has been set
    • uc_roles_feature_form
      • Issue: only $end_time from $product_role->end_time used to set expiration
      • Result: product end time not being correctly set to global expiration on edit or change
    • _uc_roles_get_expiration
      • Issue: directly returns variable_get('uc_roles_default_end_time', NULL) which is a date array, but functions calling this are expecting a timestamp
      • Result: noticed this when role would be applied after order 'Completed' - expiration was set to 1
  • uc_roles.ca.inc
    • uc_roles_action_order_renew
      • Issue: $existing_role variable is used before it gets set.
      • Result: variable initially reflects nothing and after 2nd pass of role loop, reflects previous role in loop
CommentFileSizeAuthor
#5 571822_expiration_end_times.patch3.19 KBIsland Usurper
#3 571822-uc_role_end_time_fixes.patch2.96 KBrecrit
uc_role_end_time_fixes.patch4.61 KBrecrit

Comments

univate’s picture

univate commented

I think this is the same problem has described here: #569920: logic in function uc_roles_action_order_renew is wrong.

Chad_Dupuis’s picture

Chad_Dupuis commented

subscribing

recrit’s picture

recrit commented
StatusFileSize
new 571822-uc_role_end_time_fixes.patch2.96 KB

I did see #569920: logic in function uc_roles_action_order_renew is wrong. before I wrote this patch, but my issues were mainly in uc_roles.module where 569920 fixes uc_roles.ca.inc more than what this patch does. I noticed the uc_roles.ca.inc variable bug after I had fixed the other issues. After reviewing the 569920 patch, I would be fine with removing the uc_roles.ca.inc fix from this patch.

modified patch attached

Island Usurper’s picture

Island Usurper commented
Issue tags: +Release blocker
Island Usurper’s picture

Island Usurper commented
StatusFileSize
new 571822_expiration_end_times.patch3.19 KB

I thought it might be better to keep $end_time in the same format regardless of the feature's other settings. Mostly I just moved some logic from on part of the function to another.

I also set the default absolute time to be calculated off of the set relative expiration and the current time. That seems more useful than December 31, 1969 as a default value.

recrit’s picture

recrit commented

I agree - thats a much cleaner way of setting the $end_time.

Island Usurper’s picture

Island Usurper commented
Status: Needs review » Fixed

This issue mostly deals with the expiration form, rather than how the data is used. Since the form now behaves appropriately, I think it's fair to say that the patch fixes the issue.

Committed.

Status: Fixed » Closed (fixed)
Issue tags: -Release blocker

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

Issue tags: +Release blocker

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.