Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

introduce ability to copy an product #3605

Merged
merged 30 commits into from Aug 12, 2020

Conversation

thyseus
Copy link
Contributor

@thyseus thyseus commented Jul 30, 2020

This feature works the same as the "copy cart rule" feature.
Additional copies may follow if our project needs them - or can be added manually by copying this code as example.

@thyseus
Copy link
Contributor Author

thyseus commented Aug 3, 2020

Ability to set skippable attributes and a code style refactoring will follow soon - please do not merge yet :)

Herbert Maschke added 2 commits August 3, 2020 12:40
@thyseus
Copy link
Contributor Author

thyseus commented Aug 3, 2020

Ability to set skippable attributes and a code style refactoring will follow soon - please do not merge yet :)

Done - a bagisto developer please review this PR.

Thanks a lot !

@vaishaliwebkul
Copy link

@thyseus
Thanks!

Could you fix all the below cases:

  • Kindly add the translation for the title of copy icon in all locale https://prnt.sc/tt8j4f
  • not able to copy image attribute
  • In a copy of the configurable product type, variant attribute options are missing in case of add a new variant. https://prnt.sc/tt8mob
  • some of the attributes of booking products are not copied like available_from, available_to, locations, slots in default booking.

@thyseus
Copy link
Contributor Author

thyseus commented Aug 3, 2020

Thanks for reviewing! I will add all this tomorrow.

@thyseus
Copy link
Contributor Author

thyseus commented Aug 3, 2020

I couldnt wait until tomorrow and implemented everything (with at least the languages my translator tool was able to do).

I "blocked" the possibility to copy an 'booking' event because i think this would be to cumbersome and would not really make sense :)

please review again. thanks!

@vaishaliwebkul
Copy link

vaishaliwebkul commented Aug 4, 2020

Hi @thyseus
Thanks for fixing up the bug.

  • As booking product copy is now blocked but getting an exception when click to copy the icon of a booking product

Screenshot (52)

  • copy of configurable product is also throwing an exception

Screenshot

  • one more bug I got while reviewing this PR when copying any product than in original product (inventory, image, customer groups ) attribute values reset to default value or removed.

@thyseus
Copy link
Contributor Author

thyseus commented Aug 5, 2020

@vaishaliwebkul i improved the blocking 'booking' products and also improved the copying of an 'configurable' product type. The variants should now also properly be copied. Please check again if i got it right this time.

Can you elaborate on the last error you reported? I do not understand what you mean / can not reproduce.

Oh and i also translated some strings to german (my mother language) that you missed so far while merging.

Thanks !

@vaishaliwebkul
Copy link

vaishaliwebkul commented Aug 5, 2020

@thyseus
In the last issue, Let's say you have a simple product having qty 100 when you create a copy of this product, the original product qty gets 0.
same image, customer group attributes values reset to blank of the original product. you may also check this https://prnt.sc/tudveu

Ok, Thanks I will review this PR for German lang also.

@thyseus
Copy link
Contributor Author

thyseus commented Aug 5, 2020

Ah i see. Will look into it.

@thyseus
Copy link
Contributor Author

thyseus commented Aug 5, 2020

@vaishaliwebkul are you sure the mentioned fields are still not copied properly? In my user tests everything is fine, and the automated tests also checks that the relations of the copy are saved properly. Could you please try again ? If the error still persists, could you send me a copy of the exact demo data that you try to copy?

Besides of that i wrapped the copy operation around a transaction - if anything fails, it should automatically roll back to the original state now.

@vaishaliwebkul
Copy link

@thyseus
all the attribute are copied fine but I am getting issue in the original product, not in the copied product
kindly check this video https://www.loom.com/share/1ae3c5d3708445ba8e04e33c53c95c0d , I hope you will easily get my issue.

Thanks

@bhanu-webkul
Copy link

@thyseus linked products are not getting copied

@bhanu-webkul
Copy link

@thyseus

can we change url key every time when we copy any product? As of now when we copy 2nd time same product - https://prnt.sc/tv7r6l

@thyseus
Copy link
Contributor Author

