mesnitu / EasyPopulate4BookX

Data import and export module for Zencart 1.3.x and 1.5.x
GNU General Public License v2.0
1 stars 0 forks source link

Suggestions #1

Closed mc12345678 closed 8 years ago

mc12345678 commented 9 years ago

Looks like a lot of good work done to expand EP4 to add a new product type.

Couple of things in brief review of the code... Looks like there are a lot of queries performed that probably/possibly could be done upfront as a single query even a series of left join queries... With a large dataset need a bit of a balance between queries and taking action to prevent timeout of the program.

Regarding the check of product type being 6. It would seem (and I haven't installed the plugin to get an idea of how it identifies itself in the product list) but there should be some "generic" test to see get/identify with the product type number... Expectation (although there does not seem to be many such add-ons) is that the product type could be a different number for different sites depending on what is installed... There should be some "consistent" piece of data somewhere to which a comparison can be made...

Regarding the deletion of the product using status 10 and the modifications made by the plugin to the store, if in the store a product is deleted that is a bookx item, how are the related tables modified? Reason I ask is that status 9 does this same type of delete process if it is as straight forward as deleting a product in the standard ZC and therefore status 10 would not be needed... As well, it seems that if bookx is to be incorporated, then it would make sense to keep with a status of 9 (incorporate the deletion code applicable to a bookx product type) into the that status section... Could easily add a notifier into that section to help others out with "product" deletion...

Noticed that a lot of the code looks to be "changed" but it is really an adjustment from using 2 spaces (ZC considered standard for code formatting) to 2 tabs... Would suggest trying to match these areas up to help minimize the level of "merging" for new users.

Ohh, and also, there are some notifiers that have already been added into an upcoming version and they may help with the addition of this feature as an add-on. If they do not directly support that, I think I should be able to find a few locations to add one so that it can/does incorporate this product type. Further by using such notifiers, additional features such as this can be independently maintained and not have to update/reincorporate into the code for each subsequent version...

Just some thoughts...

mesnitu commented 9 years ago

Hi Thanks. I'm not actually what you'd call a php programmer :) I just mess around and hope for the best. I've follow the existent code (music genre) and adapted to bookx. But I have my limitations, and that's why, one can only assign one author and one genre.
But I manage a book shop, and I really needed such featured. For now is working as describe in the readme. I'm doing a urgent update to the bookstore that I run, so I had to put it on hold. I hope to get back to this soon.

I was a bit indecisive, if I would stick and follow the EP querys (based on music), or use the bookx querys to get the names, etc... so, to get and download all the stuff, I could change to Bookx querys. I've ask Philou ( the guy that made the mod ), if this was ok, and if this querys would change in other releases, to be sure not to break this EP feature. But I didn't quite had a response yet.

To Upload, it's a bit messier, but bookx has a lot of tables, it's not a easy task, specially for my knowledge. I've tested with 800 books, but not sure what happens with much more.

I think product type for bookx is 6 by default...but I'll ask Philou.

Deleting: The product is deleted with status 9, but there is a bookx_extra and bookx_extra_descritpion relation, and all data would remain, ( as I remember, I think, the same happens with music product ? ) So, the status 10, is actually using a bookx function to delete all data. Not sure, if there's a easy way to do it.

"from using 2 spaces (ZC considered standard for code formatting) to 2 tabs... " - I didn't know that, I will do that later.

About notifiers, not sure if I know how to use them... but that is a great idea.

I hope in a month or two to come back to this mod, and make the necessary changes.

Thanks