General refactor #138

cc-a · 2024-10-07T20:51:34Z

Description

This PR takes a pass at refactoring (mostly moving stuff around) the code base and simplifying some of the tests. It:

Separates the view functions in process_manager/views.py into 3 submodules handling pages (full individual pages), partials (elements embedded within larger pages used by HTMX) and actions (acting on dune processes). The tests are also split accordingly.
Pulls out commands that interact with the process manager into process_manager/process_manager_interface.py and standardises how we wrap async calls to ProcessManagerDriver methods.
Reduces the use of adhoc mocking by adding an auto-used mock fixture that prevents gRPC calls being made during tests.
Shortens the names of tests that are class methods to reduce redundant information.
Adds a PermissionRequired test class as an extension to LoginRequired which checkes that an authenticated but unpriveleged user is blocked from a view correctly.
Neatens some of the fixtures a bit.

Type of change

Documentation (non-breaking change that adds or improves the documentation)
New feature (non-breaking change which adds functionality)
Optimization (non-breaking, back-end change that speeds up the code)
Bug fix (non-breaking change which fixes an issue)
Breaking change (whatever its nature)

Key checklist

All tests pass (eg. python -m pytest)
The documentation builds and looks OK (eg. python -m sphinx -b html docs docs/build)
Pre-commit hooks run successfully (eg. pre-commit run --all-files)

Further checks

Code is commented, particularly in hard-to-understand areas
Tests added or an issue has been opened to tackle that in the future. (Indicate issue here: # (issue))

codecov-commenter · 2024-10-07T21:20:15Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (f9f2bea) to head (864ca64).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #138   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines           44        46    +2     
=========================================
+ Hits            44        46    +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

alexdewar

Looks good! I've made some small comments.

Not really related to this PR: but it feels kinda wrong that we're wrapping all the async calls to make them synchronous. The DUNE folks have given us a nice async API and it seems a pity to circumvent it! It looks like Django can handle async views (see here, with the caveat that if your middleware isn't async-aware then it'll just fall back on using threads internally. In any case, if we could do things that way we could avoid some of the boilerplate wrapping code. Worth opening an issue for?

process_manager/process_manager_interface.py

process_manager/views/pages.py

tests/process_manager/views/__init__.py

process_manager/views/actions.py

process_manager/process_manager_interface.py

cc-a · 2024-10-08T10:25:10Z

Not really related to this PR: but it feels kinda wrong that we're wrapping all the async calls to make them synchronous. The DUNE folks have given us a nice async API and it seems a pity to circumvent it! It looks like Django can handle async views (see here, with the caveat that if your middleware isn't async-aware then it'll just fall back on using threads internally. In any case, if we could do things that way we could avoid some of the boilerplate wrapping code. Worth opening an issue for?

Yeah, something that's crossed my mind as well. I've created #140 to follow up.

AdrianDAlessandro

This is good, I've been wanting to do the same kind of refactor. I have a lot of comments, but they're a combination of questions and reminders to open new issues. So I'm not requesting changes.

There are some missing docstrings that don't seem to be getting caught by linting (I think they're all functions that begin with "_"). Is this intentional?

drunc_ui/settings/settings.py

process_manager/process_manager_interface.py

tests/conftest.py

tests/test_process_manager_interface.py

tests/process_manager/views/test_action_views.py

tests/process_manager/views/test_page_views.py

jamesturner246

Still following. Looks good to me 👍

jamesturner246 · 2024-10-08T21:16:33Z

process_manager/process_manager_interface.py

Okay, so the get_whatevers are just placeholders until async views are done/decided on.

tests/conftest.py

dalonsoa

I have a couple of questions, but otherwise it looks good to me!

dalonsoa · 2024-10-09T04:24:37Z

process_manager/views/pages.py

+class BootProcessView(PermissionRequiredMixin, FormView[BootProcessForm]):
+ """View for the BootProcess form."""
+
+ template_name = "process_manager/boot_process.html"
+ form_class = BootProcessForm


Why do we need BootProcessForm in when indicating the class to inherit from? It doesn't seem to add much to aid the type checking, as far as I can tell.

FormView is a generic type and we're disallowing Any generics so we either have to provide this or ignore the error.

dalonsoa · 2024-10-09T04:27:35Z

process_manager/views/pages.py

+ success_url = reverse_lazy("process_manager:index")
+ permission_required = "main.can_modify_processes"
+
+ def form_valid(self, form: BootProcessForm) -> HttpResponse:


