feat: store user ui preferences #522
Conversation
acellard
force-pushed
the
feat/user_preferences
branch
2 times, most recently
from
b0df88b
to
cd4abe8
Compare
backend/users/serializers/user.py
Outdated
| return ui_preferences | ||
|
|
||
| def update(self, instance, validated_data): | ||
| instance.set_password(validated_data.get("password")) |
There was a problem hiding this comment.
Maybe this should only be done if there is a new password?
There was a problem hiding this comment.
Yes indeed, thanks! Changed.
acellard
force-pushed
the
feat/user_preferences
branch
from
f4d7dea
to
a799ed0
Compare
acellard
requested review from
AlexandrePicosson,
AurelienGasser,
cboillet-dev,
Kelvin-M,
mblottiere,
samlesu and
sergebouchut2
as code owners
| @@ -51,7 +52,7 @@ def test_user_create_duplicate(self): | |||
| response = AuthenticatedClient(role=UserChannel.Role.ADMIN, channel=self.channel).post(self.url, data=data) | |||
|
|
|||
| assert response.status_code == status.HTTP_400_BAD_REQUEST | |||
| assert response.data.get("message") == "Username already exists" | |||
| assert response.data.get("username")[0] == "A user with that username already exists." | |||
There was a problem hiding this comment.
This API change needs to be reflected in the frontend: the message was directly displayed to the user.
There was a problem hiding this comment.
So as last commit, I leveraged django validation handling:
HTTP 400 Bad Request response with data format : {field: [error list on this field]}.
Is this convenient for clients ? Or should we catch validation exception and reformat response message to have something like {message: 'error string'}.
I feel it's convenient to use what django already does but I don't have a strong opinion on this.
backend/users/tests/test_user.py
Outdated
| @parameterized.expand( | ||
| [ | ||
| ({"role": UserChannel.Role.USER},), | ||
| ({"password": "newpas$w0rdtestofdrea6S43"},), |
There was a problem hiding this comment.
I feel updating the password should be done in a separate test with a check that the password is indeed updated.
There was a problem hiding this comment.
Separate test added
backend/users/models/user_channel.py
Outdated
| @@ -16,3 +16,4 @@ class Role(models.TextChoices): | |||
| settings.AUTH_USER_MODEL, parent_link=True, on_delete=models.CASCADE, related_name="channel" | |||
| ) | |||
| channel_name = models.CharField(max_length=100) | |||
| ui_preferences = models.JSONField(null=True) | |||
There was a problem hiding this comment.
would it be possible to not have null values but a default at {} instead?
There was a problem hiding this comment.
I hesitated between null or {} default. I initially chose null to avoid migration but from frontend pov I agree, it's better to avoid null values. Modified and migration added.
backend/users/serializers/user.py
Outdated
| from django.contrib.auth.models import User | ||
| from django.contrib.auth.password_validation import validate_password | ||
| from django.core.exceptions import ValidationError as djangoValidationError |
There was a problem hiding this comment.
missing capital "D" for DjangoValidationError
backend/users/serializers/user.py
Outdated
|
|
||
| def validate_ui_preferences(self, value): | ||
| if type(value) is not dict: | ||
| raise serializers.ValidationError({"UI preferences should be a dict"}) |
There was a problem hiding this comment.
Aren't you missing the ui_preferences key in {"ui_preferences": "UI preferences should be a dict"}?
There was a problem hiding this comment.
To keep consistency yes, key added.
backend/users/tests/test_user.py
Outdated
| [ | ||
| ({"role": UserChannel.Role.USER},), | ||
| ({"password": "newpas$w0rdtestofdrea6S43"},), | ||
| ({"ui_preferences": {"columns": ["col1", "col2"]}},), |
There was a problem hiding this comment.
Could you add a test on the fact that ui_preferences is partially updated and not fully replaced? That is keys that were not in the request payload are left untouched.
backend/users/views/user.py
Outdated
| username = kwargs.get("username") | ||
| instance = self.user_model.objects.get(username=username) |
There was a problem hiding this comment.
Did you test this with a username like "John Doe"? self.get_object() is able to decode the urlencodedentities but kwargs only contains raw values that need to be decoded manually.
There was a problem hiding this comment.
Updated the test and it seems to work yes. But still reverting to get_object(). For context, in certain cases there was an uncatch exception in get_object(). But this was fixed here https://github.com/Substra/substra-backend/pull/522/files#diff-fa54e25c9fbe9c2560d0fbef5cb51cb8f37b0c7793d322f409194417453082bbR62 so it will be catch before.
acellard
force-pushed
the
feat/user_preferences
branch
from
a799ed0
to
fa096d6
Compare
Signed-off-by: Alison Cellard <alison.cellard@owkin.com>
Signed-off-by: Alison Cellard <alison.cellard@owkin.com>
Signed-off-by: Alison Cellard <alison.cellard@owkin.com>
Signed-off-by: Alison Cellard <alison.cellard@owkin.com>
Milouu
force-pushed
the
feat/user_preferences
branch
from
fa096d6
to
1114ca8
Compare
Milouu
requested review from
oleobal,
Milouu,
guilhem-barthes and
ThibaultFy
|
Thank you for your contribution! Unfortunately, due to organisational and roadmap changes, this item has been deprioritised. We currently have no bandwidth to cope with the code drift between the current code state in the last release and this PR, therefore we are closing it. |
Description
This PR is a result of October 27th "flash hackathon" involving @Milouu and @acellard. The goal was to figure out how to properly manage user ui preferences in the backend, and provide api to get, retrieve and update them.
For now, such preferences involve columns display list and favorites compute plans. With likely more preferences in the future.
To keep it flexible and leave the responsibility to the frontend, the preferences are stored in a json field.
This PR also does some refactoring to use serializer to validate data.