ProductConfigWorker and ProductConfigWrapper multiple issues - Please comment

[Ritesh Trivedi]

Hi,

Have been trying to use configure feature and am encountering several limitations (possible bug(s)/issues) with the current implementation.

Both items #1 and #2 causes a bug where if you modify ProductConfigWrapper the cache instances still remains unchanged as both the methods returns deep copy of the wrapper.

1. ProductConfigWrapper.getProductConfigWrapper() not sure why we need
to call copy constructor again when the configWrapper was just created
brand new just in the previous statement?

productConfigCache.put(cacheKey, new ProductConfigWrapper(configWrapper));

can just be

productConfigCache.put(cacheKey, configWrapper);

2. Again ProductConfigWrapper.getProductConfigWrapper() not sure why
we need to call copy constructor again when an instance from cache
already exists
configWrapper = new
ProductConfigWrapper((ProductConfigWrapper)productConfigCache.get(cacheKey));

can just be

configWrapper = (ProductConfigWrapper)productConfigCache.get(cacheKey);

3. ConfigOption inner class, needs to expose either configItemId or
parent configItem - if lets say I store the selected configoptions in
the session and later time what to mark those items selected in
ProductConfigWrapper - both the setSelected() methods in
ProductConfigWrapper needs configItem information and not being able
to access it from the stored config options forces storage of
configItem objects too

4. Also on configOption isMandatory() should be exposed as isSelected
is exposed on configItem

[Ritesh Trivedi]

#5 ConfigOption.equals method is totally incorrect !! - just checking based on selected field? there can be multiple options selected. At a very least, configOption id and configItem id needs to be compared too.

Ritesh Trivedi wrote

Hi,

Have been trying to use configure feature and am encountering several limitations (possible bug(s)/issues) with the current implementation.

Both items #1 and #2 causes a bug where if you modify ProductConfigWrapper the cache instances still remains unchanged as both the methods returns deep copy of the wrapper.

1. ProductConfigWrapper.getProductConfigWrapper() not sure why we need
to call copy constructor again when the configWrapper was just created
brand new just in the previous statement?

productConfigCache.put(cacheKey, new ProductConfigWrapper(configWrapper));

can just be

productConfigCache.put(cacheKey, configWrapper);

2. Again ProductConfigWrapper.getProductConfigWrapper() not sure why
we need to call copy constructor again when an instance from cache
already exists
configWrapper = new
ProductConfigWrapper((ProductConfigWrapper)productConfigCache.get(cacheKey));

can just be

configWrapper = (ProductConfigWrapper)productConfigCache.get(cacheKey);

3. ConfigOption inner class, needs to expose either configItemId or
parent configItem - if lets say I store the selected configoptions in
the session and later time what to mark those items selected in
ProductConfigWrapper - both the setSelected() methods in
ProductConfigWrapper needs configItem information and not being able
to access it from the stored config options forces storage of
configItem objects too

4. Also on configOption isMandatory() should be exposed as isSelected
is exposed on configItem

[Bilgin Ibryam]

Quoting Ritesh Trivedi <[hidden email]>:

Hi Ritesh, comments inline

>
> Hi,
>
> Have been trying to use configure feature and am encountering several
> limitations (possible bug(s)/issues) with the current implementation.
>
> Both items #1 and #2 causes a bug where if you modify ProductConfigWrapper
> the cache instances still remains unchanged as both the methods returns deep
> copy of the wrapper.

In the cache is stored ProductConfigWrapper instances which are not  
configured/modified. If you check the cache key, you will see that  
there is no indicator for selected options, so it is not possible to  
make difference between configured options in the cache.

> 1. ProductConfigWrapper.getProductConfigWrapper() not sure why we need
> to call copy constructor again when the configWrapper was just created
> brand new just in the previous statement?
>
> productConfigCache.put(cacheKey, new ProductConfigWrapper(configWrapper));
>
> can just be
>
> productConfigCache.put(cacheKey, configWrapper);
>
> 2. Again ProductConfigWrapper.getProductConfigWrapper() not sure why
> we need to call copy constructor again when an instance from cache
> already exists
> configWrapper = new
> ProductConfigWrapper((ProductConfigWrapper)productConfigCache.get(cacheKey));
>
> can just be
>
> configWrapper = (ProductConfigWrapper)productConfigCache.get(cacheKey);

