potteryhouse / stock_by_attribute_1.5.4

Stock by Attribute for Zen Cart 1.5.3
GNU General Public License v2.0
2 stars 5 forks source link

Incorporated all changes that were made in SBA V1.5.3 for ZC 1.5.1. #15

Closed mc12345678 closed 9 years ago

mc12345678 commented 9 years ago

This did include revision of determining previously ordered products that have attributes and are tracked by SBA. Wanted to get some of the testing started. Still have some delete variant(s) based on other actions to do in attributes_controller; however, need to know if have missed anything in the rest of the code.

mc12345678 commented 9 years ago

Moved the SBA related information out of includes/classes/order.php into an observer class. Part of this modification changed the captured model # information stored in the associated order data table. It now follows the rule of the model # equals the result of this if statement (if $customid then return $customid otherwise return model#.

mc12345678 commented 9 years ago

Self identified that there are errors in the coding for the install portion of the procedure (created based off of manually installing the applicable portions). Should be able to provide the corrected update in the next day or two. Too bad no one said anything otherwise it would have been fixed already.

jeking928 commented 9 years ago

I installed this branch on a stock 1.5.3 Zen Cart. After adding mixed attrbiutes for a product, all attributes disappear on the product page. image

mc12345678 commented 9 years ago

Try removing the templated tpl_modules_attributes.php file, does that resolve the issue?

If so, then need to add an additional check in that file because I forgot dynamic dropdowns defines TABLE_PRODUCTS_STOCK to point at the same table already defined and used by SBA (TABLE_PRODUCTS_WITH_STOCK_ATTRIBUTES) and so perhaps that is being referenced in the template which if not installed causes a "loss" of the attributes.

jeking928 commented 9 years ago

Removing templated tpl_modules_attributes.php did fix the problem. When you have a chance to post the fixed file, I'll check again.

Jim

mc12345678 commented 9 years ago

My guess is that you also have an error log associated with that. If you wouldn't mind double checking. Probably something about the table not existing...

jeking928 commented 9 years ago

Actually, no error log at all.

Jim

mc12345678 wrote:

My guess is that you also have an error log associated with that. If you wouldn't mind double checking. Probably something about the table not existing...

-----Original Message----- From: James King notifications@github.com Date: Thu, 15 Jan 2015 14:13:05 To: potteryhouse/stock_by_attribute_1.5.4stock_by_attribute_1.5.4@noreply.github.com

