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.

Sign up for GitHub

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

Jump to bottom

[MIG] pos_product_template: Migration to 14.0 #750

Closed
wants to merge 1 commit into from
Closed

[MIG] pos_product_template: Migration to 14.0 #750

wants to merge 1 commit into from

Conversation

pedroguirao
Copy link

the current PR for [MIG] change the initial operative of the module, this PR respect the original funcionality ( In our oppion)

This was referenced Jan 14, 2022
Closed
Open
@legalsylvain
Copy link
Contributor

legalsylvain commented Jan 14, 2022 •
edited
Loading

Could you elaborate which changes don't respect original functionnality in the other PR ?
thanks !

CC : @hparfr

@pedroguirao
Copy link
Author

Hi @legalsylvain, @antoniocanovas will explain in detail.

dsolanki-initos
dsolanki-initos approved these changes Jan 31, 2022
View reviewed changes
Copy link
Contributor

@dsolanki-initos dsolanki-initos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionally test and code review. looks good to me.

@antoniocanovas
Copy link

Hello @legalsylvain and @pedroguirao, see attachment.
I think it's better because you can go though different properties and other selection are not loosed.

pull.pos_product_template.mov

enriquemartin
enriquemartin reviewed Feb 4, 2022
View reviewed changes
Copy link

@enriquemartin enriquemartin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested in runboat, got the error pasted below.
The error occurs when you select any product.

Traceback:
Error: ProductScreenChildren.find is not a function
_clickProduct@http://oca-pos-14-0-pr750-05212161f6d8.runboat.odoo-community.org/web/content/479-4355dba/point_of_sale.assets.js:1599:376
__trigger@http://oca-pos-14-0-pr750-05212161f6d8.runboat.odoo-community.org/web/content/474-b03fd02/web.assets_common.js:1270:9
owl.Component.prototype.__trigger@http://oca-pos-14-0-pr750-05212161f6d8.runboat.odoo-community.org/web/content/474-b03fd02/web.assets_common.js:1422:17
trigger@http://oca-pos-14-0-pr750-05212161f6d8.runboat.odoo-community.org/web/content/474-b03fd02/web.assets_common.js:1259:33
anonymous/p507.on.click@http://oca-pos-14-0-pr750-05212161f6d8.runboat.odoo-community.org/web/content/474-b03fd02/web.assets_common.js line 973 > Function:18:107
invokeHandler@http://oca-pos-14-0-pr750-05212161f6d8.runboat.odoo-community.org/web/content/474-b03fd02/web.assets_common.js:795:144
handleEvent@http://oca-pos-14-0-pr750-05212161f6d8.runboat.odoo-community.org/web/content/474-b03fd02/web.assets_common.js:799:105
handler@http://oca-pos-14-0-pr750-05212161f6d8.runboat.odoo-community.org/web/content/474-b03fd02/web.assets_common.js:801:69

@antoniocanovas
Copy link

@enriquemartin please review again.
I think there is something wrong on runbot instance.
I have reinstalled pos and now it works fine, we couldn't find this error on local servers.
@pedroguirao

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@dsolanki-initos
Copy link
Contributor

@legalsylvain can we merge this PR?

@legalsylvain
Copy link
Contributor

No answer from @hparfr
Merging approved PR.
Raph feel free to amend the module if you think it's relevant.
Kind regards.

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 14.0-ocabot-merge-pr-750-by-legalsylvain-bump-patch, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Feb 25, 2022
@OCA-git-bot
Merge PR #750 into 14.0
cceba98 
Signed-off-by legalsylvain
@OCA-git-bot
Copy link
Contributor

@legalsylvain your merge command was aborted due to failed check(s), which you can inspect on this commit of 14.0-ocabot-merge-pr-750-by-legalsylvain-bump-patch.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

@legalsylvain
Copy link
Contributor

/ocabot rebase

@OCA-git-bot
Copy link
Contributor

Congratulations, PR rebased to 14.0.

@legalsylvain
Copy link
Contributor

@pedroguirao : xould you fix pre-commit error ?
Thanks !

@hparfr
Copy link
Contributor

hparfr commented Feb 25, 2022

Hi,

Don't have time to make a real review here. Just 2-3 remarks:

1- this line is suspcious: async getPtavAttributeId(ptav_id) { _.fo }
2-about style: it's better to not use _.each . There is equivalents in JS.
3- do not know if it's the rebase but the history of the PR is lost.I can't see what you added after my commits.
4- my goal was to support new uses cases when only a subset of variants exists (back in the day, the variants were a cartesian product of the attribute, it's not the case any more).
5- My PR has changed the module a lot, the current status is quite different of the original module, so I'm ok to merge this one instead.
6- I did not work on this subject since 1 year. I didn't follow precisely the new changes.

Raph feel free to amend the module if you think it's relevant.
Kind regards.

Indeed.

@pedroguirao
Copy link
Author

Thanks @legalsylvain @hparfr i will do asap

@pedroguirao
Copy link
Author

Sorry ! I just figure out I was running and non updated version of pre-commit!

@legalsylvain legalsylvain added this to the 14.0 milestone Mar 6, 2022
@dsolanki-initos

This comment was marked as spam.

Sign in to view
@antoniocanovas
Copy link

Please @pedroguirao review #789

@pedroguirao
Copy link
Author

Please @pedroguirao review #789

Reviewed, now productscreen takes the template img instead of variant one.

@dsolanki-initos
Copy link
Contributor

Hello @legalsylvain can we merge this PR?

@pedroguirao
Copy link
Author

Hi !
@elvise if im not mistake i have to amend the PR history besides that i think is finished, I will try to fix this week.

@elvise
Copy link

elvise commented May 31, 2022

Hi ! @elvise if im not mistake i have to amend the PR history besides that i think is finished, I will try to fix this week.

Hi @pedroguirao i tested in runboat and as i remember there was an error when try to add product (in POS), anyway let me post a video

@elvise
Copy link

elvise commented May 31, 2022

@pedroguirao please check this video: https://recordit.co/Qq1G4Ia36W

@pedroguirao
Copy link
Author

pedroguirao commented May 31, 2022 via email

@elvise
Copy link

elvise commented May 31, 2022

Ok let me check! curious because i have this versión running on a customer for a month El mar, 31 may 2022 a las 15:57, Frisbi @.>) escribió:
…
@pedroguirao https://github.com/pedroguirao please check this video: https://recordit.co/Qq1G4Ia36W — Reply to this email directly, view it on GitHub <#750 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJWG35YPQY5HL2IKLECHJN3VMYLFVANCNFSM5L6OLPMA . You are receiving this because you were mentioned.Message ID: @.>
-- Pedro José Baños Guirao @.*** @.***>ngenieriacloud.com
-- Antes de imprimir este e-mail piense bien si es necesario, la conservación del medio ambiente es tarea de todos. Este mensaje contiene información CONFIDENCIAL sometida a secreto profesional. Si lo ha recibido por error, debe saber que la copia, uso o divulgación están prohibidos la Ley, por ello, le rogamos que nos lo comunique por esta misma vía y proceda a su destrucción.  Muchas gracias.

Great! Please let me know when you have checked 👍

@elvise
Copy link

elvise commented Jun 2, 2022

@pedroguirao did you have time to check? :)

@pedroguirao
Copy link
Author

pedroguirao commented Jun 3, 2022 via email

@elvise
Copy link

elvise commented Jun 3, 2022

Yes! is the same issue reported by @enriquemartin months ago, but we can't
reproduce in our enviorments, we think is related to another module or
runboat enviorment.

El jue, 2 jun 2022 a las 19:31, Frisbi @.***>)
escribió:

@pedroguirao https://github.com/pedroguirao did you have time to check?
:)

—
Reply to this email directly, view it on GitHub
#750 (comment), or
unsubscribe
https://github.com/notifications/unsubscribe-auth/AJWG355DDGPMZBDX5ZXOEWDVNDVXHANCNFSM5L6OLPMA
.
You are receiving this because you were mentioned.Message ID:
@.***>

--

Pedro José Baños Guirao
@.*** @.***>ngenieriacloud.com

--

Antes de imprimir este e-mail piense bien si es necesario, la
conservación del medio ambiente es tarea de todos.
Este mensaje contiene
información CONFIDENCIAL sometida a secreto profesional. Si lo ha recibido
por error, debe saber que la copia, uso o divulgación están prohibidos la
Ley, por ello, le rogamos que nos lo comunique por esta misma vía y proceda
a su destrucción. 

Muchas gracias.

Thanks! @pedroguirao Do you any plan for fix this PR for get the merge ?

@pedroguirao
Copy link
Author

pedroguirao commented Jun 3, 2022 via email

@elvise
Copy link

elvise commented Jun 3, 2022

OFC but actually I'm not sure about what is needed to fix, I suppose the
history of the PR but need some guidance of how to fix that. Im looking for
"how to" the past days sorry.

El vie, 3 jun 2022 a las 13:23, Frisbi @.***>)
escribió:

Yes! is the same issue reported by @enriquemartin
https://github.com/enriquemartin months ago, but we can't
reproduce in our enviorments, we think is related to another module or
runboat enviorment.

El jue, 2 jun 2022 a las 19:31, Frisbi @.***>)
escribió:

@pedroguirao https://github.com/pedroguirao
https://github.com/pedroguirao did you have time to check?
:)

—
Reply to this email directly, view it on GitHub
#750 (comment)
#750 (comment), or
unsubscribe

https://github.com/notifications/unsubscribe-auth/AJWG355DDGPMZBDX5ZXOEWDVNDVXHANCNFSM5L6OLPMA
.
You are receiving this because you were mentioned.Message ID:
@.***>

--

Pedro José Baños Guirao
@.*** @.***>ngenieriacloud.com

--

Antes de imprimir este e-mail piense bien si es necesario, la
conservación del medio ambiente es tarea de todos.
Este mensaje contiene
información CONFIDENCIAL sometida a secreto profesional. Si lo ha recibido
por error, debe saber que la copia, uso o divulgación están prohibidos la
Ley, por ello, le rogamos que nos lo comunique por esta misma vía y proceda
a su destrucción.