Changes #1 and #2 will cause to addition to the cache an instance  
which will be accessible and configured from the user who added it.  
But then every next user using the same cache key (the same store,  
catalog and currency) will get the already configured object with  
selected options from the first user.

>
> 3. ConfigOption inner class, needs to expose either configItemId or
> parent configItem - if lets say I store the selected configoptions in
> the session and later time what to mark those items selected in
> ProductConfigWrapper - both the setSelected() methods in
> ProductConfigWrapper needs configItem information and not being able
> to access it from the stored config options forces storage of
> configItem objects too

I think it is ok to expose parent configItem.

>
> 4. Also on configOption isMandatory() should be exposed as isSelected
> is exposed on configItem
>

If configItem is exposed (from point 3) isMandatory() can be called.  
As options can't be mandatory, adding isMandatory() to configOption  
could be confusing.

Bilgin

> --
> View this message in context:  
> http://www.nabble.com/ProductConfigWorker-and-ProductConfigWrapper-multiple-issues---Please-comment-tp19114133p19114133.html
> Sent from the OFBiz - Dev mailing list archive at Nabble.com.
>
>



----------------------------------------------------------------
This message was sent using IMP, the Internet Messaging Program.

[Ritesh Trivedi]

Hi Bilgin,

For item #1 and #2 - makes sense. I didnt notice the fact that configwrapper instance was intended to be global.

Can #3 and #5 be fixed?

Bilgin Ibryam wrote

Quoting Ritesh Trivedi <[e-mail]>:

Hi Ritesh, comments inline

>
> Hi,
>
> Have been trying to use configure feature and am encountering several
> limitations (possible bug(s)/issues) with the current implementation.
>
> Both items #1 and #2 causes a bug where if you modify ProductConfigWrapper
> the cache instances still remains unchanged as both the methods returns deep
> copy of the wrapper.

In the cache is stored ProductConfigWrapper instances which are not  
configured/modified. If you check the cache key, you will see that  
there is no indicator for selected options, so it is not possible to  
make difference between configured options in the cache.


> 1. ProductConfigWrapper.getProductConfigWrapper() not sure why we need
> to call copy constructor again when the configWrapper was just created
> brand new just in the previous statement?
>
> productConfigCache.put(cacheKey, new ProductConfigWrapper(configWrapper));
>
> can just be
>
> productConfigCache.put(cacheKey, configWrapper);
>
> 2. Again ProductConfigWrapper.getProductConfigWrapper() not sure why
> we need to call copy constructor again when an instance from cache
> already exists
> configWrapper = new
> ProductConfigWrapper((ProductConfigWrapper)productConfigCache.get(cacheKey));
>
> can just be
>
> configWrapper = (ProductConfigWrapper)productConfigCache.get(cacheKey);


Changes #1 and #2 will cause to addition to the cache an instance  
which will be accessible and configured from the user who added it.  
But then every next user using the same cache key (the same store,  
catalog and currency) will get the already configured object with  
selected options from the first user.

>
> 3. ConfigOption inner class, needs to expose either configItemId or
> parent configItem - if lets say I store the selected configoptions in
> the session and later time what to mark those items selected in
> ProductConfigWrapper - both the setSelected() methods in
> ProductConfigWrapper needs configItem information and not being able
> to access it from the stored config options forces storage of
> configItem objects too

I think it is ok to expose parent configItem.

>
> 4. Also on configOption isMandatory() should be exposed as isSelected
> is exposed on configItem
>

If configItem is exposed (from point 3) isMandatory() can be called.  
As options can't be mandatory, adding isMandatory() to configOption  
could be confusing.

Bilgin

> --
> View this message in context:  
> http://www.nabble.com/ProductConfigWorker-and-ProductConfigWrapper-multiple-issues---Please-comment-tp19114133p19114133.html
> Sent from the OFBiz - Dev mailing list archive at Nabble.com.
>
>



----------------------------------------------------------------
This message was sent using IMP, the Internet Messaging Program.

[Bilgin Ibryam]

Quoting Ritesh Trivedi <[hidden email]>:

>
> Hi Bilgin,
>
> For item #1 and #2 - makes sense. I didnt notice the fact that configwrapper
> instance was intended to be global.
>
> Can #3 and #5 be fixed?
>
>

#5 is already marked as TODO in the code and should be fixed.
Feel free to create a jira issue and provide patch. I promise to  
review/test/commit.
Thanks for your work.


----------------------------------------------------------------
This message was sent using IMP, the Internet Messaging Program.