Skip to content

Conversation

@legalsylvain
Copy link
Contributor

  • split view in two files, according OCA guidelines;
  • add legalsylvain as maintainers
  • use abstract tree and form views
  • refactor : split demo data into two files, according OCA guidelines
  • replace obsolete base.menu_reporting_dashboard by spreadsheet_dashboard entries
  • update translation
  • prevent usage of export with parameters, that requires extra work

Note export with parameters doesn't work for the time being, due to the refactoring of fields_view_get in V16.
this should be reimplemented in another PR.

@legalsylvain
Copy link
Contributor Author

/ocabot migration sql_export

@OCA-git-bot
Copy link
Contributor

Sorry @legalsylvain you are not allowed to mark the addon tobe migrated.

To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons.

If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the maintainers key of its manifest.

@HviorForgeFlow
Copy link
Member

/ocabot migration sql_export

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Oct 26, 2022
@OCA-git-bot OCA-git-bot mentioned this pull request Oct 26, 2022
15 tasks
@bealdav
Copy link
Member

bealdav commented Nov 23, 2022

@legalsylvain test-requirement.txt should alseo be removed here. Thanks a lot

florian-dacosta and others added 22 commits November 23, 2022 16:21
Add rollback after executing query as a double security with blacklist terms

add known issue in readme
[FIX] encoding with mogrify

[FIX] hide placeholder
Currently translated at 100,0% (50 of 50 strings)

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

Translation: server-tools-12.0/server-tools-12.0-sql_export
Translate-URL: https://translation.odoo-community.org/projects/server-tools-12-0/server-tools-12-0-sql_export/
@legalsylvain
Copy link
Contributor Author

Done @bealdav
Could you update your review ?

Thanks !

@bealdav
Copy link
Member

bealdav commented Nov 23, 2022

Thanks a lot @legalsylvain for this huge improvement and cleaning.

2 bugs found testing it.

In draft state all is nice
image

Missing select string and other left string
image
Probably due to js psql syntax lib. A workaround could be to not strip the query. I think it's sufficient

@bealdav
Copy link
Member

bealdav commented Nov 23, 2022

Syntax is not correct

image

But the module considers it's correct at the validation step.

image

Finally it fails at the execution step

image

@legalsylvain
Copy link
Contributor Author

Hi @bealdav.

  1. regarding the first bug, i faced to it. I think it's due to a problem in odoo js framework, when the field become readonly. Workaround found : reload the page, and all is OK. Is it OK for you ?

  2. regarding the second bug : yes, it seems that it was explicitely disabled. the last commit fixes the problem. Could you test again ?

- split view in two files, according OCA guidelines;
- add legalsylvain as maintainers
- use abstract tree and form views
- refactor : split demo data into two files, according OCA guidelines
- replace obsolete base.menu_reporting_dashboard by spreadsheet_dashboard entries
- update translation
- prevent usage of export with parameters, that requires extra work
- do not skip 'check execution' when confirming sql exports
Copy link
Member

@bealdav bealdav left a comment

Choose a reason for hiding this comment

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

LGTM tested again on runboat.
OK now
Thanks a lot

Copy link
Contributor

@florian-dacosta florian-dacosta left a comment

Choose a reason for hiding this comment

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

LGTM and thanks for this port

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

@HviorForgeFlow
Copy link
Member

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

@OCA-git-bot OCA-git-bot merged commit f57897d into OCA:16.0 Nov 28, 2022
@OCA-git-bot
Copy link
Contributor

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