-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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] web_widget_x2many_2d_matrix #2463
Conversation
/ocabot migration web_widget_x2_many_2d_matrix |
I'll also migrate the timesheet sheet module (hr_timesheet_sheet). |
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.
@tarteo thank you for this great work! The code looks very clean now.
We are all eager to test it with the timesheet sheet module 😉
Hi @tarteo. Thanks for porting this usefull module.
Thanks ! |
1e01f57
to
36f816e
Compare
7af8d48
to
8617984
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.
Code review: LGTM!
Functional test: by testing the timesheet sheet widget of OCA/timesheet#580 I implicitly tested this matrix module and it works properly
Thanks!
@legalsylvain I'm currently working on something else so I don't have time to implement extra features now. I think some PRs fixing things are already fixed by the migration. Can you please review the migation as is. Thanks! 😄 |
matrixFields="props.matrixFields" | ||
showRowTotals="props.showRowTotals" | ||
showColumnTotals="props.showColumnTotals" | ||
setDirty="props.setDirty" |
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.
setDirty="props.setDirty" | |
setDirty="(isDirty) => this.setDirty(isDirty)" |
fixed this error
`UncaughtPromiseError > OwlError
Uncaught Promise > Invalid props for component 'X2Many2DMatrixRenderer': 'setDirty' is undefined (should be a function)
OwlError: Invalid props for component 'X2Many2DMatrixRenderer': 'setDirty' is undefined (should be a function)
at Object.validateProps (http://localhost:8069/web/assets/debug/web.assets_common.js:19239:19) (/web/static/lib/owl/owl.js:3069)
at X2Many2DMatrixField.template (eval at compile (http://localhost:8069/web/assets/debug/web.assets_common.js:21606:16), <anonymous>:11:13) (/web/static/lib/owl/owl.js:5436)
at Fiber._render (http://localhost:8069/web/assets/debug/web.assets_common.js:17791:38) (/web/static/lib/owl/owl.js:1621)
at Fiber.render (http://localhost:8069/web/assets/debug/web.assets_common.js:17783:18) (/web/static/lib/owl/owl.js:1613)
at ComponentNode.initiateRender (http://localhost:8069/web/assets/debug/web.assets_common.js:18505:23) (/web/static/lib/owl/owl.js:2335)`
Reproduce
- Create wizard like in README
- Add action to menu-action open the wizard form, error showup
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! I'll fix it.
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.
Should be fixed now
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.
@sonhd91 is it ok for you now? Can you approve this PR?
Code review: Broken Features that need be restored/added/updated:
|
@PieterPaulussen You realize I did a complete rewrite of this field widget?
This is certainly not the case in previous versions. You can't differ from the aggregated field type. Checking if the field can be aggregated is literally one line of code, please calm down...
list is not the matrix it's a list of records. there's a different variable for it called "matrix" in X2Many2DMatrixRenderer. Lists are different from matrices. |
@Saran440 Thanks I'll look into it. |
8617984
to
33b5aa7
Compare
Hey @Saran440 It should be fixed now |
b9d3f0e
to
feee650
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.
Functional Test 👍
Functional test with web_widget_x2many_2d_matrix_example: 👍 for simple field. 👎 for selection and many2one fields. |
@appstogrow Thanks I think I can look at it this week. Using your migration of the example module, thanks! I only tested it with the timesheet sheet 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.
Tested on web_widget_x2many_2d_matrix_example #2520 👍🏼
@tarteo are you going to check the m2o case? |
@pedrobaeza yes |
Great, thanks. /ocabot merge nobump |
What a great day to merge this nice PR. Let's do it! |
Merge should be stopped and restarted. @pedrobaeza |
Congratulations, your PR was merged at 749d0f5. Thanks a lot for contributing to OCA. ❤️ |
@sbidoul we should implement an option of /ocabot merge stop for these cases |
Hi @MiquelRForgeFlow. Your command failed:
Ocabot commands
More information
|
@tarteo a new PR should be done to add your last commit! |
Just pushed a fix for many2one functionality |
Hi @ChrisOForgeFlow, |
This PR has the |
d32fa9b
to
476fe9a
Compare
/ocabot merge minor |
This PR looks fantastic, let's merge it! |
Congratulations, your PR was merged at 999c085. Thanks a lot for contributing to OCA. ❤️ |
No description provided.