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] mis_builder #607

Merged
merged 16 commits into from
Jul 8, 2024
Merged

[17.0][MIG] mis_builder #607

merged 16 commits into from
Jul 8, 2024

Conversation

mpascuall
Copy link
Contributor

@mpascuall mpascuall commented Apr 30, 2024

sbidoul and others added 4 commits April 30, 2024 08:33
Currently translated at 100.0% (281 of 281 strings)

Translation: mis-builder-16.0/mis-builder-16.0-mis_builder
Translate-URL: https://translation.odoo-community.org/projects/mis-builder-16-0/mis-builder-16-0-mis_builder/pt_BR/
Resolve a permission issue when creating report periods with a user without admin rights.
@mpascuall mpascuall changed the title 17.0][FIX] mis_builder: Fix Version [17.0][FIX] mis_builder: Fix Version Apr 30, 2024
@mpascuall mpascuall mentioned this pull request May 6, 2024
3 tasks
Copy link

@javierobcn javierobcn left a comment

Choose a reason for hiding this comment

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

Works well

Copy link
Member

@etobella etobella 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 runboat and seems to work well, @sbidoul Can we ask for your opinion?

@sbidoul sbidoul changed the title [17.0][FIX] mis_builder: Fix Version [17.0][MIG mis_builder May 7, 2024
@sbidoul
Copy link
Member

sbidoul commented May 7, 2024

I'll have a look soon.

I've renamed the PR title :) Can you also reword the migration commit?

Also, can you rebase without 62359ca? This was for python 3.9 support but Odoo 17 does not support it.

@sbidoul
Copy link
Member

sbidoul commented May 7, 2024

/ocabot migration mis_builder

@OCA-git-bot OCA-git-bot added this to the 17.0 milestone May 7, 2024
@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). 🤖

@sbidoul sbidoul self-assigned this May 7, 2024
@sbidoul sbidoul changed the title [17.0][MIG mis_builder [17.0][MIG] mis_builder May 7, 2024
Copy link
Member

@sbidoul sbidoul left a comment

Choose a reason for hiding this comment

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

Hi, thanks for working on this migration. Here is a first round of comments.

mis_builder/models/mis_report.py Outdated Show resolved Hide resolved
mis_builder/models/mis_report.py Outdated Show resolved Hide resolved
mis_builder/models/mis_report.py Outdated Show resolved Hide resolved
mis_builder/models/mis_report.py Outdated Show resolved Hide resolved
mis_builder/models/mis_report.py Outdated Show resolved Hide resolved
mis_builder/tests/test_aep.py Outdated Show resolved Hide resolved
OCA-git-bot and others added 7 commits May 9, 2024 08:23
Currently translated at 76.8% (216 of 281 strings)

Translation: mis-builder-16.0/mis-builder-16.0-mis_builder
Translate-URL: https://translation.odoo-community.org/projects/mis-builder-16-0/mis-builder-16-0-mis_builder/it/
Currently translated at 99.6% (280 of 281 strings)

Translation: mis-builder-16.0/mis-builder-16.0-mis_builder
Translate-URL: https://translation.odoo-community.org/projects/mis-builder-16-0/mis-builder-16-0-mis_builder/sv/
Currently translated at 100.0% (281 of 281 strings)

Translation: mis-builder-16.0/mis-builder-16.0-mis_builder
Translate-URL: https://translation.odoo-community.org/projects/mis-builder-16-0/mis-builder-16-0-mis_builder/sv/
Currently translated at 100.0% (281 of 281 strings)

Translation: mis-builder-16.0/mis-builder-16.0-mis_builder
Translate-URL: https://translation.odoo-community.org/projects/mis-builder-16-0/mis-builder-16-0-mis_builder/sv/
Currently translated at 76.8% (216 of 281 strings)

Translation: mis-builder-16.0/mis-builder-16.0-mis_builder
Translate-URL: https://translation.odoo-community.org/projects/mis-builder-16-0/mis-builder-16-0-mis_builder/it/
Currently translated at 100.0% (281 of 281 strings)

Translation: mis-builder-16.0/mis-builder-16.0-mis_builder
Translate-URL: https://translation.odoo-community.org/projects/mis-builder-16-0/mis-builder-16-0-mis_builder/it/
@@ -13,8 +13,7 @@
</div>
<div class="oe_mis_builder_cp_right_bottom">
<div class="oe_mis_builder_filter_buttons">
<FilterMenu t-if="showSearchBar" />
<DatePicker
<DateTimePicker
Copy link
Member

Choose a reason for hiding this comment

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

This looks likes this, with the full calendar being displayed:

image

It should be a date field with the calendar drop down.

Also I don't think we should use DateTime here, a Date is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

t-if="showPivotDate"
onChange="onDateTimeChanged.bind(this)"
Copy link
Member

Choose a reason for hiding this comment

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

Does this some sort of binding to state.pivot_date?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it works exactly as v16. The called function is this one:
onDateTimeChanged(ev) { this.state.pivot_date = ev; this.refresh(); }

Copy link
Member

Choose a reason for hiding this comment

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

There is something missing, because in 16, you could set the Base Date on the report form, and it was preserved when clicking Preview.

image

image

image

With this branch, it does not appear in preview. You can check on runboat.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. In v16 the DatePicker date attribute was setted with state.pivot_date value. Since DateTimeInput doesnt have this attribute i setted it in value.

@mpascuall mpascuall force-pushed the 17.0-mig-mis_builder branch 2 times, most recently from caeec94 to 17b3f97 Compare May 23, 2024 09:51
@sbidoul
Copy link
Member

sbidoul commented May 24, 2024

Ah, another thing, the button box on the report instance form does not look right:

image

image

Probably something with the classes on the button box div.

@mpascuall
Copy link
Contributor Author

Ah, another thing, the button box on the report instance form does not look right:

image

image

Probably something with the classes on the button box div.

Fixed

@mpascuall mpascuall requested a review from sbidoul June 4, 2024 07:30
@mpascuall
Copy link
Contributor Author

Any updates on this? @sbidoul

@sbidoul
Copy link
Member

sbidoul commented Jun 10, 2024

@mpascuall To me the buttons are still misplaced. In v16 they are on the right. I have not had time to look for a correct solution for this. But the oe_right oe_button_box classes we use here don't seem to work correctly in v17. Also it seems weird to add a btn class on button tags, but again I've not looked closely yet.

@mpascuall
Copy link
Contributor Author

mpascuall commented Jun 12, 2024

@mpascuall To me the buttons are still misplaced. In v16 they are on the right. I have not had time to look for a correct solution for this. But the oe_right oe_button_box classes we use here don't seem to work correctly in v17. Also it seems weird to add a btn class on button tags, but again I've not looked closely yet.

Fixed, could you check it again please?

@HaraldPanten
Copy link

@sbidoul Can our team help you in anything with this module? I'm sure it's close to be merged 😉

@sbidoul
Copy link
Member

sbidoul commented Jun 20, 2024

Yes, it's close. I'll review the latest changes after my holidays. Also check if it works in wizard mode.

@HaraldPanten
Copy link

HaraldPanten commented Jun 21, 2024

Yes, it's close. I'll review the latest changes after my holidays. Also check if it works in wizard mode.

Nice. Enjoy your holidays 👍

Copy link
Member

@sbidoul sbidoul left a comment

Choose a reason for hiding this comment

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

Looks good now. Many thanks!

@sbidoul
Copy link
Member

sbidoul commented Jul 8, 2024

/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-607-by-sbidoul-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 820e08a into OCA:17.0 Jul 8, 2024
5 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 244aded. 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.