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

✨ - Use htmx to async update messages. #342

Merged
merged 21 commits into from
Nov 28, 2022
Merged

✨ - Use htmx to async update messages. #342

merged 21 commits into from
Nov 28, 2022

Conversation

svenvandescheur
Copy link
Contributor

No description provided.

@annashamray
Copy link
Contributor

Is it possible to unit test? (if not then should we really use it?)

@svenvandescheur
Copy link
Contributor Author

Is it possible to unit test? (if not then should we really use it?)

It uses the same logic as the previous implementation except async. The actual implementation is iirc 4 properties, testing those would just be retesting the htmx library.

@annashamray
Copy link
Contributor

I still think this should be tested. If unit testing is not a good idea, end-to-end tests could be introduced

@svenvandescheur
Copy link
Contributor Author

svenvandescheur commented Nov 22, 2022

I still think this should be tested. If unit testing is not a good idea, end-to-end tests could be introduced

Ok, let's see if I can add some webtest to test the integration.

@codecov-commenter
Copy link

codecov-commenter commented Nov 23, 2022

Codecov Report

Merging #342 (cd4a1d3) into develop (de5a4fe) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #342      +/-   ##
===========================================
+ Coverage    96.41%   96.45%   +0.03%     
===========================================
  Files          433      433              
  Lines        13140    13354     +214     
===========================================
+ Hits         12669    12880     +211     
- Misses         471      474       +3     
Impacted Files Coverage Δ
.../open_inwoner/components/templatetags/form_tags.py 95.12% <ø> (ø)
src/open_inwoner/accounts/tests/test_inbox_page.py 100.00% <100.00%> (ø)
src/open_inwoner/components/tests/test_messages.py 100.00% <100.00%> (ø)
src/open_inwoner/openzaak/cases.py 67.85% <0.00%> (-4.53%) ⬇️
src/open_inwoner/openzaak/utils.py 92.59% <0.00%> (-2.41%) ⬇️
src/open_inwoner/openzaak/tests/test_cases.py 100.00% <0.00%> (ø)
.../open_inwoner/components/templatetags/card_tags.py 96.96% <0.00%> (ø)
.../open_inwoner/components/templatetags/list_tags.py 100.00% <0.00%> (ø)
src/open_inwoner/accounts/views/cases.py 99.30% <0.00%> (+0.04%) ⬆️
src/open_inwoner/utils/templatetags/utils.py 62.50% <0.00%> (+3.04%) ⬆️
... and 1 more

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

@svenvandescheur
Copy link
Contributor Author

svenvandescheur commented Nov 23, 2022

@annashamray I've added a selenium test for this.

Copy link
Member

@alextreme alextreme left a comment

Choose a reason for hiding this comment

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

Does seem like a bit of HTML magic, but lets try this out.

@annashamray see if you can help Sven with the requirements FIXME for adding selenium

@annashamray
Copy link
Contributor

@alextreme dependencies didn't compile because there was a version conflict between our certifi and selenium certifi. I updated our version and it seems to work

@alextreme
Copy link
Member

Great thanks @annashamray !

.github/workflows/code-quality.yml Show resolved Hide resolved
@@ -9,6 +9,12 @@
{% if data_confirm_title %}data-confirm-title="{{ data_confirm_title }}"{% endif %}
{% if data_confirm_cancel %}data-confirm-cancel="{{ data_confirm_cancel }}"{% endif %}
{% if data_confirm_default %}data-confirm-default="{{ data_confirm_default }}"{% endif %}
{% if async_selector %}
hx-{{ method|lower }}="{{ action|default:"./" }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

action variable is not documented in template tags

Copy link
Contributor

Choose a reason for hiding this comment

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

action is not documented again

src/open_inwoner/accounts/tests/test_inbox_page.py Outdated Show resolved Hide resolved
Copy link
Member

@alextreme alextreme left a comment

Choose a reason for hiding this comment

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

Can be merged after Anna's approval

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.

Polling doesn't work on my local machine. Does it work for you?

@svenvandescheur
Copy link
Contributor Author

Polling doesn't work on my local machine. Does it work for you?

yes

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.

I have one remark about documentation which is not resolved

Polling still doesn't work for me, but maybe the problem is in my config, so I'll approve after the template variable is documented

@@ -9,6 +9,12 @@
{% if data_confirm_title %}data-confirm-title="{{ data_confirm_title }}"{% endif %}
{% if data_confirm_cancel %}data-confirm-cancel="{{ data_confirm_cancel }}"{% endif %}
{% if data_confirm_default %}data-confirm-default="{{ data_confirm_default }}"{% endif %}
{% if async_selector %}
hx-{{ method|lower }}="{{ action|default:"./" }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

action is not documented again

@annashamray
Copy link
Contributor

@Bartvaderkin @svenvandescheur
Polling works on your local machine right? How do you test it?

My steps:

  1. log in as use A, go to "Mijn berichten" page with user B
  2. in other session hijack user B, go to "Mijn berichten" and send message to user A
  3. Wait for this message to appear in the user A session
    The message doesn't appear

@svenvandescheur
Copy link
Contributor Author

@Bartvaderkin @svenvandescheur Polling works on your local machine right? How do you test it?

My steps:

  1. log in as use A, go to "Mijn berichten" page with user B
  2. in other session hijack user B, go to "Mijn berichten" and send message to user A
  3. Wait for this message to appear in the user A session
    The message doesn't appear

I made screencapture:

oippolling

@annashamray
Copy link
Contributor

Thanks! I'll test it on our test env after deployment and will try to understand why it doesn't work locally for me

@svenvandescheur svenvandescheur merged commit 25afe67 into develop Nov 28, 2022
@svenvandescheur svenvandescheur deleted the htmx branch November 28, 2022 15:31
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