Reply-To: "potteryhouse/stock_by_attribute_1.5.4" reply@reply.github.com Subject: Re: [stock_by_attribute_1.5.4] Incorporated all changes that were made in SBA V1.5.3 for ZC 1.5.1. (#15)

Removing templated tpl_modules_attributes.php did fix the problem. When you have a chance to post the fixed file, I'll check again.

Jim


Reply to this email directly or view it on GitHub: https://github.com/potteryhouse/stock_by_attribute_1.5.4/pull/15#issuecomment-70172474

— Reply to this email directly or view it on GitHub https://github.com/potteryhouse/stock_by_attribute_1.5.4/pull/15#issuecomment-70173984.

mc12345678 commented 9 years ago

Corrected issue with tpl_modules_attributes.php for installation on new ZC without Dynamic Dropdowns installed...

jeking928 commented 9 years ago

Confirmed this issue is fixed.

jeking928 commented 9 years ago

Two new issues, the displayed qty is incorrect and the displayed options are incorrect. With a stock 1.5.3, I have stock set as: image Stock is displayed as: image

There is only 20 of the 'Value' in stock. 35 is the total of all attributes. The Memory option has all six values instead of just the three actual memory values.

The issue, I believe, is the added complexity of mixed attributes. I realize some have a need for this but I'm not sure the added complexity is worth it. I think mixed attributes are better handled by setting a single attribute and more values. For this product, it would be one option name with six values. This scenario is easily handled by SBA and simplifies the display and selection for the shopper.

mc12345678 commented 9 years ago

If I am not mistaken, I have not modified the attributes.php file. In fact I have avoided work on it, because of previous changes that were made to support a different issue (several months ago). One such change caused product to appear multiple times in a single dropdown, typically in the first or earlier dropdowns.

One of the longstanding downfalls to SBA has been exactly that it does not/has not properly handled mixed attributes. It is not a "some people" concern/issue. I will say that so far the code provided here has been able to work with Dynamic Dropdowns to at least provide indication of product that is out-of-stock; however, I would agree that the attributes.php file has some aspects requiring work... Noticed in merge/comparison yesterday with ZC 1.5.3 includes/modules/attributes.php that there are a number of pricing adjustment actions that are not considered.

Reducing the number of store attributes to accommodate SBA would be counter-intuitive to the flexibility and functionality of ZC, where attributes can be copied and mixed with various product. Moving to this path would increase the number of single entries to the number of combinations available by mixing and matching attributes instead of using the database scheme to present/modify the display data.

If instead, SBA were to combine multiple/mixed attributes into a single dropdown option, then that may be solvable without further need of java to update other fields (price, quantity available, other options available) on the fly. However, that said, the list presented to the customer when there are 3, 4 or more attributes would be tremendous... If each had 4 options, then instead of 4 individual option names, a product with 3 attributes would have 64 selections, a product with 4 attributes would have 256 options to select.... I believe that this gross quantity of options would be unacceptable to any store owner.

So, I know that I was asked also if Dynamic Dropdowns would be incorporated into this plugin. I think that the answer should be no, as not everyone would need the multi-attribute functionality that it supports; however, as evidence from including the modified tpl_module_attributes.php file, action can be taken to consider that plugin from being used...

Tracking the quantity available adjacent to the individual listing, does require java updating of the value(s) upon selection, which also has not addressed, ensuring core capability and functionality is present with follow-on results to come. If the product works as is with single attributes without fault, then the multiple-attribute functionality currently in place should be considered for the remaining development. In all cases of adding functionality, the attempt has been made to not affect what already works, but to build on the operation, even though there are some base actions carried over from previous SBA variants that do not have clear explanation of why that action was taken.

To expand this even a little further. 1) General product design suggests no more than 6 attributes for a product, which in itself is extreme; however, the need for this has been expressed time and again in the various SBA threads presented in ZC. 2) Dynamic Dropdowns as originally designed, did not take into account that not every item with an attribute would need to be tracked by attributes. Ie. product with a dropdown of add gift wrap or not, does not require the same tracking as an item available in various sizes. That said, when using Dynamic Dropdowns out of the box, the first product would be required to be entered into the SBA table and then that stock maintained in order to display the attribute. The additional code of the tpl_modules_attributes.php file removes this need, but will support SBA tracked products, therefore making that modified file a valuable portion to the functionality of this plugin and its ability to be incorporated into any store. 3) To some extent I have chosen to not yet address some of the display issues of using multiple attributes to ensure that the single attribute aspect has been satisfactorily accomplished to then support multiple attributes further. Dynamic Dropdowns currently will support multiple attributes but the display of information is currently limited to product that is in stock. I have been working on providing a class file that when incorporated into a system that has Dynamic Dropdowns installed will permit display of out of stock product as well. This does increase the amount of page loaded data (javascript data); however, provides a feature that has been desired. This type functionality more than likely could be incorporated into this code without the use of Dynamic Dropdowns; however, I've somewhat gone with modifying an existing (relatively popular) functional product rather than developing new functionality. Again, Dynamic Dropdowns may not be desired by all, especially if when using this product they only have one attribute, which will provide enough functionality to not need anything additional.

Sum in short, if the functionality offered by this branch for multiple attributes (as well as a number of other additional options/features such as the shopping_cart restriction) does not negatively affect the previous functionality that was programmed, then I recommend that this code be incorporated to at least afford the added functionality and continue modification to at least properly display information associated with multiple attributes whether with or without dynamic dropdowns.

jeking928 commented 9 years ago

I guess I'm not sure how to proceed. I'll concede that a value list can easily get to be too long to be manageable. But the current display creates issues that cause confusion. See here: http://bit.ly/1sHHv4R

The drop-downs do not reflect the actual quantities or stock values.

mc12345678 commented 9 years ago

Let me ask this... Does the same problem occur with the main thread of the software? If not what is the difference that is causing the result. If it does, then I say that the code to generate/determine the quantity available needs to be modified. Yes, it is confusing to have so many different options on display. In the test environments I have used, they typically show in the first attribute not the second, but it is confusing and personally I thought it was something previously known to be worked out, so didn't want to touch it if there was parallel work. This is also what was basically being reported in #21 although there was more going on there as well...

I guess, it appears that as the individual reporting issue #21 was using the main thread, that the issue exists throughout not just in this line of code. There are improvements in this code that I believe will help in all cases and recommend incorporating this code into the main thread and then correcting the multiple attribute issue(s) of the main thread... It is either in the code leading up to displaying the javascript, or some form of "calculation" performed in the javascript... I have looked at neither and was just starting to look at what generates the multiple option name selections and why only one of the two (haven't tried a three attribute product) attributes is so confusing by providing multiple selections for each attribute type.

jeking928 commented 9 years ago

I knew you'd ask 'Does the same problem occur with the main thread of the software?' and I was going to check. It turns out it does so it does seem to be related to #21 so I'll move comments to that thread.