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

[#901, #902, #903 ] Implemented Action-status toggle and added HTMX to backend #347

Merged
merged 11 commits into from
Dec 7, 2022

Conversation

Bartvaderkin
Copy link
Contributor

@Bartvaderkin Bartvaderkin commented Nov 23, 2022

It is quite a lot for a simple button, but this also adds htmx and django-htmx to the project.

Additionally I've added some infrastructure and related tests to be able to render a template-tag to a string.

And then tied that into a base class for a View to handle hx-post requests and respond with a rendered template-tag (this gives convenient partial responses without extra boilerplate templates or hx-select's).

And then expanded on Sven's selenium integration testing.

And then the functionality exists in two flavours (plain Actions and Actions via Plans) so implementation and test cases almost double.

image

@Bartvaderkin Bartvaderkin force-pushed the feature/901-action-status-toggle branch from 091bb37 to 5390496 Compare November 24, 2022 15:33
@Bartvaderkin Bartvaderkin changed the title WIP [#901] Implemented Action-status toggle and added HTMX to backend WIP [#901, #902, #903 ] Implemented Action-status toggle and added HTMX to backend Nov 24, 2022
@Bartvaderkin Bartvaderkin force-pushed the feature/901-action-status-toggle branch from 5390496 to cd4a91a Compare November 24, 2022 15:59
@Bartvaderkin Bartvaderkin force-pushed the feature/901-action-status-toggle branch 5 times, most recently from a535b07 to bc1560f Compare December 1, 2022 09:58
@@ -75,5 +75,6 @@ <h2 class="modal__title" id="modal__title"></h2>
</div>

<script nonce="{{ request.csp_nonce }}" src="{% static 'bundles/open_inwoner-js.js' %}" type="text/javascript"></script>
{% django_htmx_script %}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This tag adds some development features when DEBUG=True

@Bartvaderkin Bartvaderkin changed the title WIP [#901, #902, #903 ] Implemented Action-status toggle and added HTMX to backend [#901, #902, #903 ] Implemented Action-status toggle and added HTMX to backend Dec 1, 2022
@Bartvaderkin Bartvaderkin force-pushed the feature/901-action-status-toggle branch from 9b41287 to 229efa0 Compare December 1, 2022 11:14
@Bartvaderkin Bartvaderkin marked this pull request as ready for review December 1, 2022 12:56
@@ -132,3 +135,118 @@ def render(self):
kwargs,
self.template_name,
)


class RenderableTag:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bit of a black box but the main part from InclusionTagWebTest has been used for testing for a while.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I don't understand why you use it.
Can't we just use django TemplateResponse or render shortcut? It does the same things - resolves the template with it's name and then render the content. In the htmx view we return a piece of HTML, why not to do it the way django usually does it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because that would require to create boilerplate template files that would only have a {% load xyz %} and the tag we want to render with a repeat of all the arguments. This class basically creates that on the fly so we need less files and repeating of argument names/values.

Copy link
Contributor

@annashamray annashamray left a comment

Choose a reason for hiding this comment

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

django-htmx looks interesting,my main question is about the necessity of RenderableTag

src/open_inwoner/htmx/views.py Show resolved Hide resolved
@@ -132,3 +135,118 @@ def render(self):
kwargs,
self.template_name,
)


class RenderableTag:
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I don't understand why you use it.
Can't we just use django TemplateResponse or render shortcut? It does the same things - resolves the template with it's name and then render the content. In the htmx view we return a piece of HTML, why not to do it the way django usually does it?

Copy link
Contributor

@vaszig vaszig left a comment

Choose a reason for hiding this comment

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

Only one remark, I don't know if it's a problem only on my end. When an action is not mine or it's not for me, the options for toggling the status are enabled so I get a 404.

@codecov-commenter
Copy link

codecov-commenter commented Dec 6, 2022

Codecov Report

Merging #347 (58816f2) into develop (2ae8276) will increase coverage by 0.01%.
The diff coverage is 97.28%.

@@             Coverage Diff             @@
##           develop     #347      +/-   ##
===========================================
+ Coverage    96.48%   96.49%   +0.01%     
===========================================
  Files          450      457       +7     
  Lines        14034    14367     +333     
===========================================
+ Hits         13541    13864     +323     
- Misses         493      503      +10     
Impacted Files Coverage Δ
src/open_inwoner/accounts/urls.py 100.00% <ø> (ø)
src/open_inwoner/accounts/views/__init__.py 100.00% <ø> (ø)
...pen_inwoner/components/templatetags/button_tags.py 94.82% <ø> (ø)
...n_inwoner/components/templatetags/dropdown_tags.py 100.00% <ø> (ø)
src/open_inwoner/conf/base.py 94.90% <ø> (ø)
src/open_inwoner/plans/urls.py 100.00% <ø> (ø)
src/open_inwoner/plans/views.py 97.18% <80.00%> (-0.85%) ⬇️
src/open_inwoner/htmx/views.py 90.32% <90.32%> (ø)
src/open_inwoner/accounts/choices.py 97.05% <90.90%> (-2.95%) ⬇️
src/open_inwoner/components/utils.py 96.34% <91.66%> (-3.66%) ⬇️
... and 15 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Bartvaderkin Bartvaderkin force-pushed the feature/901-action-status-toggle branch from a4d7e01 to 58816f2 Compare December 7, 2022 08:37
@Bartvaderkin
Copy link
Contributor Author

Bartvaderkin commented Dec 7, 2022

I rebased after merge of the Contact's PR.

Note Vasileios had a good spot on his last comment (#347 (review)), but I fixed it in 954a891

@Bartvaderkin
Copy link
Contributor Author

@svenvandescheur Can you sign-off on this so we can get it merged? Afterwards we need a little style update on the dropdown and button style, see Anna's comment: #347 (comment)

@alextreme
Copy link
Member

Happy to merge after Anna or Vasileios approves. Create a separate Taiga task for any styling still necessary

Copy link
Contributor

@vaszig vaszig left a comment

Choose a reason for hiding this comment

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

The status toggle works as expected concerning the shared plans now. If Anna is also ok this can be merged.

Copy link
Contributor

@annashamray annashamray left a comment

Choose a reason for hiding this comment

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

If other developers are ok with this approach, I approve.
I'm curious where this approach would be developing, but it looks like worth experimenting

@alextreme alextreme merged commit 32f04c3 into develop Dec 7, 2022
@alextreme alextreme deleted the feature/901-action-status-toggle branch December 7, 2022 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants