Skip to content

[FIX] report_csv: make it working if docids is False #962

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

Merged

Conversation

maisim
Copy link

@maisim maisim commented Jan 8, 2025

len() is used on docids, but in some cases, docids is False wich raised a TypeError

Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label May 11, 2025
Copy link

@remi-filament remi-filament left a comment

Choose a reason for hiding this comment

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

Indeed, as per code in controller, docids is None by default, and in this case you will get a TypeError.

See :

def report_routes(self, reportname, docids=None, converter=None, **data):

Called from
response = self.report_routes(
reportname, converter="csv", context=context, **data
)
if not docids

@github-actions github-actions bot removed the stale PR/Issue without recent activity, it'll be soon closed automatically. label May 18, 2025
Copy link
Contributor

@petrus-v petrus-v left a comment

Choose a reason for hiding this comment

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

could be nice to add unittest to avoid regressions !

other wise code looks good to me

@maisim maisim force-pushed the 16.0-fix-report_csv-manage-no-docids-case branch from 6a52290 to bd5cc62 Compare May 19, 2025 09:20
len() is used on docids, but in some cases, docids is None
wich raised a TypeError
@maisim maisim force-pushed the 16.0-fix-report_csv-manage-no-docids-case branch from bd5cc62 to a70c672 Compare May 19, 2025 09:21
@@ -55,7 +55,7 @@ def _render_csv(self, report_ref, docids, data):
report_sudo = self._get_report(report_ref)
report_model_name = "report.%s" % report_sudo.report_name
report_model = self.env[report_model_name]
res_id = len(docids) == 1 and docids[0]
res_id = docids[0] if docids and len(docids) == 1 else None
Copy link
Author

Choose a reason for hiding this comment

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

just replaced
res_id = (len(docids) == 1 and docids[0]) if docids else False
with
res_id = docids[0] if docids and len(docids) == 1 else None

wich is more readable

Copy link
Author

Choose a reason for hiding this comment

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

@petrus-v @remi-filament
Just added a test and the little change above

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

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 16.0-ocabot-merge-pr-962-by-HviorForgeFlow-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit eb102c0 into OCA:16.0 Jun 4, 2025
7 checks passed
@OCA-git-bot
Copy link
Contributor

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

5 participants