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.

Sign up for GitHub

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

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

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
701e77c
add user metrics in api projects response
MathildeDuboille Mar 31, 2020
3b1a3da
create route to save metrics preferences and connect it to the frontend
MathildeDuboille Apr 1, 2020
099fad0
remove outdated test
MathildeDuboille Apr 1, 2020
5fc06cb
use update or create function in metrics view
MathildeDuboille Apr 2, 2020
1289a58
validate body request
MathildeDuboille Apr 3, 2020
c954dbb
remove displayedMetrics object from parameters and get metrics from p…
MathildeDuboille Apr 3, 2020
c682ac8
fix linter
MathildeDuboille Apr 3, 2020
de799f2
insert user default metric preference when member is added to project…
MathildeDuboille Apr 3, 2020
cb5e24a
delete outdated test
MathildeDuboille Apr 3, 2020
d3ab6c9
remove create_or_update use
MathildeDuboille Apr 3, 2020
a84603e
move signals into models file
MathildeDuboille Apr 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions backend/projects/migrations/0032_metricspreferences.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
# Generated by Django 3.0.3 on 2020-03-31 14:56

from django.conf import settings
import django.contrib.postgres.fields
from django.db import migrations, models
import django.db.models.deletion
import uuid


class Migration(migrations.Migration):

dependencies = [
migrations.swappable_dependency(settings.AUTH_USER_MODEL),
('projects', '0031_auto_20191122_1154'),
]

operations = [
migrations.CreateModel(
name='MetricsPreferences',
fields=[
('uuid', models.UUIDField(default=uuid.uuid4, editable=False, primary_key=True, serialize=False)),
('created_at', models.DateTimeField(auto_now_add=True)),
('updated_at', models.DateTimeField(auto_now=True)),
('metrics', django.contrib.postgres.fields.ArrayField(base_field=models.CharField(max_length=100), size=None)),
('project', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='projects.Project')),
('user', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL)),
],
options={
'ordering': ('-created_at',),
'get_latest_by': 'created_at',
'abstract': False,
},
),
]
7 changes: 7 additions & 0 deletions backend/projects/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from django.db import models
from django import forms
from fernet_fields import EncryptedTextField
from django.contrib.postgres.fields import ArrayField


class Project(BaseModel):
Expand Down Expand Up @@ -57,6 +58,12 @@ class Meta:
unique_together = ("project", "user")


class MetricsPreferences(BaseModel):
user = models.ForeignKey(User, on_delete=models.CASCADE)
project = models.ForeignKey(Project, on_delete=models.CASCADE)
metrics = ArrayField(models.CharField(max_length=100))
fargito marked this conversation as resolved.
Show resolved Hide resolved


fargito marked this conversation as resolved.
Show resolved Hide resolved
class Page(BaseModel):
name = models.CharField(max_length=100)
url = models.CharField(max_length=500)
Expand Down
17 changes: 17 additions & 0 deletions backend/projects/serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
ProjectMemberRole,
Script,
AvailableAuditParameters,
MetricsPreferences,
)

from rest_framework import serializers
Expand Down Expand Up @@ -134,6 +135,7 @@ class Meta:

class ProjectSerializer(DynamicFieldsModelSerializer):
has_siblings = serializers.SerializerMethodField("_has_siblings")
user_metrics = serializers.SerializerMethodField("_user_metrics")

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

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

@fargito fargito Mar 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

You can put both arguments in the same filter, with

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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 ?

user_id=self.context.get("user_id")
)
)
if len(metrics) == 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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!

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

metrics = [
"WPTMetricFirstViewTTI",
"WPTMetricFirstViewSpeedIndex",
"WPTMetricFirstViewLoadTime",
]
return metrics

pages = PageSerializer(many=True)
scripts = ScriptSerializer(many=True)
audit_parameters_list = ProjectAuditParametersSerializer(many=True)
Expand All @@ -164,4 +180,5 @@ class Meta:
"wpt_api_key",
"wpt_instance_url",
"has_siblings",
"user_metrics",
)
1 change: 1 addition & 0 deletions backend/projects/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ def project_detail(request, project_uuid):
"audit_parameters_list",
"screenshot_url",
"latest_audit_at",
"user_metrics",
),
context={"user_id": request.user.id},
)
Expand Down