thyseus commented Aug 7, 2020

@vaishaliwebkul Wow, this took me some time to find the bug. Here is the solution: 326c19f :) - please try again, the original product should now not be touched.

@bhanu-webkul I added a random 6 letter suffix to the name and url_key of the copy and avoided "copy of copy of ..." cascades. I do not think that linked products should also be copied, this would be unexpected behavior in my opinion.

@thyseus
Copy link
Contributor Author

thyseus commented Aug 10, 2020

@bhanu-webkul i implemented a method to set globally if a copy of a product should be automatically linked to the original (products.linkProductsOnCopy config key). I think this is what you initially ment.

@vaishaliwebkul
Copy link

vaishaliwebkul commented Aug 10, 2020

@thyseus
can you please reset the ability to add the copied product linking?? the query of @bhanu-webkul is different.

Please reset the last commit, as the issue occurs in copy category
Screenshot_10

Screenshot_9

Another bug, getting an exception when copy configurable product

Screenshot_11

@thyseus
Copy link
Contributor Author

thyseus commented Aug 10, 2020

@vaishaliwebkul i fixed the bug with the empty categories and i set "linkProductsOnCopy" to default: off.

What was the original intent of @bhanu-webkul about the linked products ?

@vaishaliwebkul
Copy link

@thyseus
His query is for provide the ability to copy the linked product, kindly read guide for more information about linked products .

@thyseus
Copy link
Contributor Author

thyseus commented Aug 10, 2020

I see. I did not know that documentation yet. Thanks a lot. So is the current implementation/behavior as intended ?

@vaishaliwebkul
Copy link

@thyseus
Yes, now the current behavior is working as expected.
Please check the issue getting while creating copy of configurable product
Screenshot_11

@thyseus
Copy link
Contributor Author

thyseus commented Aug 10, 2020

@vaishaliwebkul i added a fix that makes copying of configurable products (incl. their variants) possible again. thanks a lot for all your testing, btw.

@vaishaliwebkul
Copy link

vaishaliwebkul commented Aug 11, 2020

@thyseus
could you please fix the bug for the following case
When we copying a variant( let's say option are GREEN-S) of a configurable product, it's added into a configurable product.

Actual Behaviour: the existing variant label option(Green-s) can be added into the configurable product again by copying the variant. https://prnt.sc/txk29z
Expected Behaviour: the same attribute label variant cannot be copied if already exist, instead of it should show the alert in product list as shown https://prnt.sc/txk1ey

@thyseus
Copy link
Contributor Author

thyseus commented Aug 11, 2020

@vaishaliwebkul i do not really get the point of that. I do the following scenario:

  1. create a configurable product with 2x2 combinations. Lets call it "original".
  2. i have 5 products. One configurable and 4 simple with all the variations of "original".
  3. now i copy the configurable item.
  4. i get 5 NEW products: 1 of type configurable called "Copy of Original" and 4 copies of the 4 variants.

I now have a total of 10 products, 5 from the original, 5 copied ones.

  1. i try to copy one of the variant, "green", and "S". In this case "green" and "S" is already existing which is invalid. So i can NEVER copy a variant if this "invalid" state is being blocked by the interface. Please note that all variants are copied as status = 0 (draft/offline) so the admin can configure them before setting them to public.

So in my opinion we should allow the admin to create the "Invalid" state so it is easier for him to create all "combinations" that he need.

If you want, i can block the copying of "simple" products that had been created by a "configurable" completely, but i think this is a bad decision.

@vaishaliwebkul
Copy link

@thyseus
In my opinion, we should not break the system functionality, As in case the admin active the invalid copied product then system functionality crash.

So kindly block the copy for a simple product that is created by the configurable product & show the alert https://prnt.sc/txk1ey

@thyseus
Copy link
Contributor Author

thyseus commented Aug 11, 2020

@vaishaliwebkul ok - i blocked the ability to copy a product variant (having parent_id)

@vaishaliwebkul
Copy link

@thyseus
Thanks :)

@jitendra-webkul jitendra-webkul merged commit 2c3a8ec into bagisto:master Aug 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants