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

Implementatin mpsks clients #504

Open
wants to merge 42 commits into
base: develop
Choose a base branch
from
Open

Conversation

agmes4
Copy link
Member

@agmes4 agmes4 commented Dec 25, 2024

Content

Sipa implementation for mpsk clients so users can create delete and change them on there own

Please make sure though you did the following (tick off everything you
already did):

  • Run the tests and see them pass
  • Rebase your branch on top of develop
  • Include tests for features you introduced / bugs you fixed

added the first implentation for the client it self
still a lot to do
#498
added mpsk client to user profile
#498
added mpsks form implementation for etiting and adding clients
#498
updated html for mpsks
#498
added a html containing the table information for the mpsk clients
#498
fixed the mapping for the usersuite so the information can be loaded from the client
#498
added a form for deleting mpsks clients
minor fixes for mpskclientform
#498
added the add change and delete methods for the sample backend
#498
added the href to the table showing the mpsk clients
#498
added a model for sipa representing mpsks clients
#498
added the sipa endpoints for the pycroft fetch
#498
In the case of a non-displayable property, there is now a boolean to indicate that such a property cannot be displayed
There is an opt-out
#498
added a model for sipa representing mpsks clients
#498
moved from url args to backend values
#498
added the backend implentation for the api to pycroft
and sample user
#498
moved from url args to backend values
#498
Copy link

codecov bot commented Dec 25, 2024

Codecov Report

Attention: Patch coverage is 75.17730% with 35 lines in your changes missing coverage. Please review.

Project coverage is 31.78%. Comparing base (e4b2c30) to head (22881f1).

Files with missing lines Patch % Lines
sipa/blueprints/usersuite.py 69.09% 17 Missing ⚠️
sipa/model/pycroft/user.py 33.33% 8 Missing ⚠️
sipa/model/pycroft/api.py 50.00% 4 Missing ⚠️
sipa/model/sample/user.py 73.33% 4 Missing ⚠️
sipa/forms.py 83.33% 1 Missing ⚠️
sipa/model/user.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #504      +/-   ##
===========================================
+ Coverage    31.65%   31.78%   +0.13%     
===========================================
  Files           68       69       +1     
  Lines         6951     7239     +288     
===========================================
+ Hits          2200     2301     +101     
- Misses        4751     4938     +187     

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

Copy link
Collaborator

@lukasjuhrich lukasjuhrich left a comment

Choose a reason for hiding this comment

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

Checklist / still missing:

  • Tests
  • translations

sipa/blueprints/usersuite.py Outdated Show resolved Hide resolved
sipa/blueprints/usersuite.py Outdated Show resolved Hide resolved
sipa/model/mspk_client.py Outdated Show resolved Hide resolved
sipa/model/pycroft/user.py Outdated Show resolved Hide resolved
sipa/model/sample/user.py Outdated Show resolved Hide resolved
sipa/blueprints/usersuite.py Outdated Show resolved Hide resolved
Length(-1, 30, lazy_gettext("Gerätename zu lang"))],
description=lazy_gettext("TL-WR841N, MacBook, FritzBox, PC, Laptop, o.Ä."),
)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use two newlines to separate top-level constructs

sipa/model/pycroft/user.py Outdated Show resolved Hide resolved
sipa/model/sample/user.py Outdated Show resolved Hide resolved
Comment on lines 114 to 118
{% if property.displayable %}
<span class="{{ style }}">{{ property.value }}</span>
{% else %}
<span class="{{ style }}"></span>
{% endif %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

what precisely is the point of introducing displayable to set it to False in the sample backend?
I'm not sure what this is supposed to achieve.

Copy link
Member Author

Choose a reason for hiding this comment

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

otherwise it will try to display the clients which is not feasible cause of there amount. So I Tried to mitigate this by introducing the displayable. But forgot it in the pycorft backend

did for better readability a refactoring
refactoring for better readability change mpsk clients function
removed unnecessary \n
@FestplattenSchnitzel
Copy link
Member

I can't enter the usersuite unfortunately:

CRITICAL 2024-12-27 11:56:04,613 sipa.blueprints.generic Backend error: pycroft
Traceback (most recent call last):
  File "/tmp/sipa/sipa/model/pycroft/user.py", line 44, in __init__
    self.user_data: UserData = UserData.model_validate(user_data)
                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/sipa/venv/lib/python3.12/site-packages/pydantic/main.py", line 503, in model_validate
    return cls.__pydantic_validator__.validate_python(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
pydantic_core._pydantic_core.ValidationError: 1 validation error for UserData
mpsk_clients
  Field required [type=missing, input_value={'birthdate': '1111-11-11..., 'wifi_password': None}, input_type=dict]
    For further information visit https://errors.pydantic.dev/2.4/v/missing

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/tmp/sipa/venv/lib/python3.12/site-packages/flask/app.py", line 880, in full_dispatch_request
    rv = self.dispatch_request()
         ^^^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/sipa/venv/lib/python3.12/site-packages/flask/app.py", line 865, in dispatch_request
    return self.ensure_sync(self.view_functions[rule.endpoint])(**view_args)  # type: ignore[no-any-return]
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/sipa/sipa/blueprints/generic.py", line 143, in login
    user = User.authenticate(username, password)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/sipa/sipa/model/pycroft/user.py", line 78, in authenticate
    user = cls.get(result['id'])
           ^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/sipa/sipa/model/pycroft/user.py", line 57, in get
    return cls(user_data)
           ^^^^^^^^^^^^^^
  File "/tmp/sipa/sipa/model/pycroft/user.py", line 47, in __init__
    raise PycroftBackendError("Error when parsing user lookup response") from e
sipa.model.pycroft.exc.PycroftBackendError: Error when parsing user lookup response

@agmes4
Copy link
Member Author

agmes4 commented Dec 27, 2024

interesting yesterday it worked for me will check

@agmes4
Copy link
Member Author

agmes4 commented Dec 27, 2024

just forgot to push a naming change sorry

Copy link
Member

@FestplattenSchnitzel FestplattenSchnitzel left a comment

Choose a reason for hiding this comment

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

Unfortunately, the error persists. Are you using a modified version of Pycroft? The error occurs b/c there is no mpsk_clients field in the data returned by the Pycroft API.

fixed a bug resulting in unable to add mpsk clients
refactored code and removed unnecessary assignment
in order to not display the mpsk clients on the user suite its necessary to have the possibility to hide a certain property. Otherwise it will not display nicely
for better errorhandling
fixed the changing of mpsk clients
added the displayable arg to sample users property capabilities
its checked rather the to delete mpsk client exists for the user
refactoring
changed the changing for the sample backends changing mpsk
added tests for mpsk
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.

3 participants