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

✨ [#2149] Display new answer header for mijn vragen list #1075

Merged
merged 6 commits into from
Mar 15, 2024

Conversation

stevenbal
Copy link
Contributor

@stevenbal stevenbal commented Mar 4, 2024

task: https://taiga.maykinmedia.nl/project/open-inwoner/task/2149

This can be tested locally by connecting to the mock Klanten/Contactmomenten endpoints:

Logging in with DigiD and navigating to Mijn vragen, this should should the new answer header for the vraag, which should disappear after visiting the detail page

@stevenbal stevenbal marked this pull request as draft March 4, 2024 15:56
@stevenbal stevenbal force-pushed the feature/2149-show-new-answer-to-question branch 2 times, most recently from 35da383 to ee2189c Compare March 4, 2024 16:16
@stevenbal stevenbal changed the title 🚧 [#2149] New answer header for mijn vragen list ✨ [#2149] Display new answer header for mijn vragen list Mar 4, 2024
@stevenbal stevenbal force-pushed the feature/2149-show-new-answer-to-question branch 2 times, most recently from a496eaa to c69e652 Compare March 5, 2024 09:11
@stevenbal stevenbal force-pushed the feature/2149-show-new-answer-to-question branch from c69e652 to 863fb55 Compare March 15, 2024 08:59
Copy link
Contributor

@Bartvaderkin Bartvaderkin left a comment

Choose a reason for hiding this comment

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

Preliminary review as I'm only here in the morning.

I note you removed some of the apimocks. I don't mind and I assume there is a good reason and Paul and Jiro don't need them (I heard they actually would like more for proper list development but that could be for another time)

src/open_inwoner/openklant/admin.py Outdated Show resolved Hide resolved
src/open_inwoner/accounts/views/contactmoments.py Outdated Show resolved Hide resolved
src/open_inwoner/accounts/views/contactmoments.py Outdated Show resolved Hide resolved
src/open_inwoner/openklant/models.py Outdated Show resolved Hide resolved
src/open_inwoner/openklant/tests/test_views.py Outdated Show resolved Hide resolved
src/open_inwoner/openklant/tests/test_views.py Outdated Show resolved Hide resolved
src/open_inwoner/openklant/wrap.py Outdated Show resolved Hide resolved
src/open_inwoner/templates/pages/contactmoment/list.html Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Mar 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.02%. Comparing base (bbe2ddb) to head (2a54eda).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1075      +/-   ##
===========================================
+ Coverage    95.00%   95.02%   +0.01%     
===========================================
  Files          895      897       +2     
  Lines        31452    31559     +107     
===========================================
+ Hits         29882    29989     +107     
  Misses        1570     1570              

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

@stevenbal
Copy link
Contributor Author

I note you removed some of the apimocks.

The problem was that because the endpoints just return what is in the files, the objectcontactmomenten can only belong to one of the Contactmomenten, meaning that the detailview for the second contactmoment raises assertionerrors

@alextreme alextreme marked this pull request as ready for review March 15, 2024 13:07
@stevenbal stevenbal marked this pull request as draft March 15, 2024 13:32
@stevenbal stevenbal force-pushed the feature/2149-show-new-answer-to-question branch from 6ef5bc7 to 1310a23 Compare March 15, 2024 13:32
@stevenbal stevenbal force-pushed the feature/2149-show-new-answer-to-question branch from 1310a23 to 85fe1bd Compare March 15, 2024 13:33
replaced KlantCntactMomentLocal.klantcontactmoment_url with contactmoment_url, so that we can easily get the local mapping without having to fetch KlantContactMoment resources
@stevenbal stevenbal force-pushed the feature/2149-show-new-answer-to-question branch from 85fe1bd to 05958dd Compare March 15, 2024 13:49
@stevenbal stevenbal marked this pull request as ready for review March 15, 2024 13:50
@pi-sigma
Copy link
Contributor

pi-sigma commented Mar 15, 2024

Following the link of a question in the case detail page produces an unhandled AtributeError because of the missing mock (or did I fail to configure something?):

Traceback (most recent call last):
  File "/home/pi-sigma/.virtualenvs/oi-main/lib/python3.11/site-packages/django/core/handlers/exception.py", line 55, in inner
    response = get_response(request)
               ^^^^^^^^^^^^^^^^^^^^^
  File "/home/pi-sigma/.virtualenvs/oi-main/lib/python3.11/site-packages/django/core/handlers/base.py", line 197, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/pi-sigma/.virtualenvs/oi-main/lib/python3.11/site-packages/cms/utils/decorators.py", line 20, in inner
    return func(request, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/pi-sigma/.virtualenvs/oi-main/lib/python3.11/site-packages/django/views/generic/base.py", line 104, in view
    return self.dispatch(request, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/pi-sigma/dev/maykin/open-inwoner/src/open_inwoner/accounts/views/contactmoments.py", line 57, in dispatch
    return super().dispatch(request, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/pi-sigma/.virtualenvs/oi-main/lib/python3.11/site-packages/django/views/generic/base.py", line 143, in dispatch
    return handler(request, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/pi-sigma/dev/maykin/open-inwoner/src/open_inwoner/accounts/views/contactmoments.py", line 252, in get
    kcms = contactmoment_client.retrieve_klantcontactmomenten_for_klant(klant)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/pi-sigma/dev/maykin/open-inwoner/src/open_inwoner/openklant/clients.py", line 224, in retrieve_klantcontactmomenten_for_klant
    params={"klant": klant.url},
                     ^^^^^^^^^

Exception Type: AttributeError at /mijn-aanvragen/contactmoment/aaaaaaaa-aaaa-aaaa-aaaa-bbbbbbbbbbb2/
Exception Value: 'NoneType' object has no attribute 'url'

Judging from the tests I think it will work when data is present.

I stumbled over the API mocks myself and created an issue for it: https://taiga.maykinmedia.nl/project/open-inwoner/issue/2196

We can discuss on Monday if we want to further modify/expand them so that they're more useful

Copy link
Contributor

@pi-sigma pi-sigma left a comment

Choose a reason for hiding this comment

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

I don't know if the AttributeError is due to misconfiguration on my part or the missing mock.

A few remarks about the placement of KlantcontactMomenten in the admin the naming of classes/functions. Doesn't have to be changed, but double-check.

src/open_inwoner/conf/fixtures/django-admin-index.json Outdated Show resolved Hide resolved
src/open_inwoner/openklant/models.py Outdated Show resolved Hide resolved
src/open_inwoner/openklant/tests/test_views.py Outdated Show resolved Hide resolved
src/open_inwoner/openklant/tests/test_views.py Outdated Show resolved Hide resolved
src/open_inwoner/openklant/wrap.py Show resolved Hide resolved
@stevenbal
Copy link
Contributor Author

I don't know if the AttributeError is due to misconfiguration on my part or the missing mock.

do you have the Klanten API configured in your local Open Klant configuration?

@stevenbal stevenbal requested a review from pi-sigma March 15, 2024 15:28
@pi-sigma
Copy link
Contributor

I do have the API configured but I receive a 401 Client Error for the call to our openklant-read test API. Strange as it worked before and now doesn't, but the problem seems to be on my end.

@alextreme alextreme merged commit 761f3e2 into develop Mar 15, 2024
15 checks passed
@alextreme alextreme deleted the feature/2149-show-new-answer-to-question branch March 15, 2024 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants