-
-
Notifications
You must be signed in to change notification settings - Fork 339
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
[16.0] [MIG] stock_barcodes #587
Conversation
fdee2ca
to
43de03a
Compare
Functional testing: Set this barcode option on incoming picking type. Configura one product tracking as serial number. Purchase this product to create a incoming picking. Go to incoming picking and get this error trying to open barcode interface: Traceback (most recent call last): |
43de03a
to
524c3b0
Compare
5afdd8a
to
e873c36
Compare
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.
Hi @FranzPoize Thanks for your work here.
I didn't go deep into the python logic, although it looks correct at first sight.
As a general comment, I'm finding hard to review the changes in the javascript logic:
- There are no diffs
- There's missing documentation from the former code and for the new it isn't explained.
- There are overrides that copy the original method (a thus the inheritance is broken for those)
Some more specific comments:
stock_barcodes/__manifest__.py
Outdated
"author": "Tecnativa, " "Odoo Community Association (OCA)", | ||
"website": "https://github.com/OCA/stock-logistics-barcode", | ||
"license": "AGPL-3", | ||
"category": "Extra Tools", | ||
"depends": ["barcodes", "stock", "web_widget_numeric_step"], | ||
"depends": ["barcodes", "stock", "web_widget_numeric_step", "web"], |
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.
web_widget_numeric_step
already depends on web
</div> | ||
<div class="col-2"> | ||
<span | ||
class="pull-right text-right text-muted" |
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.
Review your bootstrap classes as they should be migrated to BS5: https://getbootstrap.com/docs/5.0/migration/
You've got some tooling in openupgradelib
to analyze your view code: https://github.com/OCA/openupgradelib/blob/master/openupgradelib/openupgrade_160.py
/* Copyright 2022 Tecnativa - Alexandre D. Díaz | ||
* License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl). */ | ||
|
||
const barcodeModels = [ |
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.
const barcodeModels = [ | |
export const barcodeModels = [ |
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.
Yes thank you !
]; | ||
|
||
export function isAllowedBarcodeModel(modelName) { | ||
return barcodeModels.indexOf(modelName) !== -1; |
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.
A little bit more expressive:
return barcodeModels.indexOf(modelName) !== -1; | |
return barcodeModels.includes(modelName); |
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.
Thank you ! Changed
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.
Organize these source files at leat in functionality folders so it's more clear where to look what
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.
Tried to. Tell me if you thinks that's better
"wiz.stock.barcodes.read.todo", | ||
]; | ||
|
||
export function isAllowedBarcodeModel(modelName) { |
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 also had a docstring
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.
Added
import {registry} from "@web/core/registry"; | ||
|
||
class BarcodeBooleanToggleField extends BooleanToggleField { | ||
onChange(newValue) { |
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.
Missing documentation from the original here as well where it was stated why this was needed...
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.
Added !
import {patch} from "@web/core/utils/patch"; | ||
|
||
patch(FormController.prototype, "Allow display.controlPanel overriding", { | ||
setup() { |
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.
Docs missing here as well
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.
Added !
// eslint-disable-next-line complexity | ||
focusNextCard(area, direction) { | ||
const {isGrouped} = this.props.list; | ||
const closestCard = document.activeElement.closest(".o_kanban_record"); | ||
if (!closestCard) { | ||
return; | ||
} | ||
const groups = isGrouped | ||
? [...area.querySelectorAll(".o_kanban_group")] | ||
: [area]; | ||
let cards = [...groups] | ||
.map((group) => [...group.querySelectorAll(".o_kanban_record")]) | ||
.filter((group) => group.length); | ||
|
||
if (isAllowedBarcodeModel(this.props.list.resModel)) { |
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.
Why do yo need to copy the whole method if you're returning in the middle of it? Maybe you have to repeat some of it if it's encapsulated, but in the end you should try to return super
. Just evaluate this first, and repeat as less as possible
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.
The code is not returning in the if
statement it's recomputing the cards
variable.
I could call super and return its value but I would still have duplicate that code because nothing is returned from the parent function. I still need to compute the candidate cards
still filter them and focus the correct one.
If you have a way to do it better I would need more information because I'm not sure how to go about it.
const rootRef = useRef("root"); | ||
useHotkey( | ||
"Enter", |
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.
What's the purpose of this?
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.
Not sure what you're referring to ? the rootRef or the useHotkey ?
Thank you @chienandalu for your review
Yes unfortunately the differences in behavior between the legacy front and owl are truly unreconcialable...
I hope it's fixed in the latest push
Yes that is not inevitable but is the least ugly way to do what needs to be done I'll go in more detail in the review comment |
e873c36
to
667f9d6
Compare
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.
Thanks @FranzPoize take also in consideration that there are lots of recent changes in the 15.0 that should be included as a base to the migration as well...
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.
Also testing a manual entry, I found this error
File "/opt/odoo/auto/addons/stock_barcodes/wizard/stock_barcodes_read.py", line 744, in action_confirm
res = self.action_done()
File "/opt/odoo/auto/addons/stock_barcodes/wizard/stock_barcodes_read_picking.py", line 269, in action_done
).action_done()
File "/opt/odoo/auto/addons/stock_barcodes/wizard/stock_barcodes_read.py", line 547, in action_done
if not self.check_done_conditions():
File "/opt/odoo/auto/addons/stock_barcodes/wizard/stock_barcodes_read_picking.py", line 670, in check_done_conditions
and float_compare(
File "/opt/odoo/custom/src/odoo/odoo/tools/float_utils.py", line 155, in float_compare
rounding_factor = _float_check_precision(precision_digits=precision_digits,
File "/opt/odoo/custom/src/odoo/odoo/tools/float_utils.py", line 29, in _float_check_precision
assert precision_rounding is None or precision_rounding > 0,\
AssertionError: precision_rounding must be positive, got 0.0
The above server error caused the following client error:
RPC_ERROR: Odoo Server Error
at makeErrorFromResponse (http://localhost:16069/web/assets/3930-e51fdb0/web.assets_backend.min.js:993:163)
at XMLHttpRequest.<anonymous> (http://localhost:16069/web/assets/3930-e51fdb0/web.assets_backend.min.js:1001:13)
Yes this PR includes all the changes up to 28th of february not sure I'll have the bandwidth to add the more recent changes if somebody can help with that I'd be glad. I'll fix the codecov issue |
We can do this: finish your ongoing fixes and then we can supersed the migration from the 15.0 branch and putting on top your migration commit fixing conflicts and finishing whatever needs to be fixed from there. What do you think? |
} | ||
|
||
def _create_new_lot(self): | ||
StockProductionLot = self.env["stock.production.lot"] |
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.
Hi, you need to change this model to stock.lot
StockProductionLot = self.env["stock.lot"]
667f9d6
to
64b56e0
Compare
Hi, @FranzPoize what do you think about what I proposed?:
|
7ed52ba
to
5f4dde5
Compare
When user creates a lot, set default product the wizard selected product. When confirm the lot creation wizard, set this lot in lot wizard scanning barcode field.
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: stock-logistics-barcode-11.0/stock-logistics-barcode-11.0-stock_barcodes Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-barcode-11-0/stock-logistics-barcode-11-0-stock_barcodes/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: stock-logistics-barcode-15.0/stock-logistics-barcode-15.0-stock_barcodes Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-barcode-15-0/stock-logistics-barcode-15-0-stock_barcodes/
…t create lot option is checked. Display notification for required field.
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: stock-logistics-barcode-15.0/stock-logistics-barcode-15.0-stock_barcodes Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-barcode-15-0/stock-logistics-barcode-15-0-stock_barcodes/
… order by location or location_dest_id
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: stock-logistics-barcode-15.0/stock-logistics-barcode-15.0-stock_barcodes Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-barcode-15-0/stock-logistics-barcode-15-0-stock_barcodes/
5f4dde5
to
6bda4d3
Compare
@chienandalu When you use the barcode button on a picking type. It calls
So during
This means no step at the end in some case (which are real convoluted but not so much).
Or simpler use the demo Picking internals barcode option on internal transfer and click on the barcode on the internal transfer picking type. |
/ocabot migration stock_barcodes |
Found the commit that adds a new call to This seems to break the option/step finding since if there is no My question is it suppose to work without a picking_id in the |
cbe6ee6
to
95f3aa3
Compare
To be taken into account: #605 |
The frontend has been completely redone this should be resolved in 16.0
Le jeu. 2 mai 2024 à 12:56, Pedro M. Baeza ***@***.***> a
écrit :
… To be taken into account: #605
<#605>
—
Reply to this email directly, view it on GitHub
<#587 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAHNHZYLAG6HRJBVBNBBTRTZAILU3AVCNFSM6AAAAABD6PUX2WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJQGIYDCOJRGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
To be taken into account: #604 |
The next week in Spanish OCA days we will work about it... |
95f3aa3
to
3828828
Compare
Superseded by #610, that was the result of the Spanish OCA days, |
No description provided.