feat(risk-matrix): use plotly #2
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2 +/- ##
==========================================
+ Coverage 70.92% 71.05% +0.12%
==========================================
Files 61 61
Lines 2573 2591 +18
==========================================
+ Hits 1825 1841 +16
- Misses 748 750 +2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Looks good for the most part, there's still some style nits that are probably leftovers from black auto formatting and an architectural change on how to get a numbered column inside a DataTable (especially one that remains stable once sorting and/or AJAX are involved, although the latter case you can probably forget about for now, since we'll likely never deal with thousands of rows, so loading them in separated requests isn't really worth it at that point).
| for idx, item in enumerate(query): | ||
| item.nr = idx + 1 |
There was a problem hiding this comment.
| for idx, item in enumerate(query): | |
| item.nr = idx + 1 | |
| for idx, item in enumerate(query, start=1): | |
| item.nr = idx |
Alright now I see what you use this nr for. This probably shouldn't really be stored on the model at all (definitely not inside the database through a mapped_column, a regular attribute should be fine). I would probably implement this using a subclass of DataColumn. It will be a little bit more tricky to get right though from that end, so maybe this should just be an option on the DataTable to prepend a numeric column, just like you can optionally add buttons, although I'm not sure how this will interact with custom sorting.
You could also potentially do it entirely through the frontend: https://datatables.net/examples/api/counter_columns.html
| "title": _("Risk Matrix"), | ||
| "plot": Markup(plot_risk_matrix(table.query())), # noqa: MS001 | ||
| "table": table, |
There was a problem hiding this comment.
| "title": _("Risk Matrix"), | |
| "plot": Markup(plot_risk_matrix(table.query())), # noqa: MS001 | |
| "table": table, | |
| 'title': _('Risk Matrix'), | |
| 'plot': plot_risk_matrix(table.query()), | |
| 'table': table, |
style nit, also I would return Markup from plot_risk_matrix, don't wrap it here.
src/riskmatrix/subscribers.py
Outdated
| @@ -1,6 +1,6 @@ | |||
| from pyramid.events import NewRequest | |||
| from pyramid.events import NewResponse | |||
|
|
|||
| import secrets | |||
There was a problem hiding this comment.
Try to keep imports sorted alphabetically and by type, if that's too tedious you can consider adding isort as precommit hook, so it does it for you. Although isort will not separate out typing imports into its own block at the bottom, like I usually do, I can live with those being mixed in with the other imports.
| @@ -51,6 +51,12 @@ def sentry_context(event: NewRequest) -> None: | |||
| scope.user = {'id': request.user.id} | |||
|
|
|||
|
|
|||
| def request_none_generator(event: 'NewRequest') -> None: | |||
| request = event.request | |||
| request.set_property(lambda r: secrets.token_urlsafe(), 'csp_nonce', reify=True) | |||
There was a problem hiding this comment.
Make sure to also add a csp_nonce attribute to testing.DummyRequest, this can just be a static string for testing. Some tests currently fail because the attribute doesn't exist.
| @@ -22,7 +24,7 @@ | |||
| if TYPE_CHECKING: | |||
| from pyramid.interfaces import IRequest | |||
| from sqlalchemy.orm.query import Query | |||
| from typing import TypeVar | |||
| from typing import TypeVar, Iterator | |||
There was a problem hiding this comment.
| from typing import TypeVar, Iterator | |
| from collections.abc import Iterator | |
| from typing import TypeVar |
Always use the collections.abc imports for these protocols, the typing aliases have been deprecated since 3.9. You could use pyupgrade to auto-correct imports. Although if you're also going to include isort it might be easier to just switch most of the linting over to ruff and only keep flake8 for some of the plugins that aren't included in ruff or not as up-to-date like flake8-type-checking.
ruff also includes auto fixes for a lot of the errors, if you're into that sort of thing.
|
|
||
|
|
||
| class RiskMatrixAssessment(RiskAssessment): | ||
| nr: ClassVar[int] |
There was a problem hiding this comment.
| nr: ClassVar[int] | |
| nr: int |
Based on how this is used, this shouldn't be a ClassVar.
| hovertemplate=f'{risk.nr} {risk.name} \ | ||
| (Impact: {risk.impact} Likelihood: {risk.likelihood})', |
There was a problem hiding this comment.
| hovertemplate=f'{risk.nr} {risk.name} \ | |
| (Impact: {risk.impact} Likelihood: {risk.likelihood})', | |
| hovertemplate=f'{risk.nr} {risk.name} ' | |
| '(Impact: {risk.impact} Likelihood: {risk.likelihood})', |
Please avoid using broken lines, especially inside string literals. Python has implicit string concatenation. There's flake8-broken-lines in case this is something copilot tries to make you do, rather than a conscious decision.
No description provided.