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

Counter activity stats #895

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

Counter activity stats #895

wants to merge 11 commits into from

Conversation

NaNoMelo
Copy link
Contributor

Implémentation d'un graph des perms de la semaine passée sur la page activité des comptoirs

@@ -22,6 +26,9 @@
{% trans %}There is currently no barman connected.{% endtrans %}
{% endif %}
</ul>
<h4>{% trans %}Last Week Activity {% endtrans %}</h4>
<div id="activityGraph" width="400" height="200"></div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Je suis pas sûr que ça soit du html valide, les propriétés width et height sur les divs. Et tu mets la height à 200, alors que dans le JS elle est à 600.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes en effet, j'ai juste l'impression que le js override tout ça, mais dans le doute je l'ai enlevé

@NaNoMelo NaNoMelo force-pushed the counter-activity-stats branch from 98b4616 to 21b4a5d Compare October 22, 2024 11:20
@NaNoMelo NaNoMelo requested review from klmp200 and imperosol October 22, 2024 11:22
@NaNoMelo NaNoMelo marked this pull request as ready for review October 22, 2024 11:39
Copy link
Contributor

@klmp200 klmp200 left a comment

Choose a reason for hiding this comment

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

Ce serait bien d'avoir des images de ce à quoi ça ressemble :)

Copy link
Contributor

Choose a reason for hiding this comment

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

counter/static/webpack/counter/permanencies/time-grid-index.ts

counter/static/webpack/graph-index.ts Outdated Show resolved Hide resolved
permanencyFetchPermanancies,
} from "#openapi";

interface ActivityChartConfig {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
interface ActivityChartConfig {
interface ActivityTimeGridConfig {

interface EventInput {
start: Date;
end: Date;
backgroundColor: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ça devrais être contrôllé par le CSS, je trouve pas ça super que ce soit le code de logique qui choisisse le style

Copy link
Contributor Author

Choose a reason for hiding this comment

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

la couleur de background est prise en paramètre des events directement, et dans ce cas de figure je les change de couleur selon si ils sont dans la semaine précédente ou actuelle

Copy link
Contributor

Choose a reason for hiding this comment

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

Est-ce que la lib permet d'injecter une classe CSS, plutôt que directement le background-color ? Ca permettrait de gérer procéduralement la couleur, sans pour autant que ça soit trop hardcodé.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

j'en ai absolument aucune idée, je saurais même pas trop comment faire ça

counter/static/counter/css/activity.scss Outdated Show resolved Hide resolved
counter/static/counter/css/activity.scss Outdated Show resolved Hide resolved
counter/static/webpack/graph-index.ts Outdated Show resolved Hide resolved
counter/api.py Outdated Show resolved Hide resolved
counter/static/counter/css/activity.scss Outdated Show resolved Hide resolved
interface EventInput {
start: Date;
end: Date;
backgroundColor: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Est-ce que la lib permet d'injecter une classe CSS, plutôt que directement le background-color ? Ca permettrait de gérer procéduralement la couleur, sans pour autant que ça soit trop hardcodé.

counter/static/webpack/graph-index.ts Outdated Show resolved Hide resolved
counter/static/webpack/graph-index.ts Outdated Show resolved Hide resolved
}

// Function to get last Monday at 00:00
function getLastMonday(now = new Date()): Date {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ca pose pas de problème en JS, de mettre un appel de fonction/constructeur en paramètre ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

non pas du tout, ça fait juste que le paramètre est optionnel, et prends une valeur par défaut a un nouvel objet, en l'occurrence la date du moment présent
c'est aussi faisable en C++ ce genre de trucs

Copy link
Contributor

Choose a reason for hiding this comment

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

Faut se méfier du "ça se fait aussi dans tel langage", parce que justement en Python ça se fait pas. On a même eu des problèmes dans le passé avec ça (cf #656) et Ruff a un lint pour ce cas très précis (https://docs.astral.sh/ruff/rules/function-call-in-default-argument/).

@NaNoMelo
Copy link
Contributor Author

J'ai refactor un peu tout le code :

  • les permanancies sont maintenant des permanencies
  • les chart et autres graphs sont maintenant des TimeGrid
  • le chemin du script a été modifié pour éviter le désordre dans les imports de webpack
  • la requête à l'orm a été modifiée pour éviter le N+1

Quelques petites images pour illustrer quand même un peu la chose :

Le planning tel qu'il sera visible directement sur la page activity :

image

Plannings complets (pas ceux visible en prod, mais au moins y'a tout sur un screen)

Semaine en cours :
image

Semaine passée :
image

Il me reste potentiellement encore à voir la question du css par classes pour les évènements, je regarderais peut-être ça demain, sinon je laisserais ça comme ça et ça marche bien aussi !

@NaNoMelo NaNoMelo requested review from klmp200 and imperosol October 22, 2024 21:25
Comment on lines +54 to +97
def fetch_permanencies(self, filters: Query[PermanencyFilterSchema]):
return (
filters.filter(Permanency.objects.all())
.distinct()
.order_by("-start")
.select_related("counter")
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Petits tests ? 👉 👈

@imperosol imperosol force-pushed the counter-activity-stats branch 2 times, most recently from 5601339 to dae5cb0 Compare November 27, 2024 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants