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

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

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
8 changes: 3 additions & 5 deletions backend/projects/migrations/0032_metricspreferences.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Generated by Django 3.0.3 on 2020-03-31 14:56
# Generated by Django 3.0.3 on 2020-04-01 13:36

from django.conf import settings
import django.contrib.postgres.fields
Expand All @@ -21,14 +21,12 @@ class Migration(migrations.Migration):
('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)),
('metrics', django.contrib.postgres.fields.ArrayField(base_field=models.CharField(max_length=100), null=True, 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,
'unique_together': {('project', 'user')},
},
),
]
5 changes: 4 additions & 1 deletion backend/projects/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,10 @@ class Meta:
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))
metrics = ArrayField(models.CharField(max_length=100), null=True)

class Meta:
unique_together = ("project", "user")


fargito marked this conversation as resolved.
Show resolved Hide resolved
class Page(BaseModel):
Expand Down
22 changes: 11 additions & 11 deletions backend/projects/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -146,18 +146,18 @@ def _has_siblings(self, obj) -> bool:
)

def _user_metrics(self, obj):
metrics = list(
MetricsPreferences.objects.filter(project=obj).filter(
user_id=self.context.get("user_id")
)
metrics = MetricsPreferences.objects.filter(
project=obj, user_id=self.context.get("user_id")
)
if len(metrics) == 0:
metrics = [
"WPTMetricFirstViewTTI",
"WPTMetricFirstViewSpeedIndex",
"WPTMetricFirstViewLoadTime",
]
return metrics
if metrics:
metrics = metrics.values_list("metrics", flat=True).get()
if metrics is not None and len(metrics) > 0:
return metrics
return [
"WPTMetricFirstViewTTI",
"WPTMetricFirstViewSpeedIndex",
"WPTMetricFirstViewLoadTime",
]

pages = PageSerializer(many=True)
scripts = ScriptSerializer(many=True)
Expand Down
1 change: 1 addition & 0 deletions backend/projects/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the url be set by project ?

]
25 changes: 25 additions & 0 deletions backend/projects/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
ProjectAuditParameters,
AvailableAuditParameters,
Script,
MetricsPreferences,
)
from projects.serializers import (
PageSerializer,
Expand Down Expand Up @@ -529,3 +530,27 @@ def project_script_detail(request, project_uuid, script_uuid):
check_if_admin_of_project(request.user.id, project.uuid)
script.delete()
return JsonResponse({}, status=status.HTTP_204_NO_CONTENT)


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

Choose a reason for hiding this comment

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

Updates a user’s metric preferences for a given project

tags=["Metrics"],
)
@api_view(["POST"])
@permission_classes([permissions.IsAuthenticated])
def metrics(request):
data = JSONParser().parse(request)
project_id = data["project"]
new_metrics = data["metrics"]
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

metrics = MetricsPreferences.objects.filter(
Copy link
Contributor

Choose a reason for hiding this comment

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

project_id=project_id, user_id=request.user.id
)
if not metrics:
new_metric_preferences = MetricsPreferences(
project_id=project_id, user_id=request.user.id, metrics=new_metrics
)
new_metric_preferences.save()
else:
metrics.update(metrics=new_metrics)
return JsonResponse({})
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

46 changes: 42 additions & 4 deletions frontend/src/pages/Audits/Audits.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { ValueType } from 'react-select/lib/types';

import { AuditParametersType } from 'redux/entities/auditParameters/types';
import { PageType } from 'redux/entities/pages/types';
import { ProjectType } from 'redux/entities/projects/types';
import {ProjectToastrDisplayType, ProjectType} from 'redux/entities/projects/types';
fargito marked this conversation as resolved.
Show resolved Hide resolved
import { ScriptType } from 'redux/entities/scripts/types';

import Badge from 'components/Badge';
Expand All @@ -13,6 +13,8 @@ import MessagePill from 'components/MessagePill';
import Select from 'components/Select';
import dayjs from 'dayjs';
import { FormattedMessage, InjectedIntlProps } from 'react-intl';
import ReduxToastr, { toastr } from 'react-redux-toastr';
import 'react-redux-toastr/lib/css/react-redux-toastr.min.css';
import { auditStatus, AuditStatusHistoryType } from 'redux/entities/auditStatusHistories/types';
import { useFetchProjectIfUndefined } from 'redux/entities/projects/useFetchProjectIfUndefined';
import { routeDefinitions } from 'routes';
Expand Down Expand Up @@ -62,6 +64,8 @@ type Props = {
setCurrentPageId: (pageId: string | null | undefined) => void;
setCurrentScriptId: (scriptId: string | null | undefined) => void;
setCurrentScriptStepId: (scriptStepId: string | null | undefined) => void;
toastrDisplay: ProjectToastrDisplayType;
setProjectToastrDisplay: (toastrDisplay: ProjectToastrDisplayType) => void;
} & OwnProps &
InjectedIntlProps;

Expand All @@ -84,6 +88,8 @@ export const Audits: React.FunctionComponent<Props> = ({
setCurrentPageId,
setCurrentScriptId,
setCurrentScriptStepId,
toastrDisplay,
setProjectToastrDisplay,
}) => {
const { projectId, pageOrScriptId, auditParametersId, scriptStepId } = match.params;

Expand Down Expand Up @@ -138,6 +144,30 @@ export const Audits: React.FunctionComponent<Props> = ({
[script && script.uuid, scriptStepId, setCurrentScriptStepId],
);

React.useEffect(
() => {
if ('' !== toastrDisplay) {
switch (toastrDisplay) {
case 'updateDisplayedMetricsSuccess':
toastr.success(
intl.formatMessage({ id: 'Toastr.ProjectSettings.success_title' }),
intl.formatMessage({ id: 'Toastr.ProjectSettings.update_metrics_success_message' }),
);
break;
case 'updateDisplayedMetricsError':
toastr.error(
intl.formatMessage({ id: 'Toastr.ProjectSettings.error_title' }),
intl.formatMessage({ id: 'Toastr.ProjectSettings.error_message' }),
);
break;
}

setProjectToastrDisplay('');
}
},
[toastrDisplay, setProjectToastrDisplay, intl],
);

// we set a loader if the project hasn't been loaded from the server or if the page or the script haven't been
// loaded (one of them must be defined when the page is active)
if (project === undefined || (page === undefined && script === undefined)) {
Expand Down Expand Up @@ -244,16 +274,16 @@ export const Audits: React.FunctionComponent<Props> = ({
case auditStatus.requested:
return <FormattedMessage id="Audits.AuditStatusHistory.audit_requested" />;
case auditStatus.queuing:
return auditStatusHistory.info && auditStatusHistory.info.positionInQueue
return auditStatusHistory.info && auditStatusHistory.info.positionInQueue
? <FormattedMessage id="Audits.AuditStatusHistory.audit_in_queue_behind" values={{ positionInQueue: auditStatusHistory.info.positionInQueue }}/>
: <FormattedMessage id="Audits.AuditStatusHistory.audit_in_queue" />
case auditStatus.running:
if(auditStatusHistory.info && auditStatusHistory.info.runningTime) {
return <FormattedMessage id="Audits.AuditStatusHistory.audit_started" values={{ runningTime: auditStatusHistory.info.runningTime }}/>
} else if(auditStatusHistory.info && auditStatusHistory.info.totalTests && auditStatusHistory.info.completedTests) {
return (
<FormattedMessage
id="Audits.AuditStatusHistory.audit_tests_running"
<FormattedMessage
id="Audits.AuditStatusHistory.audit_tests_running"
values={{
completedTests: auditStatusHistory.info.completedTests,
totalTests: auditStatusHistory.info.totalTests,
Expand Down Expand Up @@ -351,6 +381,14 @@ export const Audits: React.FunctionComponent<Props> = ({
blockMargin={`0 0 ${getSpacing(8)} 0`}
auditResultIds={sortedAuditResultsIds}
/>
<ReduxToastr
timeOut={4000}
newestOnTop={false}
preventDuplicates
transitionIn="fadeIn"
transitionOut="fadeOut"
closeOnToastrClick
/>
</Container>
);
};
8 changes: 6 additions & 2 deletions frontend/src/pages/Audits/Audits.wrap.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,9 @@ import {
} from 'redux/auditResults/selectors';
import { getAuditParameters } from 'redux/entities/auditParameters/selectors';
import { getPage, getPageLatestAuditStatusHistory } from 'redux/entities/pages/selectors';
import { fetchProjectsRequest } from 'redux/entities/projects';
import { getProject } from 'redux/entities/projects/selectors';
import { fetchProjectsRequest, setProjectToastrDisplay } from 'redux/entities/projects';
import { getProject, getProjectToastrDisplay } from 'redux/entities/projects/selectors';
import { ProjectToastrDisplayType } from 'redux/entities/projects/types';
import { getScript, getScriptLatestAuditStatusHistory } from 'redux/entities/scripts/selectors';
import {
setCurrentAuditParametersId,
Expand Down Expand Up @@ -44,6 +45,7 @@ const mapStateToProps = (state: RootState, props: OwnProps) => ({
props.match.params.auditParametersId,
props.match.params.pageOrScriptId,
),
toastrDisplay: getProjectToastrDisplay(state),
});

const mapDispatchToProps = (dispatch: Dispatch) => ({
Expand All @@ -62,6 +64,8 @@ const mapDispatchToProps = (dispatch: Dispatch) => ({
dispatch(setCurrentScriptId({ scriptId })),
setCurrentScriptStepId: (scriptStepId: string | null | undefined) =>
dispatch(setCurrentScriptStepId({ scriptStepId })),
setProjectToastrDisplay: (toastrDisplay: ProjectToastrDisplayType) =>
dispatch(setProjectToastrDisplay({ toastrDisplay })),
});

export default connect(
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { MouseEvent, useState } from 'react';
import React, {MouseEvent, useEffect, useState} from 'react';
import { FormattedMessage } from 'react-intl';
import Modal from 'react-modal';

Expand Down Expand Up @@ -26,15 +26,15 @@ interface Props extends OwnProps {
metrics: MetricType[];
show: boolean;
close: () => void;
updateDisplayedMetrics: (projectId: string, selectedMetrics: MetricType[]) => void;
updateDisplayedMetricsRequest: (projectId: string, selectedMetrics: MetricType[]) => void;
}

const MetricModal: React.FunctionComponent<Props> = ({
metrics,
show,
close,
updateDisplayedMetrics,
projectId,
updateDisplayedMetricsRequest,
}) => {
const modalStyles = {
content: {
Expand Down Expand Up @@ -64,6 +64,10 @@ const MetricModal: React.FunctionComponent<Props> = ({

const [selectedMetrics, updateSelectedMetrics] = useState(metrics);

useEffect(() => {
updateSelectedMetrics(metrics);
}, [metrics])

const updateMetrics = (event: MouseEvent, selectedValue: MetricType) => {
if (selectedMetrics.indexOf(selectedValue) === -1) {
updateSelectedMetrics(currentSelectedMetrics => [...currentSelectedMetrics, selectedValue]);
Expand All @@ -83,7 +87,7 @@ const MetricModal: React.FunctionComponent<Props> = ({

const submitDisplayedMetrics = (event: MouseEvent) => {
event.preventDefault();
updateDisplayedMetrics(projectId, selectedMetrics);
updateDisplayedMetricsRequest(projectId, selectedMetrics);
close();
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { connect } from 'react-redux';

import { Dispatch } from 'redux';
import { MetricType } from 'redux/auditResults/types';
import { updateDisplayedMetrics } from 'redux/parameters';
import { updateDisplayedMetricsRequest } from 'redux/entities/projects';
import { getCurrentProjectId } from 'redux/selectors';
import { RootStateWithRouter } from 'redux/types';
import MetricModal from './MetricModal';
Expand All @@ -12,8 +12,8 @@ const mapStateToProps = (state: RootStateWithRouter) => ({
});

const mapDispatchToProps = (dispatch: Dispatch) => ({
updateDisplayedMetrics: (projectId: string, displayedMetrics: MetricType[]) =>
dispatch(updateDisplayedMetrics({ projectId, displayedMetrics })),
updateDisplayedMetricsRequest: (projectId: string, displayedMetrics: MetricType[]) =>
dispatch(updateDisplayedMetricsRequest({projectId, displayedMetrics}))
});

export default connect(
Expand Down
7 changes: 7 additions & 0 deletions frontend/src/redux/entities/projects/actions.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { createStandardAction } from 'typesafe-actions';

import {MetricType} from "redux/auditResults/types";
fargito marked this conversation as resolved.
Show resolved Hide resolved
import { AuditParametersType } from '../auditParameters/types';
import { PageType } from '../pages/types';
import { ApiProjectType, ProjectToastrDisplayType, ProjectType } from './types';
Expand Down Expand Up @@ -137,6 +138,11 @@ export const deleteScriptFromProjectSuccess = createStandardAction('projects/DEL
scriptId: string;
}>();

export const updateDisplayedMetricsRequest = createStandardAction('projects/UPDATE_DISPLAYED_METRICS_REQUEST')<{
projectId: string;
displayedMetrics: MetricType[];
}>();

export default {
addMemberToProjectRequest,
addMemberToProjectError,
Expand Down Expand Up @@ -167,4 +173,5 @@ export default {
deleteAuditParameterFromProjectSuccess,
addScriptToProjectSuccess,
deleteScriptFromProjectSuccess,
updateDisplayedMetricsRequest
};
3 changes: 2 additions & 1 deletion frontend/src/redux/entities/projects/modelizer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ export const modelizeProject = (project: ApiProjectType): Record<string, Project
latestAuditAt: project.latest_audit_at,
projectMembers: project.project_members.map(apiProjectMember => modelizeProjectMember(apiProjectMember)),
wptApiKey: project.wpt_api_key,
wptInstanceURL: project.wpt_instance_url
wptInstanceURL: project.wpt_instance_url,
userMetrics: project.user_metrics,
},
});

Expand Down
Loading