Projet

Général

Profil

Development #39091

nouvelle cellule "Pages récemment modifiées"

Ajouté par Frédéric Péters il y a environ 4 ans. Mis à jour il y a environ 4 ans.

Statut:
Fermé
Priorité:
Normal
Version cible:
-
Début:
19 janvier 2020
Echéance:
17 février 2020
% réalisé:

0%

Temps estimé:
Patch proposed:
Oui
Planning:
Non

Description

Cellule qu'on configurerait avec la hiérarchie de pages à suivre (en gros sélection de "tout le site" ou d'une page précise) + le nombre de pages à reprendre; et le rendu ce serait une liste de lien vers les pages + date/heure (de la dernière modification) +
un petit mot "nouvelle page" pour distinguer les créations de pages des modifications.


Fichiers

0001-kb-add-cell-to-display-last-updated-pages-39091.patch (14,4 ko) 0001-kb-add-cell-to-display-last-updated-pages-39091.patch Nicolas Roche (absent jusqu'au 3 avril), 21 janvier 2020 18:44
0004-kb-add-cell-to-display-last-updated-pages-39091.patch (12,4 ko) 0004-kb-add-cell-to-display-last-updated-pages-39091.patch Nicolas Roche (absent jusqu'au 3 avril), 23 janvier 2020 17:09
0003-data-get-descendants-of-a-page-39091.patch (2,23 ko) 0003-data-get-descendants-of-a-page-39091.patch Nicolas Roche (absent jusqu'au 3 avril), 23 janvier 2020 17:09
0002-data-deprecate-get_last_update_time-using-signals-39.patch (2,29 ko) 0002-data-deprecate-get_last_update_time-using-signals-39.patch Nicolas Roche (absent jusqu'au 3 avril), 23 janvier 2020 17:09
0001-data-add-creation-timestamp-on-Page-object-39091.patch (3,58 ko) 0001-data-add-creation-timestamp-on-Page-object-39091.patch Nicolas Roche (absent jusqu'au 3 avril), 23 janvier 2020 17:09
0001-data-add-creation-timestamp-on-Page-object-39091.patch (3,68 ko) 0001-data-add-creation-timestamp-on-Page-object-39091.patch Nicolas Roche (absent jusqu'au 3 avril), 27 janvier 2020 14:02
0001-data-add-creation-timestamp-on-Page-object-39091.patch (3,44 ko) 0001-data-add-creation-timestamp-on-Page-object-39091.patch Nicolas Roche (absent jusqu'au 3 avril), 27 janvier 2020 17:55
0002-data-deprecate-get_last_update_time-using-signals-39.patch (3,65 ko) 0002-data-deprecate-get_last_update_time-using-signals-39.patch Nicolas Roche (absent jusqu'au 3 avril), 27 janvier 2020 17:55
0003-data-get-descendants-of-a-page-39091.patch (2,23 ko) 0003-data-get-descendants-of-a-page-39091.patch Nicolas Roche (absent jusqu'au 3 avril), 27 janvier 2020 17:55
0004-kb-add-cell-to-display-last-updated-pages-39091.patch (12,8 ko) 0004-kb-add-cell-to-display-last-updated-pages-39091.patch Nicolas Roche (absent jusqu'au 3 avril), 27 janvier 2020 17:55
0004-kb-add-cell-to-display-last-updated-pages-39091.patch (12,8 ko) 0004-kb-add-cell-to-display-last-updated-pages-39091.patch Nicolas Roche (absent jusqu'au 3 avril), 27 janvier 2020 18:20
0004-kb-add-cell-to-display-last-updated-pages-39091.patch (12,8 ko) 0004-kb-add-cell-to-display-last-updated-pages-39091.patch Nicolas Roche (absent jusqu'au 3 avril), 28 janvier 2020 10:13
0004-kb-add-cell-to-display-last-updated-pages-39091.patch (12,7 ko) 0004-kb-add-cell-to-display-last-updated-pages-39091.patch Nicolas Roche (absent jusqu'au 3 avril), 31 janvier 2020 11:41
0004-kb-add-cell-to-display-last-page-updates-39091.patch (12,8 ko) 0004-kb-add-cell-to-display-last-page-updates-39091.patch Nicolas Roche (absent jusqu'au 3 avril), 31 janvier 2020 16:23
0004-kb-add-cell-to-display-last-page-updates-39091.patch (12,8 ko) 0004-kb-add-cell-to-display-last-page-updates-39091.patch Nicolas Roche (absent jusqu'au 3 avril), 31 janvier 2020 16:30
0004-kb-add-cell-to-display-last-page-updates-39091.patch (12,8 ko) 0004-kb-add-cell-to-display-last-page-updates-39091.patch Nicolas Roche (absent jusqu'au 3 avril), 31 janvier 2020 16:49
0003-data-get-descendants-of-a-page-39091.patch (2,23 ko) 0003-data-get-descendants-of-a-page-39091.patch Nicolas Roche (absent jusqu'au 3 avril), 31 janvier 2020 16:49
0002-data-deprecate-get_last_update_time-using-signals-39.patch (3,65 ko) 0002-data-deprecate-get_last_update_time-using-signals-39.patch Nicolas Roche (absent jusqu'au 3 avril), 31 janvier 2020 16:49
0001-data-add-creation-timestamp-on-Page-object-39091.patch (3,44 ko) 0001-data-add-creation-timestamp-on-Page-object-39091.patch Nicolas Roche (absent jusqu'au 3 avril), 31 janvier 2020 16:49

Révisions associées

Révision c84c4751 (diff)
Ajouté par Nicolas Roche (absent jusqu'au 3 avril) il y a environ 4 ans

data: add creation timestamp on Page object (#39091)

Révision 3e5e80f6 (diff)
Ajouté par Nicolas Roche (absent jusqu'au 3 avril) il y a environ 4 ans

data: deprecate get_last_update_time using signals (#39091)

Révision bd558163 (diff)
Ajouté par Nicolas Roche (absent jusqu'au 3 avril) il y a environ 4 ans

data: get descendants of a page (#39091)

Révision e451097b (diff)
Ajouté par Nicolas Roche (absent jusqu'au 3 avril) il y a environ 4 ans

kb: add cell to display last page updates (#39091)

Historique

#2

Mis à jour par Frédéric Péters il y a environ 4 ans

  • Echéance mis à 17 février 2020
  • Assigné à mis à Nicolas Roche (absent jusqu'au 3 avril)
#3

Mis à jour par Frédéric Péters il y a environ 4 ans

Faire ça dans un nouveau module combo.apps.kb, qui pourra plus tard prendre d'autres trucs en rapport base de connaissances.

#4

Mis à jour par Nicolas Roche (absent jusqu'au 3 avril) il y a environ 4 ans

nickel les specs, merci.

#5

Mis à jour par Frédéric Péters il y a environ 4 ans

class LastUpdatedCell(CellBase):

Appelons plutôt ça LastPageUpdatesCell.

     follow_page = models.ForeignKey(... verbose_name=_('Followed parent page')))

J'ai du mal à capter la traduction que tu imagines ici. (je suggérerais de faire comme MenuCell et d'appeler ça root_page).

        def get_descendants(page):
            descendants = [page]
            for item in page.get_children():
                descendants.extend(get_descendants(item))
            return descendants

Ça me semble pouvoir être descendu au niveau de l'objet Page.

        last_update_times = [(x.get_last_update_time(), x) for x in followed_pages]

Directement .ordered_by('-last_update_timestamp') plutôt qu'un tri manuel après coup.

Ailleurs il faudrait noter get_last_update_time comme deprecated et juste retourner .last_update_timestamp, qui va toujours être bon via le cache dans l'attribut related_cells.

            item['is_new'] = not PageSnapshot.objects.filter(page=page)

Ça semble bricolage bizarre mais je me rends compte qu'il n'y a pas de creation_timestamp sur l'objet Page. L'ajouter et simplement utiliser ça pour déterminer si c'est une nouvelle page. (définir nouvelle page comme page créée il y a moins d'une semaine, genre, arbitrairement).

urlpatterns = [
]

Ça, et tout le fichier, sont inutiles.

from bs4 import BeautifulSoup

Ça m'irait de ne pas ajouter cette dépendance, que simplement ça se passe ainsi :

-    html = BeautifulSoup(cell.render(ctx))
-    assert len(html.find_all('li')) == 3
+    assert cell.render(ctx).count('<li') == 3
#6

Mis à jour par Nicolas Roche (absent jusqu'au 3 avril) il y a environ 4 ans

Les 3 premiers patchs modifient l'existant
  • il n'y a pas de creation_timestamp sur l'objet Page. L'ajouter et simplement utiliser ça pour déterminer si c'est une nouvelle page.
  • noter get_last_update_time comme deprecated et juste retourner .last_update_timestamp, qui va toujours être bon via le cache dans l'attribut related_cells.
  • Ça me semble pouvoir être descendu au niveau de l'objet Page.

Puis le dernier correctif prend en compte les autres remarques, propres à ce ticket.

Avec en plus une modification sur la suppression de la root_page d'une cellule "page récemment modifiées" :
plutôt de de supprimer la cellule (on_delete=CASCADE), la cellule pointe à présent sur toutes les pages (on delete=SET_NULL).
(ce comportement me semblait plus approprié)

#7

Mis à jour par Nicolas Roche (absent jusqu'au 3 avril) il y a environ 4 ans

Ci-dessous deux explications supplémentaires.

Dans le premier patch,

creation_timestamp = models.DateTimeField(auto_now_add=True, null=True)

met la date de la migration pour les pages pré-existantes, ce qui peu sembler pas terrible.

Si je met

creation_timestamp = models.DateTimeField(auto_now_add=True)

alors la migration demande une valeur :
You are trying to add the field 'creation_timestamp' with 'auto_now_add=True' to page without a default; the database needs something to populate existing rows.
 1) Provide a one-off default now (will be set on all existing rows)
 2) Quit, and let me add a default in models.py

L'option 1 done au moment du migrate_schema :

django.db.utils.IntegrityError: ERREUR:  la colonne « creation_timestamp » contient des valeurs NULL

L'option 2 ne fonctionne pas non plus :

creation_timestamp = models.DateTimeField(auto_now_add=True, null=True, default=None)

done au moment du makemigrations :
ERRORS:
data.Page.creation_timestamp: (fields.E160) The options auto_now, auto_now_add, and default are mutually exclusive. Only one of these options may be present.

Donc soit je fait une migration de donnée comme ici : https://stackoverflow.com/questions/56992825/how-to-let-existing-entries-empty-when-using-auto-now-add
soit je je laisse en l'état ce premier patch et pendant la première semaines, les pages pré-existantes seront toutes vues comme nouvelles.

Le second patch fait en sorte que chaque modification sur les cellules met à jour le champs last_update_timestamp de la page qui la contient.
Il se base sur le travail fait dans #24239.

#8

Mis à jour par Frédéric Péters il y a environ 4 ans

1) Provide a one-off default now (will be set on all existing rows)

À ça, répondre last_update_timestamp, ne fonctionne pas ?

#9

Mis à jour par Nicolas Roche (absent jusqu'au 3 avril) il y a environ 4 ans

Non, répondre last_update_timestamp, ne fonctionne pas.
Mais, c'est effectivement un bon candidat comme valeur initiale pour les lignes pré-existantes (cf 0001).

Encore autre chose, mais là je tourne en rond :
comme la migration (patch ci-dessus) fait un save() pour renseigner la valeur du nouveau champ, alors toutes les anciennes pages vont être "rafraîchies" et apparaître comme ayant été modifiées dans la cellule "pages récemment modifiées" (ce qui est déjà un peu mieux, malgré tout).

#10

Mis à jour par Frédéric Péters il y a environ 4 ans

Je trouve curieux que ça puisse fonctionner, parce que ça va laisser des lignes sans valeur entre le migrations.AddField et le migrations.RunPython, non ? Tu as joué ça avec postgresql ? Mais sinon on s'en fout on met timezone.now et hop, plutôt que s'ajouter des nouvelles questions.

~~

     def get_last_update_time(self):
+        '''deprecated:
+        page.last_update_timestamp is now updated by
+        cell_maintain_page_cell_cache signal, triggered when saving cells.
+        '''
         cells = CellBase.get_cells(page_id=self.id)
         return max([self.last_update_timestamp] + [x.last_update_timestamp for x in cells])

Ça sert à rien de laisser tous les calculs alors qu'on met un commentaire comme quoi page.last_update_timestamp suffit. Virer le commentaire/docstring et retourner page.last_update_timestamp.

~~

followed_pages

Je ne comprends toujours pas ce qui peut être compris par "followed"; tape juste pages comme nom de variable.

~~

+        {{ page.title }}
+        <em>
+          &nbsp;
+          {% trans "on"%} {{ page.last_update_timestamp }}
+          {% if page.is_new %}
+          {% trans "(new page)"%}
+          {% endif %}
+        </em>

il n'est sans doute pas nécessaire de taper d'espace insécable.

Taper aussi bien le titre que la date que le (new page) dans des <span> indépendants, avec une classe avec un nom approprité, pas taper de <em>.

#11

Mis à jour par Nicolas Roche (absent jusqu'au 3 avril) il y a environ 4 ans

Tu as joué ça avec postgresql ?

oui

on s'en fout on met timezone.now

fait dans 0001

Virer le commentaire/docstring et retourner page.last_update_timestamp.

fait dans 0002 (j'ai du retirer l'ancien test qui ne fonctionnait plus)

J'ai traité les remarques suivantes dans 0004 (merci ThomasJ).

Remarque :
Les espaces entre le texte des balises <span> sont présents sur le portail agent (display: block),
mais pas sur le portail utilisateur, avec les thèmes de base (display: flex).

#12

Mis à jour par Frédéric Péters il y a environ 4 ans

lastpagesupdatescellcestvraimenttropcompliquéàlire, je trouve.

Tape plutôt le contenu contre les balises, pour éviter des espaces qu'on ne veut pas nécessairement.

        <span class="lastpagesupdatescell--item-isnew">{% trans "(new page)" %}</span>
#13

Mis à jour par Nicolas Roche (absent jusqu'au 3 avril) il y a environ 4 ans

        <time
          class="lastpagesupdatescell--item-date" 
          datetime={{ page.last_update_timestamp|datetime|date:"c"}}
          >{% trans "on"%} {{ page.last_update_timestamp }}</time>
        {% if page.is_new %}
        <span class="lastpagesupdatescell--item-isnew">{% trans "(new page)"%}</span>

Sinon ThomasJ devrais aussi bientôt passer par ici...
Il en veut aussi à la longueur du nom de ma classe CSS je crois : 'lastpagesupdatescell'.
#14

Mis à jour par Frédéric Péters il y a environ 4 ans

Et ce bout vient de moi, désolé c'est seulement mainenant que je vois le truc mauvais,

Appelons plutôt ça LastPageUpdatesCell.

C'est plutôt Latest que Last qu'il faut; LatestPageUpdatedCell, désolé.

lastpagesupdatescell

Et le truc facile, comme c'est fait pour les noms de fichiers etc., c'est les tirets.

Le gabarit s'appelle last-pages-updates-cell.html (et mettons sera latest-page-updates-cell.html), la version "classe CSS" da la même cellule devrait être la même chose.

(alors oui, il y a une classe css automatiquement posée à partir du nom de la classe et c'est sans tiret, mais c'est l'occasion de se faire la remarque et de décider de changer ça, j'en fais un ticket dédié, #39314).

#15

Mis à jour par Thomas Jund il y a environ 4 ans

Je me demandais même si latest n'est pas implicite et pourrais être supprimé.
Que `updated-pages-cell` pourrait suffire ?

#16

Mis à jour par Nicolas Roche (absent jusqu'au 3 avril) il y a environ 4 ans

Après rebasage sur #39314

`updated-pages-cell` pourrait suffire ?

Dites-moi, je peux aussi couper la poire en deux et surcharger class_name()
pour avoir un nom de classe CSS plus court que celui de la classe Python,
mais après le travail fait dans #39314 je trouve ça dommage.

#17

Mis à jour par Frédéric Péters il y a environ 4 ans

Dites-moi, je peux aussi couper la poire en deux et surcharger class_name()

Non, il est important de garder une uniformité.

Si on trouve "Latest" redondant/inutile, il l'est alors également dans le nom de la classe Python.

#18

Mis à jour par Nicolas Roche (absent jusqu'au 3 avril) il y a environ 4 ans

Ce dernier patch économise 150 octets et contribue donc à sauver des ours du réchauffement climatique.

#19

Mis à jour par Frédéric Péters il y a environ 4 ans

  • Statut changé de Solution proposée à Solution validée

Si on trouve "Latest" redondant/inutile (...)

Et dans le titre de la cellule etc. i.e. si on ne veut pas latest, qu'il n'apparaisse pas du tout dans le patch. (perso je trouve plus clair d'avoir le latest partout mais tu fais comme tu veux).

À part ça, ok mais datetime={{ page.last_update_timestamp|datetime|date:"c"}} et {% trans "on"%} , ajoute un espace avant la fermeture du }} ou %}. Aussi, pour l'attribut datetime, ajoute des guillemets autour.

#20

Mis à jour par Nicolas Roche (absent jusqu'au 3 avril) il y a environ 4 ans

perso je trouve plus clair d'avoir le latest

moi aussi, mais dans le précédent patch j'avais déjà une incohérence avec le titre de la cellule.

Et du coup, pour plus de cohérence,
plutôt que de modifier le titre en "Latest Page Updated",
est-ce que s/LatestPageUpdatedCell/LatestUpdatedPagesCell/ conviendrait ?

#21

Mis à jour par Frédéric Péters il y a environ 4 ans

Je serais pour LatestPageUpdate*S*Cell; et Latest Page Updates comme titre, etc.

#22

Mis à jour par Nicolas Roche (absent jusqu'au 3 avril) il y a environ 4 ans

ok, à présent ça me semble calé.

#23

Mis à jour par Nicolas Roche (absent jusqu'au 3 avril) il y a environ 4 ans

oups, j'avai encore oublié un espace dans le template :

<<<
{% trans "(new page)"%}
---
{% trans "(new page)" %}
>>>

#25

Mis à jour par Nicolas Roche (absent jusqu'au 3 avril) il y a environ 4 ans

  • Statut changé de Solution proposée à Résolu (à déployer)

(Thomas m'a suggéré que c'était déjà validé en l'état)

commit e451097beff65a41d4a39fada9efd1e84fce9e35
Author: Nicolas ROCHE <nroche@entrouvert.com>
Date:   Fri Jan 31 16:45:27 2020 +0100

    kb: add cell to display last page updates (#39091)

commit bd558163a85526f34f2a79ecd4d35b2669869ad6
Author: Nicolas ROCHE <nroche@entrouvert.com>
Date:   Thu Jan 23 15:30:57 2020 +0100

    data: get descendants of a page (#39091)

commit 3e5e80f6e6daec96914f75345639a5b3b284b149
Author: Nicolas ROCHE <nroche@entrouvert.com>
Date:   Thu Jan 23 16:25:32 2020 +0100

    data: deprecate get_last_update_time using signals (#39091)

commit c84c4751e2933de28dc2718f1469330fcba82dc1
Author: Nicolas ROCHE <nroche@entrouvert.com>
Date:   Thu Jan 23 16:24:45 2020 +0100

    data: add creation timestamp on Page object (#39091)

#26

Mis à jour par Frédéric Péters il y a environ 4 ans

  • Statut changé de Résolu (à déployer) à Solution déployée

Formats disponibles : Atom PDF