[#152] save user metrics preferences in database #166

MathildeDuboille · 2020-03-31T15:40:44Z

PR Checklist

Please check if your PR fulfills the following requirements:

Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Other information

Closes: #152

fargito · 2020-03-31T15:51:46Z

backend/projects/serializers.py

@@ -143,6 +145,20 @@ def _has_siblings(self, obj) -> bool:
            > 1
        )

+    def _user_metrics(self, obj):
+        metrics = list(
+            MetricsPreferences.objects.filter(project=obj).filter(


You can put both arguments in the same filter, with

MetricsPreferences.objects.filter( project=obj, user_id=self.context.get("user_id") )

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 ?

backend/projects/models.py

fargito · 2020-04-01T17:34:10Z

backend/projects/views.py

+    data = JSONParser().parse(request)
+    project_id = data["project"]
+    new_metrics = data["metrics"]
+    metrics = MetricsPreferences.objects.filter(


you can use the update_or_create function
https://docs.djangoproject.com/en/3.0/ref/models/querysets/#update-or-create

phacks · 2020-04-02T12:42:22Z

backend/projects/serializers.py

+                user_id=self.context.get("user_id")
+            )
+        )
+        if len(metrics) == 0:


Are that the default metrics? Shouldn’t we set that as a default in the DB migration?

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 ?

I second @MathildeDuboille, this will save a lot of space in the database

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.

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!

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.

Ok that's convincing, we should put these default metrics in the database

phacks · 2020-04-02T12:43:13Z

backend/projects/views.py

+
+@swagger_auto_schema(
+    methods=["post"],
+    responses={200: openapi.Response("Updates a metric preference.")},


Updates a user’s metric preferences for a given project

phacks · 2020-04-02T12:45:21Z

backend/projects/views.py

+def metrics(request):
+    data = JSONParser().parse(request)
+    project_id = data["project"]
+    new_metrics = data["metrics"]


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.

phacks · 2020-04-02T12:45:53Z

backend/projects/views.py

+        new_metric_preferences.save()
+    else:
+        metrics.update(metrics=new_metrics)
+    return JsonResponse({})


You could reply with the metrics object that was created or updated

(I consider it good practice when POSTing or PATCHing an object to reply with the created or updated object in the response body)

phacks · 2020-04-02T12:50:05Z

frontend/src/redux/parameters/reducer.ts

+    case getType(updateAllDisplayedMetrics):
+      return {
+        ...state,
+        displayedMetrics: {


Could we rename this to currentDisplayedMetrics to be consistent with the naming of the other parameters in this Redux state?

phacks · 2020-04-02T12:52:51Z

frontend/src/redux/entities/projects/sagas.ts

@@ -157,6 +166,14 @@ function* fetchProjects(action: ActionType<typeof fetchProjectsRequest>) {
    true,
    null,
  );
+  const displayedMetrics = projects.reduce(


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.

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 :/

OK got it! You could maybe rename displayedMetrics into displayedMetricsByProjectId so that it becomes obvious what the id in the object stands for.

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?

I agree, the metrics should be handled at project level

fargito · 2020-04-02T19:17:08Z

backend/projects/urls.py

@@ -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),


Shouldn't the url be set by project ?

frontend/src/pages/Audits/Audits.tsx

frontend/src/redux/entities/projects/actions.ts

…rojects by Id object

…. Delete it when user is removed

sonarcloud · 2020-04-03T16:23:58Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
6 Code Smells

No Coverage information
6.1% Duplication

phacks temporarily deployed to falco-feature-152-save--u5lzlm March 31, 2020 15:44 Inactive

fargito reviewed Mar 31, 2020

View reviewed changes

backend/projects/models.py Outdated Show resolved Hide resolved

fargito reviewed Mar 31, 2020

View reviewed changes

backend/projects/models.py Show resolved Hide resolved

phacks temporarily deployed to falco-feature-152-save--u5lzlm April 1, 2020 14:01 Inactive

phacks temporarily deployed to falco-feature-152-save--u5lzlm April 1, 2020 14:17 Inactive

MathildeDuboille added 3 commits April 1, 2020 16:18

add user metrics in api projects response

701e77c

create route to save metrics preferences and connect it to the frontend

3b1a3da

remove outdated test

099fad0

MathildeDuboille force-pushed the feature/152-save-user-metrics-preferences branch from 7dc3e17 to 099fad0 Compare April 1, 2020 14:20

phacks temporarily deployed to falco-feature-152-save--u5lzlm April 1, 2020 14:20 Inactive

MathildeDuboille changed the title ~~[WIP] save user metrics preferences in database~~ [#152] save user metrics preferences in database Apr 1, 2020

fargito reviewed Apr 1, 2020

View reviewed changes

use update or create function in metrics view

5fc06cb

phacks temporarily deployed to falco-feature-152-save--u5lzlm April 2, 2020 09:57 Inactive

phacks reviewed Apr 2, 2020

View reviewed changes

fargito reviewed Apr 2, 2020

View reviewed changes

frontend/src/pages/Audits/Audits.tsx Outdated Show resolved Hide resolved

fargito reviewed Apr 2, 2020

View reviewed changes

frontend/src/redux/entities/projects/actions.ts Outdated Show resolved Hide resolved

MathildeDuboille added 3 commits April 3, 2020 11:34

validate body request

1289a58

remove displayedMetrics object from parameters and get metrics from p…

c954dbb

…rojects by Id object

fix linter

c682ac8

phacks temporarily deployed to falco-feature-152-save--u5lzlm April 3, 2020 10:03 Inactive

insert user default metric preference when member is added to project…

de799f2

…. Delete it when user is removed

phacks temporarily deployed to falco-feature-152-save--u5lzlm April 3, 2020 15:02 Inactive

MathildeDuboille added 2 commits April 3, 2020 17:08

delete outdated test

cb5e24a

remove create_or_update use

d3ab6c9

phacks temporarily deployed to falco-feature-152-save--u5lzlm April 3, 2020 15:13 Inactive

move signals into models file

a84603e

phacks temporarily deployed to falco-feature-152-save--u5lzlm April 3, 2020 16:23 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#152] save user metrics preferences in database #166

[#152] save user metrics preferences in database #166

MathildeDuboille commented Mar 31, 2020 •

edited

fargito Mar 31, 2020 •

edited

fargito Mar 31, 2020

fargito Apr 1, 2020

phacks Apr 2, 2020

MathildeDuboille Apr 2, 2020

fargito Apr 2, 2020

phacks Apr 2, 2020

phacks Apr 2, 2020

phacks Apr 2, 2020

fargito Apr 2, 2020

phacks Apr 2, 2020

phacks Apr 2, 2020

phacks Apr 2, 2020

phacks Apr 2, 2020

phacks Apr 2, 2020

phacks Apr 2, 2020

MathildeDuboille Apr 2, 2020

phacks Apr 2, 2020

phacks Apr 2, 2020 •

edited

fargito Apr 2, 2020

fargito Apr 2, 2020

sonarcloud bot commented Apr 3, 2020

[#152] save user metrics preferences in database #166

Are you sure you want to change the base?

[#152] save user metrics preferences in database #166

Conversation

MathildeDuboille commented Mar 31, 2020 • edited

PR Checklist

PR Type

Other information

fargito Mar 31, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phacks Apr 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Apr 3, 2020

MathildeDuboille commented Mar 31, 2020 •

edited

fargito Mar 31, 2020 •

edited

phacks Apr 2, 2020 •

edited