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

[17.0][MIG] stock_secondary_unit: Migration to 17.0 #2121

Merged
merged 51 commits into from
Jan 20, 2025

Conversation

rov-adhoc
Copy link

No description provided.

@rov-adhoc rov-adhoc force-pushed the 17.0-mig-stock_secondary_unit branch from 489e38d to 7e66877 Compare July 30, 2024 11:11
@rov-adhoc rov-adhoc force-pushed the 17.0-mig-stock_secondary_unit branch from 7e66877 to 3ba5a7a Compare October 7, 2024 13:26
@rov-adhoc
Copy link
Author

@rousseldenis Hey! I’d like to ask if there’s anything missing for it to be approved. Thanks in advance! Best regards!

@rousseldenis
Copy link
Contributor

/ocabot migration stock_secondary_unit

@OCA-git-bot OCA-git-bot added this to the 17.0 milestone Oct 7, 2024
@rousseldenis
Copy link
Contributor

@rousseldenis Hey! I’d like to ask if there’s anything missing for it to be approved. Thanks in advance! Best regards!

Reviews :-)

@rov-adhoc rov-adhoc force-pushed the 17.0-mig-stock_secondary_unit branch 2 times, most recently from 4d546d3 to 0bb3672 Compare October 7, 2024 14:54
@pilarvargas-tecnativa
Copy link
Contributor

Please include #2080

@pilarvargas-tecnativa
Copy link
Contributor

Sorry but I have seen that there are 2 other PRs for this migration, please indicate if this one overwrites them to be closed.

@rov-adhoc
Copy link
Author

@pilarvargas-tecnativa Hey! I just added it!

@rov-adhoc
Copy link
Author

@fredericgrall Hey! Would you mind closing your PR and reviewing this one when you have a moment? I’d really appreciate it. Thanks in advance!

Copy link
Contributor

@rousseldenis rousseldenis left a comment

Choose a reason for hiding this comment

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

LGTM

@rov-adhoc
Copy link
Author

@pilarvargas-tecnativa Hi!! I would like to ask you if you can review this PR.
Thanks in advance

@rov-adhoc rov-adhoc force-pushed the 17.0-mig-stock_secondary_unit branch from a9ed261 to 984cec6 Compare October 17, 2024 14:09
Copy link

@lav-adhoc lav-adhoc left a comment

Choose a reason for hiding this comment

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

LGTM

@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). 🤖

@rov-adhoc
Copy link
Author

@rousseldenis HI!! I would like to ask you if is something missing out? Regards

@rov-adhoc rov-adhoc force-pushed the 17.0-mig-stock_secondary_unit branch from a52533a to 960991e Compare January 8, 2025 12:20
@rov-adhoc
Copy link
Author

@rousseldenis HI!! I would like to ask you if is something missing out? Regards

Hello, it remains to answer my previous question. Regards

image

I just corrected it

@rov-adhoc
Copy link
Author

@rov-adhoc Could you take a look at #2121 (comment) and check if everything is correct, or should I open a new PR?

I just added it

@pilarvargas-tecnativa
Copy link
Contributor

I just added it

Hi @rov-adhoc ,

The comment to which @juancarlosonate-tecnativa refers is the one about the xpath error: https://github.com/OCA/stock-logistics-warehouse/pull/2121/files#r1843723939

You have marked it as resolved but the error persists. With an incorrect xpath the fields are duplicated and are not visible in the mobile view, which was the purpose of the commit I mentioned when you removed this part of the code. When you add this code again, which has its purpose, you have to adapt it so that it can be seen correctly in mobile views.

image

image

This is v16 and is how it should look in mobile view:

image

@rov-adhoc rov-adhoc force-pushed the 17.0-mig-stock_secondary_unit branch from 960991e to 31d1b2b Compare January 8, 2025 15:16
@rov-adhoc
Copy link
Author

I just added it

Hi @rov-adhoc ,

The comment to which @juancarlosonate-tecnativa refers is the one about the xpath error: https://github.com/OCA/stock-logistics-warehouse/pull/2121/files#r1843723939

You have marked it as resolved but the error persists. With an incorrect xpath the fields are duplicated and are not visible in the mobile view, which was the purpose of the commit I mentioned when you removed this part of the code. When you add this code again, which has its purpose, you have to adapt it so that it can be seen correctly in mobile views.

image

image

This is v16 and is how it should look in mobile view:

image

Thank you for providing more details! Now I fully understand what you mean. I’ve made the necessary corrections, and I hope everything is in order now

Copy link
Contributor

@pilarvargas-tecnativa pilarvargas-tecnativa left a comment

Choose a reason for hiding this comment

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

Good! Thanks!

@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). 🤖

Copy link

@juancarlosonate-tecnativa juancarlosonate-tecnativa left a comment

Choose a reason for hiding this comment

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

in the product form, the action_update_quantity_on_hand appears duplicated

31d1b2b#diff-0ed52cda6c2ce263d32165b3d1b7998595886c080423e3e66de63ff19b4a12f1

image

Comment on lines 24 to 37
ref="stock.product_template_form_view_procurement_button"
/>
<field name="arch" type="xml">
<xpath expr="//button[@name='action_open_quants']" position="after">
<xpath
expr="//button[@name='action_update_quantity_on_hand']"
position="after"
>
<button
type="object"
name="action_open_quants"
attrs="{'invisible':[('type', '!=', 'product')]}"
name="action_update_quantity_on_hand"
invisible="type != 'product'"
class="oe_stat_button"
icon="fa-building-o"
groups="stock.group_stock_user"

Choose a reason for hiding this comment

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

something must be wrong here because the button shouldnt look like this

Copy link
Author

Choose a reason for hiding this comment

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

Can you please check again?

Choose a reason for hiding this comment

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

i don't see any changes

Copy link
Author

Choose a reason for hiding this comment

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

image

Choose a reason for hiding this comment

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

Look at v16

Selección_001

Copy link
Author

Choose a reason for hiding this comment

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

It appears to be quite similar. Let me check how it behaves when a secondary unit is set for this product
image

Choose a reason for hiding this comment

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

similar but not the same, the xpath is not correct and neither is the action, i think
image

Copy link
Author

Choose a reason for hiding this comment

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

Please check again! Hope now should be ok

@rov-adhoc rov-adhoc force-pushed the 17.0-mig-stock_secondary_unit branch from 31d1b2b to 7524309 Compare January 9, 2025 11:19
@rov-adhoc rov-adhoc force-pushed the 17.0-mig-stock_secondary_unit branch from 7524309 to 6ad41fc Compare January 10, 2025 12:03
@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). 🤖

@pedrobaeza
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 17.0-ocabot-merge-pr-2121-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at b446469. Thanks a lot for contributing to OCA. ❤️

@OCA-git-bot OCA-git-bot merged commit d77d6ba into OCA:17.0 Jan 20, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.