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

Hide reports list behind a feature flag #4065

Merged
merged 1 commit into from
Mar 18, 2021

Conversation

StefanFl
Copy link
Member

The reports list is only used to make pdf reports accessible. Now the PDF reports are not supported anymore for more than a year now. This PR hides the list behind a feature flag and renames the button in the report builder to just "Run". Goal is to avoid misunderstandings of users.

Additionally it removes the upper button to generate a report in the report builder because it is redundant and looks different than the one at the bottom.

Shall we additionally include a message, that PDF reports are deprecated and removed in the future, eg. 30.6.2021?

@valentijnscholten
Copy link
Member

I see that this feature also kinda re-enables the pdf reports. Are they working correctly? It could still be an interesteing fearure if the PDFs look nice. But it's just wikihtmltopdf I think, so they might be not so nice looking. Similar to "print to pdf" from the browser they look pretty poor, no colours, etc.

@StefanFl
Copy link
Member Author

I tried to test wkhtmltopdf with docker, but it is problematic because the library needs an X11 server. So it's only usable in a source installation what we don't support anymore. If you can't produce a PDF export like it is now, then the reports list dialogue could be disabled completely right now as well. The feature flag with the PDF as a choice is a consistent way to leave the current functionality available for people who need it.

Marking the current way of producing PDF as deprecated and remove it from the source 3 months later would be what I propose. If someone needs PDF reports, then it might be the best way to start from scratch with a Python based library.

@valentijnscholten
Copy link
Member

Yes, so we merge this but also create a github issue labeled with "2.0.0" for removing PDF reports alltogether. I think there's a lot more code around that, also in the API to generate reports etc.

@StefanFl
Copy link
Member Author

Yes there is some more code that are obsolete. I can write the issue for removing the PDF reports later today.

And I would like to make a note in the documentation about removing the PDF reports. Can do it in a second PR later this week.

Copy link
Contributor

@Maffooch Maffooch left a comment

Choose a reason for hiding this comment

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

Good fix for now to reduce the questions.
+1 for removing altogether in a 2.0 of sorts

@StefanFl
Copy link
Member Author

The issue to remove the code for PDF reports is #4077. Please attach it to milestone 2.0 as discussed. Thank you

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.

None yet

3 participants