Dev - Bug in the updateCreditCard service

[Jacopo Cappellato]

Hi,

there is a problem in the current implementation of the
"updateCreditCard" service: the service expires the old cc (by setting
the PaymentMethod.thuDate field) and creates a new one (CreditCard and
PaymentMethod) with the updated values.
The problem is that, if the cc has been already expired, calling the
"updateCreditCard" service causes the cc to be re-enabled.

I'm not sure if this is good or not however this is a problem when the
"updateCreditCard" service is called as a consequence of the billing
address update; this can happen when:

1) you call the "updatePostalAddress" service; this service expires the
original address and creates a new one
2) a seca calls the "updatePaymentMethodAddress" service
3) the "updatePaymentMethodAddress" service calls the "updateCreditCard"
service for all the cc that have the old address associated to them

#3 will make all the expired ccs, associated with that address,
re-enabled automatically.

The attached patch for the "updateCreditCard" service fixes the bug but
I'm not sure that this is the best way to do this... should we modify
the "updatePaymentMethodAddress" service instead (to update only the
active ccs)?

Jacopo

Index: applications/accounting/src/org/ofbiz/accounting/payment/PaymentMethodServices.java
===================================================================
--- applications/accounting/src/org/ofbiz/accounting/payment/PaymentMethodServices.java (revision 7275)
+++ applications/accounting/src/org/ofbiz/accounting/payment/PaymentMethodServices.java (working copy)
@@ -324,7 +324,10 @@
 
         newPm.set("partyId", partyId);
         newPm.set("fromDate", context.get("fromDate"), false);
-        newPm.set("thruDate", context.get("thruDate"));
+        // The following check is needed to avoid to reactivate an expired pm
+        if (newPm.get("thruDate") == null) {
+            newPm.set("thruDate", context.get("thruDate"));
+        }
         newCc.set("companyNameOnCard", context.get("companyNameOnCard"));
         newCc.set("titleOnCard", context.get("titleOnCard"));
         newCc.set("firstNameOnCard", context.get("firstNameOnCard"));

 
_______________________________________________
Dev mailing list
[hidden email]
http://lists.ofbiz.org/mailman/listinfo/dev

[Oleg Kozyrev Jr.]

Hello all,

Ftl transforms loading in org.ofbiz.base.util.template.FreeMarkerWorker is not really flexible at the moment.
Now classes to load are hard-coded, like this:
   ftlTransforms.put("ofbizUrl", loader.loadClass("org.ofbiz.webapp.ftl.OfbizUrlTransform").newInstance());
   ftlTransforms.put("ofbizContentUrl", loader.loadClass("org.ofbiz.webapp.ftl.OfbizContentTransform").newInstance());
   ftlTransforms.put("ofbizCurrency", loader.loadClass("org.ofbiz.webapp.ftl.OfbizCurrencyTransform").newInstance());
   ftlTransforms.put("ofbizAmount", loader.loadClass("org.ofbiz.webapp.ftl.OfbizAmountTransform").newInstance());

So, if we need to add new class we need to recompile all the component. This is not right way, I think.

I suggest to create separate properties file, like:
ofbizUrl=org.ofbiz.webapp.ftl.OfbizUrlTransform
ofbizContentUrl=org.ofbiz.webapp.ftl.OfbizContentTransform
ofbizCurrency=org.ofbiz.webapp.ftl.OfbizCurrencyTransform

and load them using UtilProperties class.

If nobody is against I can create a Jira issue with patches. What do you think?

Oleg.
 
_______________________________________________
Dev mailing list
[hidden email]
http://lists.ofbiz.org/mailman/listinfo/dev

[Andrew Sykes]

Oleg,

This sounds like a good idea!
--
Kind Regards
Andrew Sykes <[hidden email]>
Sykes Development Ltd
http://www.sykesdevelopment.com

 
_______________________________________________
Dev mailing list
[hidden email]
http://lists.ofbiz.org/mailman/listinfo/dev

[Si Chen-2]

Jacopo,

I think we should fix it in updateCreditCard, but by doing a check right at the beginning to see if the change is for the expiration date and, if not, whether the credit card is already expired.  Thus, we should let the credit card be brought back to life if the user is updating the expiration date.  Otherwise, if the card is already expired, no other updates should be allowed.

Si

Jacopo Cappellato wrote:

Hi,

there is a problem in the current implementation of the "updateCreditCard" service: the service expires the old cc (by setting the PaymentMethod.thuDate field) and creates a new one (CreditCard and PaymentMethod) with the updated values.
The problem is that, if the cc has been already expired, calling the "updateCreditCard" service causes the cc to be re-enabled.

I'm not sure if this is good or not however this is a problem when the "updateCreditCard" service is called as a consequence of the billing address update; this can happen when:

1) you call the "updatePostalAddress" service; this service expires the original address and creates a new one
2) a seca calls the "updatePaymentMethodAddress" service
3) the "updatePaymentMethodAddress" service calls the "updateCreditCard" service for all the cc that have the old address associated to them

#3 will make all the expired ccs, associated with that address, re-enabled automatically.

The attached patch for the "updateCreditCard" service fixes the bug but I'm not sure that this is the best way to do this... should we modify the "updatePaymentMethodAddress" service instead (to update only the active ccs)?

Jacopo

Index: applications/accounting/src/org/ofbiz/accounting/payment/PaymentMethodServices.java
===================================================================
--- applications/accounting/src/org/ofbiz/accounting/payment/PaymentMethodServices.java	(revision 7275)
+++ applications/accounting/src/org/ofbiz/accounting/payment/PaymentMethodServices.java	(working copy)
@@ -324,7 +324,10 @@
 
         newPm.set("partyId", partyId);
         newPm.set("fromDate", context.get("fromDate"), false);
