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

Fixes #13666: Fix behavior for reports without test methods #13667

Merged
merged 1 commit into from
Sep 26, 2023

Conversation

JCWasmx86
Copy link
Contributor

@JCWasmx86 JCWasmx86 commented Sep 3, 2023

Fixes: #13666

If you upload a report file without any test methods, the entire GUI for the reports page fails. This fixes the error by splitting up the report_modules into valid and empty ones. The valid report_modules are rendered as before, the empty ones are listed with the possibility to delete them. (Before you had to drop into the CLI to delete them)

The list of reports without test methods are at the top of the page to increase the visibility of them, but you could consider moving them into the normal list of reports, but show an error/warning there - similar to e.g. "Failed to load foo.py"

Additionally there is a second commit (But can be either dropped/moved to new PR as it changes the scope of the issue a little bit). It fixes the API by filtering out those reports

Note: I wasn't able to run black, as black --config pyproject.toml netbox/ wants to reformat everything, but pycodestyle --ignore=W504,E501 netbox/ shows no warnings)

Copy link
Member

@jeremystretch jeremystretch left a comment

Choose a reason for hiding this comment

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

A much simpler fix would be to simply remove the exception (below) and perhaps add a warning to the UI when viewing a report with no test methods. (It might also make sense to disable the "run" action.)

if not test_methods:
raise Exception("A report must contain at least one test method.")

@JCWasmx86
Copy link
Contributor Author

Yes I will do that. I hesitated to remove the exception, as I thought it was maybe important (And just not documented, why missing test methods should be an error instead of an warning - thus this more complex solution)

@JCWasmx86
Copy link
Contributor Author

Do you like a design like that:

image

Or should the alert be an entire table row below the ReportClass?

@jeremystretch
Copy link
Member

I would change the text to something like "Invalid (no test methods found)" and remove the alert styling (it's not intended for table cells). But otherwise yes I think this is great!

@JCWasmx86
Copy link
Contributor Author

JCWasmx86 commented Sep 26, 2023

Looks like that now:

image

The button here is now disabled (I think that's the only other way to start a report using the GUI):

image

Copy link
Member

@jeremystretch jeremystretch left a comment

Choose a reason for hiding this comment

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

Thanks @JCWasmx86!

@jeremystretch jeremystretch merged commit e67624f into netbox-community:develop Sep 26, 2023
4 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uploading report without test method yields "Server Error"
2 participants