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

[16.0][MIG] web_widget_x2many_2d_matrix #2463

Merged
merged 2 commits into from
Jun 7, 2023

Conversation

tarteo
Copy link
Member

@tarteo tarteo commented Mar 21, 2023

No description provided.

@tarteo
Copy link
Member Author

tarteo commented Mar 21, 2023

/ocabot migration web_widget_x2_many_2d_matrix

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Mar 21, 2023
@OCA-git-bot OCA-git-bot mentioned this pull request Mar 21, 2023
46 tasks
@tarteo
Copy link
Member Author

tarteo commented Mar 21, 2023

I'll also migrate the timesheet sheet module (hr_timesheet_sheet).

Copy link
Member

@astirpe astirpe left a 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 😉

web_widget_x2many_2d_matrix/__manifest__.py Outdated Show resolved Hide resolved
@legalsylvain
Copy link
Contributor

@tarteo tarteo force-pushed the 16-mig-matrix-widget branch from 1e01f57 to 36f816e Compare March 22, 2023 13:17
@tarteo tarteo changed the title [16.0][MIG] web_widget_x2_many_2d_matrix [16.0][MIG] web_widget_x2many_2d_matrix Mar 22, 2023
@tarteo tarteo force-pushed the 16-mig-matrix-widget branch 2 times, most recently from 7af8d48 to 8617984 Compare March 22, 2023 14:32
Copy link
Member

@astirpe astirpe left a 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!

@tarteo
Copy link
Member Author

tarteo commented Mar 28, 2023

@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"
Copy link

@sonhd91 sonhd91 Apr 25, 2023

Choose a reason for hiding this comment

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

Suggested change
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

  1. Create wizard like in README
  2. Add action to menu-action open the wizard form, error showup

Copy link
Member Author

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be fixed now

Copy link
Member

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?

@Saran440
Copy link
Member

Hi,

In v16, It error
Selection_011

with my code

<field
    name="line_ids"
    widget="x2many_2d_matrix"
    field_x_axis="date_range_id"
    field_y_axis="name"
    field_value="amount"
>
    <tree>
        <field name="date_range_id" />
        <field name="name" />
        <field name="amount" />
    </tree>
</field>

result in v15
Selection_012

@PieterPaulussen
Copy link

Code review: Broken
I've tested the module thoroughly and support for a Many2one value field is missing. I've created a PR to address the issue here, although I personally think it still falls short of the features it should have.

Features that need be restored/added/updated:

  • Add possibility to specify the aggregation field type (int, float, ...). Current code version uses the same field type to display the aggregation value. There is also no check to validate if the row or column CAN be aggregated at all (which seemed to be present in older versions?)
  • The use of the word list seems confusing. I've refactored the use of list to matrixRows which seems more appropriate and clarifies its purpose.

@tarteo
Copy link
Member Author

tarteo commented May 10, 2023

@PieterPaulussen You realize I did a complete rewrite of this field widget?

Add possibility to specify the aggregation field type (int, float, ...). Current code version uses the same field type to display the aggregation value. There is also no check to validate if the row or column CAN be aggregated at all (which seemed to be present in older versions?)

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...

The use of the word list seems confusing. I've refactored the use of list to matrixRows which seems more appropriate and clarifies its purpose.

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.

@tarteo
Copy link
Member Author

tarteo commented May 10, 2023

@Saran440 Thanks I'll look into it.

@tarteo tarteo force-pushed the 16-mig-matrix-widget branch from 8617984 to 33b5aa7 Compare May 11, 2023 12:21
@tarteo
Copy link
Member Author

tarteo commented May 11, 2023

Hey @Saran440 It should be fixed now

@tarteo tarteo force-pushed the 16-mig-matrix-widget branch 2 times, most recently from b9d3f0e to feee650 Compare May 11, 2023 12:58
Copy link
Member

@Saran440 Saran440 left a comment

Choose a reason for hiding this comment

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

Functional Test 👍

@ghost
Copy link

ghost commented May 28, 2023

Functional test with web_widget_x2many_2d_matrix_example: 👍 for simple field. 👎 for selection and many2one fields.

@tarteo
Copy link
Member Author

tarteo commented May 30, 2023

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

Copy link
Member

@ernestotejeda ernestotejeda left a 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 👍🏼

@pedrobaeza
Copy link
Member

@tarteo are you going to check the m2o case?

@tarteo
Copy link
Member Author

tarteo commented Jun 7, 2023

@pedrobaeza yes

@pedrobaeza
Copy link
Member

Great, thanks.

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-2463-by-pedrobaeza-bump-nobump, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Jun 7, 2023
Signed-off-by pedrobaeza
@MiquelRForgeFlow
Copy link
Contributor

MiquelRForgeFlow commented Jun 7, 2023

Merge should be stopped and restarted. @pedrobaeza

@OCA-git-bot
Copy link
Contributor

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

@MiquelRForgeFlow
Copy link
Contributor

@sbidoul we should implement an option of /ocabot merge stop for these cases

@OCA-git-bot
Copy link
Contributor

Hi @MiquelRForgeFlow. Your command failed:

Invalid options for command merge: stop for these cases.

Ocabot commands

  • ocabot merge major|minor|patch|nobump
  • ocabot rebase
  • ocabot migration {MODULE_NAME}

More information

@MiquelRForgeFlow
Copy link
Contributor

@tarteo a new PR should be done to add your last commit!

@tarteo
Copy link
Member Author

tarteo commented Jun 7, 2023

Just pushed a fix for many2one functionality

@tarteo tarteo reopened this Jun 7, 2023
@OCA-git-bot
Copy link
Contributor

Hi @ChrisOForgeFlow,
some modules you are maintaining are being modified, check this out!

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

@tarteo tarteo force-pushed the 16-mig-matrix-widget branch from d32fa9b to 476fe9a Compare June 7, 2023 14:12
@tarteo
Copy link
Member Author

tarteo commented Jun 7, 2023

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 16.0-ocabot-merge-pr-2463-by-tarteo-bump-minor, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 88c5d19 into OCA:16.0 Jun 7, 2023
@OCA-git-bot
Copy link
Contributor

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

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.

10 participants