-        newPm.set("thruDate", context.get("thruDate"));
+        // The following check is needed to avoid to reactivate an expired pm
+        if (newPm.get("thruDate") == null) {
+            newPm.set("thruDate", context.get("thruDate"));
+        }
         newCc.set("companyNameOnCard", context.get("companyNameOnCard"));
         newCc.set("titleOnCard", context.get("titleOnCard"));
         newCc.set("firstNameOnCard", context.get("firstNameOnCard"));
  

 
_______________________________________________
Dev mailing list
[hidden email]
http://lists.ofbiz.org/mailman/listinfo/dev

 
_______________________________________________
Dev mailing list
[hidden email]
http://lists.ofbiz.org/mailman/listinfo/dev

[David E. Jones]

Taking a step back from the specifics on this... Why are we updating payment methods that point to the address in the first place? The whole point of having immutable artifacts like a ContactMech is to avoid this. Do we really want to be doing this? Would it not be better to go through and change the payment methods manually as we really don't know if the user intended that address change to carry over into this area?

-David


Si Chen wrote:

> Jacopo,
>
> I think we should fix it in updateCreditCard, but by doing a check right
> at the beginning to see if the change is for the expiration date and, if
> not, whether the credit card is already expired.  Thus, we should let
> the credit card be brought back to life if the user is updating the
> expiration date.  Otherwise, if the card is already expired, no other
> updates should be allowed.
>
> Si
>
> Jacopo Cappellato wrote:
>> Hi,
>>
>> there is a problem in the current implementation of the
>> "updateCreditCard" service: the service expires the old cc (by setting
>> the PaymentMethod.thuDate field) and creates a new one (CreditCard and
>> PaymentMethod) with the updated values.
>> The problem is that, if the cc has been already expired, calling the
>> "updateCreditCard" service causes the cc to be re-enabled.
>>
>> I'm not sure if this is good or not however this is a problem when the
>> "updateCreditCard" service is called as a consequence of the billing
>> address update; this can happen when:
>>
>> 1) you call the "updatePostalAddress" service; this service expires
>> the original address and creates a new one
>> 2) a seca calls the "updatePaymentMethodAddress" service
>> 3) the "updatePaymentMethodAddress" service calls the
>> "updateCreditCard" service for all the cc that have the old address
>> associated to them
>>
>> #3 will make all the expired ccs, associated with that address,
>> re-enabled automatically.
>>
>> The attached patch for the "updateCreditCard" service fixes the bug
>> but I'm not sure that this is the best way to do this... should we
>> modify the "updatePaymentMethodAddress" service instead (to update
>> only the active ccs)?
>>
>> Jacopo
>> ------------------------------------------------------------------------
>>
>> Index: applications/accounting/src/org/ofbiz/accounting/payment/PaymentMethodServices.java
>> ===================================================================
>> --- applications/accounting/src/org/ofbiz/accounting/payment/PaymentMethodServices.java (revision 7275)
>> +++ applications/accounting/src/org/ofbiz/accounting/payment/PaymentMethodServices.java (working copy)
>> @@ -324,7 +324,10 @@
>>  
>>          newPm.set("partyId", partyId);
>>          newPm.set("fromDate", context.get("fromDate"), false);
>> -        newPm.set("thruDate", context.get("thruDate"));
>> +        // The following check is needed to avoid to reactivate an expired pm
>> +        if (newPm.get("thruDate") == null) {
>> +            newPm.set("thruDate", context.get("thruDate"));
>> +        }
>>          newCc.set("companyNameOnCard", context.get("companyNameOnCard"));
>>          newCc.set("titleOnCard", context.get("titleOnCard"));
>>          newCc.set("firstNameOnCard", context.get("firstNameOnCard"));
>>  
>> ------------------------------------------------------------------------
>>
>>  
>> _______________________________________________
>> Dev mailing list
>> [hidden email]
>> http://lists.ofbiz.org/mailman/listinfo/dev
>
> ------------------------------------------------------------------------
>
>  
> _______________________________________________
> Dev mailing list
> [hidden email]
> http://lists.ofbiz.org/mailman/listinfo/dev

 
_______________________________________________
Dev mailing list
[hidden email]
http://lists.ofbiz.org/mailman/listinfo/dev

[Oleg Kozyrev Jr.]

I have created Jira issue on this point: http://jira.undersunconsulting.com/browse/OFBIZ-848

Oleg.

-----Original Message-----
From: Andrew Sykes <[hidden email]>
To: OFBiz Project Development Discussion <[hidden email]>
Date: Mon, 17 Apr 2006 12:53:57 +0100
Subject: Re: [OFBiz] Dev - FreeMarkerWorker flexibility

>
> Oleg,
>
> This sounds like a good idea!
> --
> Kind Regards
> Andrew Sykes <[hidden email]>
> Sykes Development Ltd
> http://www.sykesdevelopment.com
>
>  
> _______________________________________________
> Dev mailing list
> [hidden email]
> http://lists.ofbiz.org/mailman/listinfo/dev
>

Жуки@Mail.ru -  маленькие герои в мире большого спорта!
http://r.mail.ru/cln3055/zhuki.mail.ru/
 
_______________________________________________
Dev mailing list
[hidden email]
http://lists.ofbiz.org/mailman/listinfo/dev