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

[15.0][MIG] excel_import_export, excel_import_export_demo: migration #2329

Merged
merged 80 commits into from
Jan 12, 2023
Merged

[15.0][MIG] excel_import_export, excel_import_export_demo: migration #2329

merged 80 commits into from
Jan 12, 2023

Conversation

Mantux11
Copy link
Contributor

Migration to version 15.0. Changes by pre-commit and changes because of Odoo code changes.

@Mantux11 Mantux11 marked this pull request as draft April 29, 2022 11:30
@Mantux11
Copy link
Contributor Author

Mantux11 commented Apr 29, 2022

@kittiu Hey. Because of your migration to v13 and v14, I want some help from you. I think, you know how JS worked here and mostly all functionality what JS code does here. Personally, I don't know probably anything about JS, and I saw that Odoo deleted ActionManager and I suggest that everything was moved to services or somewhere else. So my questions are:

  1. How JS code worked here, what was functionality of JS code, why it was used here?
  2. Maybe you could suggest, where I could try to look to understand what is going on here? Maybe you know where everything was moved, what was changed and where I could insert these functions? Some files to look, classes and etc.

Thank you! 😉

@kittiu
Copy link
Member

kittiu commented May 4, 2022

@Mantux11 you are wrong that I know what I was doing about JS 😆
I just try to imitate the work from other module, i..e, report_csv, report_xlsx.
Now, I saw this already migrated to v15 https://github.com/OCA/reporting-engine/blob/15.0/report_xlsx/static/src/js/report/action_manager_report.esm.js

:)

@Mantux11
Copy link
Contributor Author

Mantux11 commented May 5, 2022

@kittiu Thanks for your answer. I will try to change what it's needed to be changed. :D

@Mantux11 Mantux11 marked this pull request as ready for review May 5, 2022 14:03
@kittiu
Copy link
Member

kittiu commented May 16, 2022

Not sure this is ready for review. Functional test couple of action without success.

  1. Sales > Reporting > Partner List Report
    image
  2. Sales > Sales Order > Quotation / Order (.xlsx) ----> not in the print action yet.
  3. Technical > Excel Import/Export > Sample Lead Report, error when click to export.
    image

@Mantux11
Copy link
Contributor Author

Not sure this is ready for review. Functional test couple of action without success.

  1. Sales > Reporting > Partner List Report

image

  1. Sales > Sales Order > Quotation / Order (.xlsx) ----> not in the print action yet.

  2. Technical > Excel Import/Export > Sample Lead Report, error when click to export.

image

Okey, I will test more on that. Thanks, for review

@Mantux11
Copy link
Contributor Author

Mantux11 commented May 20, 2022

@kittiu Talking about Sample Lead Report, you also missed that, this isn't working in 14 version, because planned_revenue field is deleted from 14 version. So I changed to expected_revenue, why I changed to that, you could see comparing 13 and 14 crm.lead core code. Partner List report is fixed. Talking about SO report, I see that when you press Export XLSX, you could choose what template to use in wizard. Maybe I'm wrong, but this should work as it is? Or there should be one more report? It would be really appreciated, If you could give me more example's on SO report, how it should look like,.

@gaikaz
Copy link
Member

gaikaz commented Aug 24, 2022

@OCA/server-environment-maintainers
Sorry to be pinging, but this is a valid migration that is just not getting any attention for 3 months now. Wouldn't want to be considered stale and closed.
Could we please get a maintainer review or two? 🙏🏻

@kittiu
Copy link
Member

kittiu commented Aug 24, 2022

@gaikaz have you tested this? Can you approve this? Thanks!

@kittiu
Copy link
Member

kittiu commented Aug 24, 2022

Ping @Saran440 @ps-tubtim

@gaikaz
Copy link
Member

gaikaz commented Aug 24, 2022

@kittiu
We'll be migrating one of our modules that depend on this, hopefully over next week.
Will do my review too.

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.

Test with demo 👍

@GreyShim
Copy link

Hello guys,

Is it possible to merge the pull request ? I need this module for a project

Regards

@Mantux11 Mantux11 requested a review from Du-ma October 11, 2022 07:35
@kittiu
Copy link
Member

kittiu commented Oct 11, 2022

@Mantux11 looks like pre-commit is failing. Can you check please?

@GreyShim
Copy link

@Mantux11

do not forget to integrate this change : https://github.com/OCA/server-tools/pull/2283/files
it's very important, data are deleted without this

@Mantux11
Copy link
Contributor Author

@GreyShim I would wait for that PR to be merged. And I would include that here.

@Mantux11
Copy link
Contributor Author

Mantux11 commented Nov 24, 2022

@kittiu, @GreyShim, @Du-ma Could you review it now, please? This module is waiting in the line a long time ago. I changed what @GreyShim said that, and also, included what @luc-demeyer suggested in that PR above.

Copy link

@norlinhenrik norlinhenrik left a comment

Choose a reason for hiding this comment

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

Thank you for migrating and fixing this module!

