[#152] save user metrics preferences in database #166
Conversation
backend/projects/serializers.py
Outdated
| @@ -143,6 +145,20 @@ def _has_siblings(self, obj) -> bool: | |||
| > 1 | |||
| ) | |||
|
|
|||
| def _user_metrics(self, obj): | |||
| metrics = list( | |||
| MetricsPreferences.objects.filter(project=obj).filter( | |||
There was a problem hiding this comment.
You can put both arguments in the same filter, with
MetricsPreferences.objects.filter(
project=obj,
user_id=self.context.get("user_id")
)
There was a problem hiding this comment.
I think that here you would not get the unique MetricsPreference object that corresponds to user and project, but rather the list of MetricPreferences right ?
MathildeDuboille
force-pushed
the
feature/152-save-user-metrics-preferences
branch
from
7dc3e17
to
099fad0
Compare
MathildeDuboille
changed the title
| data = JSONParser().parse(request) | ||
| project_id = data["project"] | ||
| new_metrics = data["metrics"] | ||
| metrics = MetricsPreferences.objects.filter( |
There was a problem hiding this comment.
you can use the update_or_create function
https://docs.djangoproject.com/en/3.0/ref/models/querysets/#update-or-create
backend/projects/serializers.py
Outdated
| user_id=self.context.get("user_id") | ||
| ) | ||
| ) | ||
| if len(metrics) == 0: |
There was a problem hiding this comment.
Are that the default metrics? Shouldn’t we set that as a default in the DB migration?
There was a problem hiding this comment.
If we set these metrics as default in the DB migration, we need to insert a line for each user and for each of his/her project in the table. So I don't think this is ideal, what do you think ?
There was a problem hiding this comment.
I second @MathildeDuboille, this will save a lot of space in the database
There was a problem hiding this comment.
Hmm I see your point. However I think that the typical Falco instance has few users and few projects. Even for Theodo’s Falco (which I think would be one of the biggest one), we have 43 projects and ~130 user, and every user averages 1-2 projects. So that would mean <300 lines in DB, which I think is definitely manageable.
I might be mistaken, but wouldn’t using the DB default to store the three defaults simplify the mechanism for creating / updating the metrics ? For one, creation would be already taken care of. Also, there’s something “weird” about asking for an object in an API, receiving one, and not being able to tell if 1) that’s the state of the DB 2) the DB’s empty and we’re receiving default.
There was a problem hiding this comment.
Also I should note that I do not consider this topic as blocking, and we can totally merge this and have this discussion in a later issue. Just think it’s an interesting discussion!
There was a problem hiding this comment.
FYI, the free DB instance in Heroku is capped at 10,000 lines, and the cheapest one (the one we have at Theodo) is capped at 10,000,000 lines.
So either you do not pay and will likely have only a few number of projects (otherwise you’ll reach the max pretty quickly), or you do pay and a few hundred lines are quite insignificant.
There was a problem hiding this comment.
Ok that's convincing, we should put these default metrics in the database
backend/projects/views.py
Outdated
|
|
||
| @swagger_auto_schema( | ||
| methods=["post"], | ||
| responses={200: openapi.Response("Updates a metric preference.")}, |
There was a problem hiding this comment.
Updates a user’s metric preferences for a given project
| def metrics(request): | ||
| data = JSONParser().parse(request) | ||
| project_id = data["project"] | ||
| new_metrics = data["metrics"] |
There was a problem hiding this comment.
what happens if a metric is not recognized here (either because of a bug or because someone is trying to attack this endpoint)? Do we reply with a 400 or a 500?
I think we should reply with a 400, meaning that we might have to check here whether the metrics sent by the users all belong to the list of “accepted” metrics.
backend/projects/views.py
Outdated
| new_metric_preferences.save() | ||
| else: | ||
| metrics.update(metrics=new_metrics) | ||
| return JsonResponse({}) |
There was a problem hiding this comment.
You could reply with the metrics object that was created or updated
There was a problem hiding this comment.
(I consider it good practice when POSTing or PATCHing an object to reply with the created or updated object in the response body)
| case getType(updateAllDisplayedMetrics): | ||
| return { | ||
| ...state, | ||
| displayedMetrics: { |
There was a problem hiding this comment.
Could we rename this to currentDisplayedMetrics to be consistent with the naming of the other parameters in this Redux state?
| @@ -157,6 +166,14 @@ function* fetchProjects(action: ActionType<typeof fetchProjectsRequest>) { | |||
| true, | |||
| null, | |||
| ); | |||
| const displayedMetrics = projects.reduce( | |||
There was a problem hiding this comment.
Is is necessary to have the displayedMetrics for all projects, even though we only show projects one at a time? It looks like the page ID, parameters, etc… are scoped as the current page.
I’ll have a deeper look into the Redux structure and try to figure out a solution more in line with the rest of the Redux mechanics.
There was a problem hiding this comment.
From what I see in the network tab in the console, we don't call /projects/<id> every time we switch to another project, so it may be a problem if we don't store all the metrics when we fetch all projects :/
There was a problem hiding this comment.
OK got it! You could maybe rename displayedMetrics into displayedMetricsByProjectId so that it becomes obvious what the id in the object stands for.
There was a problem hiding this comment.
Another solution would be to send the displayed_metrics with the GET /projects/first and GET /projects and store that info inside each project’s information. That would however require a number of changes in this PR. @fargito what do you think?
There was a problem hiding this comment.
I agree, the metrics should be handled at project level
backend/projects/urls.py
Outdated
| @@ -22,4 +22,5 @@ | |||
| ), | |||
| path("<uuid:project_uuid>/scripts", views.project_scripts), | |||
| path("<uuid:project_uuid>/scripts/<uuid:script_uuid>", views.project_script_detail), | |||
| path("metrics", views.metrics), | |||
There was a problem hiding this comment.
Shouldn't the url be set by project ?
…. Delete it when user is removed
|
SonarCloud Quality Gate failed.
|
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Other information
Closes: #152