Muchas gracias.

Thanks! @pedroguirao https://github.com/pedroguirao Do you any plan for
fix this PR for get the merge ?

—
Reply to this email directly, view it on GitHub
#750 (comment), or
unsubscribe
https://github.com/notifications/unsubscribe-auth/AJWG357TAVYVZH6X6DGEQJDVNHTJXANCNFSM5L6OLPMA
.
You are receiving this because you were mentioned.Message ID:
@.***>

--

Pedro José Baños Guirao
@.*** @.***>ngenieriacloud.com

--

Antes de imprimir este e-mail piense bien si es necesario, la
conservación del medio ambiente es tarea de todos.
Este mensaje contiene
información CONFIDENCIAL sometida a secreto profesional. Si lo ha recibido
por error, debe saber que la copia, uso o divulgación están prohibidos la
Ley, por ello, le rogamos que nos lo comunique por esta misma vía y proceda
a su destrucción. 

Muchas gracias.

@enriquemartin any suggestions ? :)

@PierrickBrun
Copy link
Contributor

Hi @pedroguirao , thanks for this work.

Can you integrate this commit to the PR ? akretion@7b9c607

I don't know why but I can't do a PR against your repository.

@francesco-ooops
Copy link
Contributor

francesco-ooops commented Jun 7, 2022 •
edited
Loading

OFC but actually I'm not sure about what is needed to fix, I suppose the history of the PR but need some guidance of how to fix that. Im looking for "how to" the past days sorry.

Hi @pedroguirao , you should find everything here: https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-14.0

@pedroguirao
Copy link
Author

Hi @PierrickBrun Thanks! i tried to cherry-pick it but gives me an error so I made the change manually if you are ok.

@francesco-ooops , thanks too, but I read the migrations steps and this case is not explained. Or at least i didn't see it.

@PierrickBrun
Copy link
Contributor

Hi @PierrickBrun Thanks! i tried to cherry-pick it but gives me an error so I made the change manually if you are ok.

@francesco-ooops , thanks too, but I read the migrations steps and this case is not explained. Or at least i didn't see it.

no problem

@francesco-ooops
Copy link
Contributor

@francesco-ooops , thanks too, but I read the migrations steps and this case is not explained. Or at least i didn't see it.

@pedrobaeza not the first time I get feedback from a dev having issues applying migration instructions, could you help @pedroguirao out? Thank you!

@pedrobaeza
Copy link
Member

Which is the problem?

@pedroguirao
Copy link
Author

pedroguirao commented Jun 7, 2022 via email

@pedrobaeza
Copy link
Member

pedrobaeza commented Jun 7, 2022 •
edited
Loading

Here are the steps to get the commit history:

https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-14.0#technical-method-to-migrate-a-module-from-130-to-140-branch

But this should be done from the beginning.

@elvise
Copy link

elvise commented Jun 8, 2022

Here are the steps to get the commit history:

https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-14.0#technical-method-to-migrate-a-module-from-130-to-140-branch

But this should be done from the beginning.

@pedroguirao what do you think about this link ? :)

@dsolanki-initos
Copy link
Contributor

@pedroguirao can you please check?

@elvise
Copy link

elvise commented Jun 12, 2022

@pedroguirao can you check ?

@francesco-ooops
Copy link
Contributor

@pedroguirao do you think you have time to allocate to this task? We're always available if you need help

@dsolanki-initos
Copy link
Contributor

@pedroguirao can you please check?

@pedroguirao
Copy link
Author

@dsolanki-initos @elvise @francesco-ooops

Hi all, the steps indicated on https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-14.0#technical-method-to-migrate-a-module-from-130-to-140-branch were made from the beginning as @pedrobaeza says, so if this PR is broken i can close an make a new one from the beginning, it's the fastest solutions but not sure if this is OK for you all.

@pedrobaeza
Copy link
Member

@pedroguirao the problem may be that you are doing the migration jumping one version in between. If that's the case, you need to adapt the commands to say 12.0 instead of 13.0 as the source branch.

@pedroguirao
Copy link
Author

@pedrobaeza that's it! sorry newbie in contributions, Ok, so I can do the format-patch again and amend or better I do a new PR?.
Thanks for your time.

@pedrobaeza
Copy link
Member

You should start from scratch, but save current code, and when you finish the procedure, just remove the existing folder and put the one you save, and commit the result.

@pedroguirao pedroguirao mentioned this pull request Jun 20, 2022
Merged
@pedroguirao
Copy link
Author

Hi all @legalsylvain , @dsolanki-initos , @elvise , @francesco-ooops , @pedrobaeza and thanks for the support.

Just done the process from the beginning en here is the result : #800.

I think this PR can be closed.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@enriquemartin enriquemartin enriquemartin approved these changes

@dsolanki-initos dsolanki-initos dsolanki-initos approved these changes

Assignees
No one assigned
Labels
approved question
Projects
None yet
Milestone
14.0
Development

Successfully merging this pull request may close these issues.