models/common.py
line = "{}.{}".format("xls", uuid.uuid4()) + "," + line
To be consistent, please change to
line = "{}.{}".format("--excel_import_export--", uuid.uuid4()) + "," + line
with underscore instead of hyphen. (I cannot write underscore since it is evaluated as bold.)

@gaikaz
Copy link
Member

gaikaz commented Dec 1, 2022

@Mantux11 Henrik here has very good suggestion.

@norlinhenrik
You can do \_ and have __as many underscores as you want__😉

@Du-ma
Copy link
Member

Du-ma commented Dec 2, 2022

Pls don't copy paste that

@Mantux11
Copy link
Contributor Author

Mantux11 commented Dec 5, 2022

@norlinhenrik Changed that. @Du-ma Yes I know, how you like to use f"{}" string :D

excel_import_export/models/ir_report.py Outdated Show resolved Hide resolved
excel_import_export/wizard/import_xlsx_wizard.py Outdated Show resolved Hide resolved
excel_import_export/wizard/import_xlsx_wizard.py Outdated Show resolved Hide resolved
excel_import_export/wizard/import_xlsx_wizard.py Outdated Show resolved Hide resolved
@Du-ma
Copy link
Member

Du-ma commented Dec 14, 2022

Also a general question, why is this module not in oca/reporting-engine?

Copy link
Contributor

@AungKoKoLin1997 AungKoKoLin1997 left a comment

Choose a reason for hiding this comment

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

Test with demo data: work as expected
For the functional, there is a minor things.

We can remove xlrd and xlwt from external dependencies because these two are part of base odoo requirements.

Sales > Sales Order > Quotation / Order (.xlsx) ----> not in the print action yet.
We need to click manually to add this report under print action in report definition. But I think this is not a problem. We can export under action.

Screenshot from 2022-12-14 17-32-08

oca-transbot and others added 14 commits December 29, 2022 11:58
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: server-tools-14.0/server-tools-14.0-excel_import_export
Translate-URL: https://translation.odoo-community.org/projects/server-tools-14-0/server-tools-14-0-excel_import_export/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: server-tools-14.0/server-tools-14.0-excel_import_export_demo
Translate-URL: https://translation.odoo-community.org/projects/server-tools-14-0/server-tools-14-0-excel_import_export_demo/
Currently translated at 5.5% (11 of 200 strings)

Translation: server-tools-14.0/server-tools-14.0-excel_import_export
Translate-URL: https://translation.odoo-community.org/projects/server-tools-14-0/server-tools-14-0-excel_import_export/it/
Currently translated at 82.5% (165 of 200 strings)

Translation: server-tools-14.0/server-tools-14.0-excel_import_export
Translate-URL: https://translation.odoo-community.org/projects/server-tools-14-0/server-tools-14-0-excel_import_export/it/
Currently translated at 82.5% (165 of 200 strings)

Translation: server-tools-14.0/server-tools-14.0-excel_import_export
Translate-URL: https://translation.odoo-community.org/projects/server-tools-14-0/server-tools-14-0-excel_import_export/it/
Currently translated at 82.5% (165 of 200 strings)

Translation: server-tools-14.0/server-tools-14.0-excel_import_export
Translate-URL: https://translation.odoo-community.org/projects/server-tools-14-0/server-tools-14-0-excel_import_export/it/
Because those dependencies are already part of Odoo's base dependencies.
Currently translated at 100.0% (200 of 200 strings)

Translation: server-tools-14.0/server-tools-14.0-excel_import_export
Translate-URL: https://translation.odoo-community.org/projects/server-tools-14-0/server-tools-14-0-excel_import_export/es_AR/
@Mantux11
Copy link
Contributor Author

Mantux11 commented Dec 29, 2022

@AungKoKoLin1997 I included all newest commit's history from 14.0 with commit that removes mentioned libaries, Could you review, and maybe merge this?

Or @kittiu Could you merge?

@gaikaz
Copy link
Member

gaikaz commented Jan 2, 2023

@kittiu
I can finally approve this 🎉
We have a client with a big excel report that uses this module for generating it and after we now have migrated the client that uses this module while internally fixing any found issues with @Mantux11, it now definitely works.

@kittiu
Copy link
Member

kittiu commented Jan 2, 2023

Thank you all but I don't have power to merge. @dreispt can you help merge this? Thanks!

@gaikaz
Copy link
Member

gaikaz commented Jan 10, 2023

@OCA/server-environment-maintainers
Pretty please, review this 🙏🏻 This has a lot of approves with functional tests as well.
If we (Via laurea) find any issues, we'll make a PR to fix. Pinky promise.

@dreispt
Copy link
Member

dreispt commented Jan 12, 2023

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 15.0-ocabot-merge-pr-2329-by-dreispt-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 08afeb1 into OCA:15.0 Jan 12, 2023
@OCA-git-bot
Copy link
Contributor

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

@Mantux11 Mantux11 deleted the 15.0-mig-excel_import_export,-excel_import_export_demo branch January 12, 2023 19:47
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.