You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ofbiz.apache.org by Adrian Crum <ad...@sandglass-software.com> on 2013/03/15 13:38:28 UTC
Re: svn commit: r1456897 - /ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/tax/TaxAuthorityServices.java
This will work, but it would have been best to include the date range
filter in the entity condition.
-Adrian
On 3/15/2013 11:16 AM, paulfoxworthy@apache.org wrote:
> Author: paulfoxworthy
> Date: Fri Mar 15 11:16:38 2013
> New Revision: 1456897
>
> URL: http://svn.apache.org/r1456897
> Log:
> Exclude expired product category members from tax calc (OFBIZ-4940)
>
> Modified:
> ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/tax/TaxAuthorityServices.java
>
> Modified: ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/tax/TaxAuthorityServices.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/tax/TaxAuthorityServices.java?rev=1456897&r1=1456896&r2=1456897&view=diff
> ==============================================================================
> --- ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/tax/TaxAuthorityServices.java (original)
> +++ ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/tax/TaxAuthorityServices.java Fri Mar 15 11:16:38 2013
> @@ -330,7 +330,7 @@ public class TaxAuthorityServices {
> } else {
> productIdCond = EntityCondition.makeCondition("productId", EntityOperator.EQUALS, product.getString("productId"));
> }
> - List<GenericValue> pcmList = delegator.findList("ProductCategoryMember", productIdCond, UtilMisc.toSet("productCategoryId"), null, null, true);
> + List<GenericValue> pcmList = delegator.findList("ProductCategoryMember", productIdCond, UtilMisc.toSet("productCategoryId", "fromDate", "thruDate"), null, null, true);
> pcmList = EntityUtil.filterByDate(pcmList, true);
> for(GenericValue pcm : pcmList) {
> productCategoryIdSet.add(pcm.getString("productCategoryId"));
>
>
Re: svn commit: r1456897 - /ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/tax/TaxAuthorityServices.java
Posted by Scott Gray <sc...@hotwaxmedia.com>.
Hi Paul,
The result would actually be hit because of the bug I mentioned in the EntityDateFilterCondition class, you'll note that it doesn't store the moment as a field and hence doesn't reference it in the equals() method. So equality is based solely on the class and the field names of the from and thru dates. I filed a jira a few years back: https://issues.apache.org/jira/browse/OFBIZ-3880
Assuming that was fixed then the result would not be hit again because the moment in time would have changed and the condition wouldn't be matched in the cache. You can't really only check for a date without the time because it would defeat the purpose of a timestamp being used. User's would no longer have the ability to end a promotion (for example) at a certain time of the day. Even if we were to do this for certain types of records where that specificity might not necessarily be needed, even crossovers at midnight would be unreliable because the server might not be located in the same timezone as the tax jurisdiction (a closer example) implementing a change in policy.
Regards
Scott
On 19/03/2013, at 1:55 AM, Paul Foxworthy wrote:
> Hi Adrian and Scott,
>
> Many thanks for reviewing my commit.
>
> The design decision to do date filtering separately from the entity
> condition predates my change. I don't feel qualified to vary the existing
> design. Please feel free to reopen OFBIZ-4940 and revise if you wish.
>
> Scott, is the reason a cached result won't be hit is we are comparing from
> and thru dates with "now", where "now" is changing all the time? If we
> restricted date filtering, where possible, to date and not time of day,
> would that help caching?
>
> Which class doesn't implement equals correctly?
>
> Thanks
>
> Paul Foxworthy
>
>
> Scott Gray-2 wrote
>> Actually in this case probably not. The query uses the cache and the
>> result set is likely small so filtering by date in memory should be more
>> efficient, you can't use the cache and include a date condition because
>> the cached result would never be hit again. There's also a bug in the
>> date condition code that prevents it from caching correctly (it doesn't
>> implement equals() correctly which causes false hits).
>>
>> A potentially cool feature that just occurred to me would be to implement
>> a list or iterator wrapper that would filter by date as the results are
>> iterated over, that would prevent the need to iterate over the results
>> twice (once for the date filter and again to actually work with the
>> values). The list or iterator could be constructed with an
>> EntityCondition parameter.
>>
>> Regards
>> Scott
>>
>> On 16/03/2013, at 1:38 AM, Adrian Crum wrote:
>>
>>> This will work, but it would have been best to include the date range
>>> filter in the entity condition.
>>>
>>> -Adrian
>>>
>>> On 3/15/2013 11:16 AM,
>
>> paulfoxworthy@
>
>> wrote:
>>>> Author: paulfoxworthy
>>>> Date: Fri Mar 15 11:16:38 2013
>>>> New Revision: 1456897
>>>>
>>>> URL: http://svn.apache.org/r1456897
>>>> Log:
>>>> Exclude expired product category members from tax calc (OFBIZ-4940)
>>>>
>>>> Modified:
>>>>
>>>> ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/tax/TaxAuthorityServices.java
>>>>
>>>> Modified:
>>>> ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/tax/TaxAuthorityServices.java
>>>> URL:
>>>> http://svn.apache.org/viewvc/ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/tax/TaxAuthorityServices.java?rev=1456897&r1=1456896&r2=1456897&view=diff
>>>> ==============================================================================
>>>> ---
>>>> ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/tax/TaxAuthorityServices.java
>>>> (original)
>>>> +++
>>>> ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/tax/TaxAuthorityServices.java
>>>> Fri Mar 15 11:16:38 2013
>>>> @@ -330,7 +330,7 @@ public class TaxAuthorityServices {
>>>> } else {
>>>> productIdCond =
>>>> EntityCondition.makeCondition("productId", EntityOperator.EQUALS,
>>>> product.getString("productId"));
>>>> }
>>>> - List
>> <GenericValue>
>> pcmList = delegator.findList("ProductCategoryMember", productIdCond,
>> UtilMisc.toSet("productCategoryId"), null, null, true);
>>>> + List
>> <GenericValue>
>> pcmList = delegator.findList("ProductCategoryMember", productIdCond,
>> UtilMisc.toSet("productCategoryId", "fromDate", "thruDate"), null, null,
>> true);
>>>> pcmList = EntityUtil.filterByDate(pcmList, true);
>>>> for(GenericValue pcm : pcmList) {
>>>>
>>>> productCategoryIdSet.add(pcm.getString("productCategoryId"));
>>>>
>>>>
>>>
>
>
>
>
>
> -----
> --
> Coherent Software Australia Pty Ltd
> http://www.coherentsoftware.com.au/
>
> Bonsai ERP, the all-inclusive ERP system
> http://www.bonsaierp.com.au/
>
> --
> View this message in context: http://ofbiz.135035.n4.nabble.com/Re-svn-commit-r1456897-ofbiz-trunk-applications-accounting-src-org-ofbiz-accounting-tax-TaxAuthoritya-tp4639885p4639906.html
> Sent from the OFBiz - Dev mailing list archive at Nabble.com.
Re: svn commit: r1456897 -
/ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/tax/TaxAuthorityServices.java
Posted by Paul Foxworthy <pa...@cohsoft.com.au>.
Hi Adrian and Scott,
Many thanks for reviewing my commit.
The design decision to do date filtering separately from the entity
condition predates my change. I don't feel qualified to vary the existing
design. Please feel free to reopen OFBIZ-4940 and revise if you wish.
Scott, is the reason a cached result won't be hit is we are comparing from
and thru dates with "now", where "now" is changing all the time? If we
restricted date filtering, where possible, to date and not time of day,
would that help caching?
Which class doesn't implement equals correctly?
Thanks
Paul Foxworthy
Scott Gray-2 wrote
> Actually in this case probably not. The query uses the cache and the
> result set is likely small so filtering by date in memory should be more
> efficient, you can't use the cache and include a date condition because
> the cached result would never be hit again. There's also a bug in the
> date condition code that prevents it from caching correctly (it doesn't
> implement equals() correctly which causes false hits).
>
> A potentially cool feature that just occurred to me would be to implement
> a list or iterator wrapper that would filter by date as the results are
> iterated over, that would prevent the need to iterate over the results
> twice (once for the date filter and again to actually work with the
> values). The list or iterator could be constructed with an
> EntityCondition parameter.
>
> Regards
> Scott
>
> On 16/03/2013, at 1:38 AM, Adrian Crum wrote:
>
>> This will work, but it would have been best to include the date range
>> filter in the entity condition.
>>
>> -Adrian
>>
>> On 3/15/2013 11:16 AM,
> paulfoxworthy@
> wrote:
>>> Author: paulfoxworthy
>>> Date: Fri Mar 15 11:16:38 2013
>>> New Revision: 1456897
>>>
>>> URL: http://svn.apache.org/r1456897
>>> Log:
>>> Exclude expired product category members from tax calc (OFBIZ-4940)
>>>
>>> Modified:
>>>
>>> ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/tax/TaxAuthorityServices.java
>>>
>>> Modified:
>>> ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/tax/TaxAuthorityServices.java
>>> URL:
>>> http://svn.apache.org/viewvc/ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/tax/TaxAuthorityServices.java?rev=1456897&r1=1456896&r2=1456897&view=diff
>>> ==============================================================================
>>> ---
>>> ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/tax/TaxAuthorityServices.java
>>> (original)
>>> +++
>>> ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/tax/TaxAuthorityServices.java
>>> Fri Mar 15 11:16:38 2013
>>> @@ -330,7 +330,7 @@ public class TaxAuthorityServices {
>>> } else {
>>> productIdCond =
>>> EntityCondition.makeCondition("productId", EntityOperator.EQUALS,
>>> product.getString("productId"));
>>> }
>>> - List
> <GenericValue>
> pcmList = delegator.findList("ProductCategoryMember", productIdCond,
> UtilMisc.toSet("productCategoryId"), null, null, true);
>>> + List
> <GenericValue>
> pcmList = delegator.findList("ProductCategoryMember", productIdCond,
> UtilMisc.toSet("productCategoryId", "fromDate", "thruDate"), null, null,
> true);
>>> pcmList = EntityUtil.filterByDate(pcmList, true);
>>> for(GenericValue pcm : pcmList) {
>>>
>>> productCategoryIdSet.add(pcm.getString("productCategoryId"));
>>>
>>>
>>
-----
--
Coherent Software Australia Pty Ltd
http://www.coherentsoftware.com.au/
Bonsai ERP, the all-inclusive ERP system
http://www.bonsaierp.com.au/
--
View this message in context: http://ofbiz.135035.n4.nabble.com/Re-svn-commit-r1456897-ofbiz-trunk-applications-accounting-src-org-ofbiz-accounting-tax-TaxAuthoritya-tp4639885p4639906.html
Sent from the OFBiz - Dev mailing list archive at Nabble.com.
Re: svn commit: r1456897 - /ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/tax/TaxAuthorityServices.java
Posted by Scott Gray <sc...@hotwaxmedia.com>.
Actually in this case probably not. The query uses the cache and the result set is likely small so filtering by date in memory should be more efficient, you can't use the cache and include a date condition because the cached result would never be hit again. There's also a bug in the date condition code that prevents it from caching correctly (it doesn't implement equals() correctly which causes false hits).
A potentially cool feature that just occurred to me would be to implement a list or iterator wrapper that would filter by date as the results are iterated over, that would prevent the need to iterate over the results twice (once for the date filter and again to actually work with the values). The list or iterator could be constructed with an EntityCondition parameter.
Regards
Scott
On 16/03/2013, at 1:38 AM, Adrian Crum wrote:
> This will work, but it would have been best to include the date range filter in the entity condition.
>
> -Adrian
>
> On 3/15/2013 11:16 AM, paulfoxworthy@apache.org wrote:
>> Author: paulfoxworthy
>> Date: Fri Mar 15 11:16:38 2013
>> New Revision: 1456897
>>
>> URL: http://svn.apache.org/r1456897
>> Log:
>> Exclude expired product category members from tax calc (OFBIZ-4940)
>>
>> Modified:
>> ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/tax/TaxAuthorityServices.java
>>
>> Modified: ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/tax/TaxAuthorityServices.java
>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/tax/TaxAuthorityServices.java?rev=1456897&r1=1456896&r2=1456897&view=diff
>> ==============================================================================
>> --- ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/tax/TaxAuthorityServices.java (original)
>> +++ ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/tax/TaxAuthorityServices.java Fri Mar 15 11:16:38 2013
>> @@ -330,7 +330,7 @@ public class TaxAuthorityServices {
>> } else {
>> productIdCond = EntityCondition.makeCondition("productId", EntityOperator.EQUALS, product.getString("productId"));
>> }
>> - List<GenericValue> pcmList = delegator.findList("ProductCategoryMember", productIdCond, UtilMisc.toSet("productCategoryId"), null, null, true);
>> + List<GenericValue> pcmList = delegator.findList("ProductCategoryMember", productIdCond, UtilMisc.toSet("productCategoryId", "fromDate", "thruDate"), null, null, true);
>> pcmList = EntityUtil.filterByDate(pcmList, true);
>> for(GenericValue pcm : pcmList) {
>> productCategoryIdSet.add(pcm.getString("productCategoryId"));
>>
>>
>