There's nothing in this method that uses specific features from the BootProcessForm, so using just forms.Form is more generic and enough for the type checker to work.

Suggested change

def form_valid(self, form: BootProcessForm) -> HttpResponse:

def form_valid(self, form: form.Form) -> HttpResponse:

I think that BootProcessForm makes the most sense as that is the type we expect in order for the correct data to be present for the function to work correctly. If it was just typed as Form then any Form subclass would be valid even though these don't contain the required data.

dalonsoa · 2024-10-09T04:33:58Z

process_manager/views/partials.py

+ no check boxes selected. POST renders the table with checked boxes for any table row
+ with a uuid provided in the select key of the request data.
+ """
+ selected_rows = request.POST.getlist("select", [])


When it is a GET method, does POST still exist with no select? Otherwise, how do we distinguish between a GET and a POST?

It does still exist. We make use of the case that there is no difference in behaviour of the function when there is a GET request or a POST request with no data.

tests/conftest.py

cc-a requested review from dalonsoa, jamesturner246, TinyMarsh, alexdewar, AdrianDAlessandro and Sahil590 October 7, 2024 20:51

cc-a force-pushed the general-refactor branch 2 times, most recently from 1b68573 to 992de6e Compare October 7, 2024 21:17

cc-a added 5 commits October 7, 2024 22:18

Refactor views

7e31921

Tidy mocks

c99737f

Shorten test method names

b4f7d07

Refactor tests with PermissionRequiredTest class

ebab00e

Tidy user permission fixtures

df4f5a2

cc-a force-pushed the general-refactor branch from 992de6e to df4f5a2 Compare October 7, 2024 21:18

alexdewar approved these changes Oct 8, 2024

View reviewed changes

cc-a added 2 commits October 8, 2024 11:11

Improve type hints

e802aaa

Add comment on error handling of enum value in process_action view

55ae84e

Add django-stubs-ext as a non-dev dependency

995e719

AdrianDAlessandro approved these changes Oct 8, 2024

View reviewed changes

Remove test_admin from TestIndexView

864ca64

jamesturner246 approved these changes Oct 8, 2024

View reviewed changes

dalonsoa approved these changes Oct 9, 2024

View reviewed changes

cc-a merged commit 560b576 into main Oct 9, 2024
4 checks passed

cc-a deleted the general-refactor branch October 9, 2024 09:25

This was referenced Oct 9, 2024

Use unmanaged model for Drunc processes #63

Closed

Expand test coverage in process_manager_interface #144

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

General refactor #138

General refactor #138

cc-a commented Oct 7, 2024 •

edited

Loading

codecov-commenter commented Oct 7, 2024 •

edited

Loading

alexdewar left a comment

cc-a commented Oct 8, 2024

AdrianDAlessandro left a comment

jamesturner246 left a comment

jamesturner246 Oct 8, 2024

dalonsoa left a comment

dalonsoa Oct 9, 2024

cc-a Oct 9, 2024

dalonsoa Oct 9, 2024

cc-a Oct 9, 2024

dalonsoa Oct 9, 2024

cc-a Oct 9, 2024

	def form_valid(self, form: BootProcessForm) -> HttpResponse:
	def form_valid(self, form: form.Form) -> HttpResponse:

General refactor #138

General refactor #138

Conversation

cc-a commented Oct 7, 2024 • edited Loading

Description

Type of change

Key checklist

Further checks

codecov-commenter commented Oct 7, 2024 • edited Loading

Codecov Report

alexdewar left a comment

Choose a reason for hiding this comment

cc-a commented Oct 8, 2024

AdrianDAlessandro left a comment

Choose a reason for hiding this comment

jamesturner246 left a comment

Choose a reason for hiding this comment

jamesturner246 Oct 8, 2024

Choose a reason for hiding this comment

dalonsoa left a comment

Choose a reason for hiding this comment

dalonsoa Oct 9, 2024

Choose a reason for hiding this comment

cc-a Oct 9, 2024

Choose a reason for hiding this comment

dalonsoa Oct 9, 2024

Choose a reason for hiding this comment

cc-a Oct 9, 2024

Choose a reason for hiding this comment

dalonsoa Oct 9, 2024

Choose a reason for hiding this comment

cc-a Oct 9, 2024

Choose a reason for hiding this comment

cc-a commented Oct 7, 2024 •

edited

Loading

codecov-commenter commented Oct 7, 2024 •

edited

Loading