-
-
Notifications
You must be signed in to change notification settings - Fork 234
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
[FIX] stock_quantity_history_location: Migrate js to OWL #282
[FIX] stock_quantity_history_location: Migrate js to OWL #282
Conversation
This is in development phase |
Hi @luisg123v, |
77b0c2c
to
9eab428
Compare
Hi, @luisg123v, @imanie383 and @desdelinux, please, could you review this PR? Do you know anything about the lint in the pre-commit? |
Hi @sebasdrk17, Could you review, please? |
"stock_quantity_history_location/static/src/components" | ||
"/inventory_report/inventory_report.js", | ||
"stock_quantity_history_location/static/src/components/" | ||
"inventory_report//inventory_report.xml", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra slash
@@ -0,0 +1,26 @@ | |||
<?xml version="1.0" encoding="UTF-8" ?> | |||
<templates> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant ID:
<templates> | |
<templates id="template" xml:space="preserve"> |
<templates> | ||
<t t-inherit="stock.InventoryReport.Buttons" t-inherit-mode="extension" owl="1"> | ||
<xpath expr="//button[contains(@t-if, 'stock.quant')]" position="attributes"> | ||
<attribute name="t-if" add="and !this.multi_location" separator=" " /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no safe way to add conditions to an attribute, as order is not guaranteed
I'd suggest creating a <t t-if
element and moving button there, and inserting the new button just after that element using t-else
"stock_quantity_history_location/static/src/xml/inventory_report.xml", | ||
"stock_quantity_history_location/static/src/components" | ||
"/inventory_report/inventory_report.js", | ||
"stock_quantity_history_location/static/src/components/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are not importing any files in L22
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was importing the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@imanie383 to clarify, this is a multiline string, next line contains file name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know, but @luisg123v prefers explicit file paths when there are few files.
this._super(...arguments); | ||
this.multi_location = false; | ||
onWillStart(async () => { | ||
this.multi_location = await session.user_has_group( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
9eab428
to
c19fc50
Compare
Hi, @luisg123v and @imanie383, I applied your suggestions. |
@luisg123v Am I still needed here? I've seen that @imanie383 has already review |
@sebasdrk17 maybe not as needed anymore, but any comment is welcomed anyway. |
<?xml version="1.0" encoding="UTF-8" ?> | ||
<templates id="template" xml:space="preserve"> | ||
<t t-inherit="stock.InventoryReport.Buttons" t-inherit-mode="extension" owl="1"> | ||
<xpath expr="(//div/*)[last()]" position="after"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK this is inserting elements at the end, even after the "Appli All" button.
Maybe what you need is to place this before the "Apply All" button, by filtering by t-on-click="onClickApplyAll"
.
!props.context.inventory_mode | ||
or props.context.inventory_report_mode | ||
) | ||
and !props.context.no_at_date |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add an ID to this button, so if we (or anybody else) migrate the module stock_account_quantity_history_location
, xpaths are simpler
@@ -0,0 +1,17 @@ | |||
/** @odoo-module */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/** @odoo-module */ | |
/** @odoo-module **/ |
b89296a
to
3226a7c
Compare
Hi, @luisg123v and @imanie383, I applied your suggestions. |
2132463
to
dd88aeb
Compare
3226a7c
to
8325609
Compare
Hi, @luisg123v and @imanie383, could you review this PR again? CI is green now |
<t t-else="" name="has_not_multi_location" /> | ||
</button> | ||
<t name="has_not_multi_location" position="inside"> | ||
<xpath expr="//button[contains(@t-if, 'stock.quant')]" position="move" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, I'm not sure why you need another button if your button is exactly the same as the original button(if and click-method)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, hasclass('o_button_at_date') is more ambiguous because stock_account add another button with that class, actually using the model is more specific for every button. What do you think? (This module will work together with stock_account_quantity_history_location if stock_account_quantity_history_location is migrated).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @rolandojduartem, current filter is preferred to using class.
Regarding this:
Wait, I'm not sure why you need another button if your button is exactly the same as the original button(if and click-method)
Because the text is different and there's no way to change or hide current text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can use a t-out like
<button t-out="'demo'">imanie</button>
that overwrites the inner text
<templates id="template" xml:space="preserve"> | ||
<t t-inherit="stock.InventoryReport.Buttons" t-inherit-mode="extension" owl="1"> | ||
<button t-on-click="onClickApplyAll" position="before"> | ||
<t t-if="this.multi_location" name="has_multi_location"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This commit solves the error "Non loaded module stock_quantity_history_location.InventoryReportListController" because the dependency stock.InventoryReportListController was missed. The dependency from stock was migrated to OWL [1], so it is inherited using the OWL to preserve the behavior. References: - [1] odoo/odoo@ec0b1005
8325609
to
5ae2895
Compare
Hi, @luisg123v and @imanie383, what do you think now? I could not make the groups attribute works for this inheritance, it would be great to remove the js file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
/ocabot merge nobump
Not increasing version because this change is already increasing it.
This PR looks fantastic, let's merge it! |
Congratulations, your PR was merged at 928abb2. Thanks a lot for contributing to OCA. ❤️ |
This commit solves the error
"Non loaded module
stock_quantity_history_location.InventoryReportListController" because
the dependency stock.InventoryReportListController was missed. The
dependency from stock was migrated to OWL [1], so it is inherited using the
OWL to preserve the behavior.
References: