Refactor page tree and add page based statistics #3325

JoeyStk · 2025-01-11T17:32:40Z

Short description

This PR refactors parts of the page tree and adds the page based statistics to statistics.

Proposed changes

Refactor page tree
- Split page tree into pages_page_tree and statistics_page_tree
- Adjust naming
- Merge page_tree_archived into pages_page_tree
Add page based statistics
- Call to Matomo
- Show page based statistics in statistics view
Update Lucide

Side effects

In collaboration with @jarlhengstmengel
Refactoring page tree is always dangerous, thorough testing is very, very welcome
I would propose that this PR gets at least three reviews
Please test this with a database dump

Resolved issues

Fixes: #3130

Pull Request Review Guidelines

…ags for archive nodes

…sible_box' of https://github.com/digitalfabrik/integreat-cms into feature/refactor_page_tree_and_add_statistics_to_collapsible_box

…ags for archive nodes

JoeyStk · 2025-01-19T11:18:56Z

Current state of this PR: @jarlhengstmengel is still working on fixing the tests. As soon as this is done, this PR is ready to be reviewed :)

jarlhengstmengel · 2025-01-20T13:53:27Z

The existing tests do pass now so I marked this PR as ready to be reviewed!

MizukiTemma · 2025-01-20T15:27:26Z

@jarlhengstmengel @JoeyStk
Could you squash the commits?

JoeyStk · 2025-01-20T15:33:56Z

We decided to keep the commits until we merge so we can go back to previous states in case something doesn't work as expected

integreat_cms/static/src/js/analytics/statistics-page-accesses.ts

charludo

Thank you! This is a really nice refactor! I do have a couple of small comments (see below), as well as some comments about the statistics.

However, I am not sure if this PR is just meant to add a draft of the statistics, or if it should be the "final" statistics implementation, and depending on which it is, some of my points might not be applicable.

the sidebar-boxes should not resize when a parent page is expanded. Esp. when a lot of pages are epanded, blank space is added at weird-looking places in the sidebar boxes. IMO they should always just be their initial size and stay at the top of the page.
the last column of the statistics page tree ("accesses" with the date range) has a left-aligned header, but right-aligned content. Just looks a bit weird.
you are currently blocking the reordering of pages in the statistics view. Is there a reason to still show the icon used for dragging and dropping? IMO it should not be shown at all in the statistics view.
what happened to the bar charts intended for the statistics page tree? 😁

I have tested all the views where the new and old page trees were used to the best of my abilities, and have found 0 issues with how they look or behave. Good job!! 😁 🎉

charludo · 2025-01-27T13:47:50Z

integreat_cms/cms/templates/pages/_generic_page_tree.html

Maybe generic_page_tree_header.html? From the name I initially thought this was the main "entry point" for the tree

charludo · 2025-01-27T13:56:48Z

integreat_cms/cms/templates/pages/_page_tree_children.html

+            {% if is_statistics %}
+                {% include "statistics/statistics_viewed_pages_node.html" %}
+            {% else %}
+                {% include "pages/pages_page_tree_node.html" %}
+            {% endif %}


Nit: if we decide to use the page tree for even more things / in more variants, this becomes cumbersome. Maybe we could pass something like row_template from the view?

(...but since I do not know if we will be extending the usecases of this, it really is a nit. Feel free to ignore without comment 😄)

integreat_cms/cms/views/pages/page_tree_view.py

integreat_cms/cms/views/statistics/statistics_actions.py

charludo · 2025-01-27T14:05:27Z

integreat_cms/cms/views/statistics/statistics_actions.py

In addition to the error-responses, could you also add error logs here?

integreat_cms/locale/de/LC_MESSAGES/django.po

charludo · 2025-01-27T14:21:58Z

integreat_cms/matomo_api/utils.py

+    translation_slugs: dict = {}
+    for page in pages:
+        for language in languages:
+            if page_translation := page.get_translation(language.slug):
+                if page.id not in translation_slugs:
+                    translation_slugs[page.id] = {}
+                translation_slugs[page.id][
+                    language.slug
+                ] = page_translation.get_absolute_url().rstrip("/")


I'm wondering if this could be faster if formulated as a database query. This is only used in get_page_accesses_async, which is also only called once in get_page_accesses, where region.get_pages() is passed to this function. get_pages() returns a queryset, not a list[Page], so you should be able to work directly on that?

But maybe you have a reason to not do that, I only took a cursory look at this part 😅

integreat_cms/matomo_api/utils.py

charludo · 2025-01-27T14:28:06Z

integreat_cms/static/src/js/analytics/statistics-charts.ts

@@ -29,6 +29,7 @@ let exportLabels: Array<string>;
 const updateChart = async (): Promise<void> => {
    // Get Chart instance
    const chart = Chart.instances[0];
+    (window as any).chart = chart;


Is this actually used anywhere? 😅

I think statistics-page-accesses.ts makes use of the chart to track for changes in filtering

NVM it's never used

MizukiTemma · 2025-01-27T15:13:31Z

integreat_cms/static/src/js/analytics/statistics-page-accesses.ts

+};
+
+window.addEventListener("load", async () => {
+    updatePageAccesses();


Suggested change

updatePageAccesses();

updatePageAccesses();

Not really a but but...this is causing TypeError: Cannot read properties of null (reading 'classList') everywhere except on the statistics page 😢

MizukiTemma · 2025-01-27T15:14:12Z

integreat_cms/static/src/js/analytics/statistics-page-accesses.ts

+    updatePageAccesses();
+    chart = Chart.instances[0];
+    // Refresh the statistics every time a new timeframe or language is selected
+    const oldUpdate = chart.update;


This chart too. It does not exists in many pages of our system 😢

MizukiTemma · 2025-01-27T15:20:08Z

I tried this branch with the dump date from 20 Janurary, and it seems loading time needs improvement, as the chart bars never loaded (or let's hear from other reviewers testing with a dump date, it's can be only in my environment that it's so slow...).

Trying with less pages in a region (by deleting or archiving most of pages), it seems to be working nice 😸

JoeyStk · 2025-01-28T10:20:11Z

Thank you for your reviews!

what happened to the bar charts intended for the statistics page tree? 😁

They should show up :D
Like this:

The statistics can only be withdrawn from Matomo if the slug is exactly identical. Based on our test data I'd expect Welcome and German Language to work. The exact slug that should work are: https://integreat.app/augsburg/de/willkommen and https://integreat.app/augsburg/de/deutsche-sprache 🤔 Could you please check again, if it works with these slugs? 🤔 That would help a lot :)

With this in mind

the last column of the statistics page tree ("accesses" with the date range) has a left-aligned header, but right-aligned content. Just looks a bit weird.

should also be partly resolved, right? 🤔

Co-authored-by: Charlotte <[email protected]>

JoeyStk · 2025-01-28T11:37:19Z

you are currently blocking the reordering of pages in the statistics view. Is there a reason to still show the icon used for dragging and dropping? IMO it should not be shown at all in the statistics view.

That's a really good point. We spoke about this quite a bit. We also wanted to remove the icon, but then a. it looked a bit strange and the hierarchy of the child pages wasn't as clear anymore and b. we discovered that we also use the icon for archived pages, although it's also not possible to move pages there. Therefore we decided to go for what already exists in the system. But yes, maybe it would be an idea to give this to UI/UX and change it later for page statistics and for archived pages

charludo · 2025-02-05T08:28:59Z

Thank you for your reviews!

what happened to the bar charts intended for the statistics page tree? 😁

They should show up :D Like this:

The statistics can only be withdrawn from Matomo if the slug is exactly identical. Based on our test data I'd expect Welcome and German Language to work. The exact slug that should work are: https://integreat.app/augsburg/de/willkommen and https://integreat.app/augsburg/de/deutsche-sprache 🤔 Could you please check again, if it works with these slugs? 🤔 That would help a lot :)

With this in mind

the last column of the statistics page tree ("accesses" with the date range) has a left-aligned header, but right-aligned content. Just looks a bit weird.

should also be partly resolved, right? 🤔

Ohhhhhhhh

I just used the Matomo Testumgebung token, with 0 views across the board, so this is entirely my fault 🤦🏼‍♀️ 😂
Thank you, those look great! 🔥 🎉

And yes, the alignment now makes sense. Sorry about this.

charludo · 2025-02-05T08:30:48Z

you are currently blocking the reordering of pages in the statistics view. Is there a reason to still show the icon used for dragging and dropping? IMO it should not be shown at all in the statistics view.

That's a really good point. We spoke about this quite a bit. We also wanted to remove the icon, but then a. it looked a bit strange and the hierarchy of the child pages wasn't as clear anymore and b. we discovered that we also use the icon for archived pages, although it's also not possible to move pages there. Therefore we decided to go for what already exists in the system. But yes, maybe it would be an idea to give this to UI/UX and change it later for page statistics and for archived pages

OK, makes sense. Then I'd say leave it as is for now, but could you open an issue for changing the icon/behavior in both instances?

jarlhengstmengel and others added 30 commits November 27, 2024 18:23

Merge page_tree_archived into page_tree

39de968

Fix translation format

63219f4

Add template for statistics about viewed pages

8eb7279

Add _generic_page_tree.html

94fcc30

Add additional column to page tree on statistics

3c12cc7

Fix missing backend titles for root pages in page tree and deleting t…

b1545cc

…ags for archive nodes

Merging page_tree_node and page_tree_archived_node

9edae89

Disable dragable tree node for archived tree nodes

e3c0d63

[WIP] Page based access

8466b66

[WIP] Restored commit

813b751

Fix loading of childnodes

04ce650

Fix codestyle

aed75ed

Add _generic_page_tree_node.hmtl

05c2ad1

update translations

7128904

Cleaning up and deleting page_tree_archived files

718358e

Rename page_tree to pages_page_tree

4fd9720

Add page accesses calls

feb7a64

Merge branch 'feature/refactor_page_tree_and_add_statistics_to_collap…

a41035d

…sible_box' of https://github.com/digitalfabrik/integreat-cms into feature/refactor_page_tree_and_add_statistics_to_collapsible_box

Fix pylint

bbfb58c

[WIP] Include page tree to statistics

c375fde

Javascript code

68ccfe1

[WIP] First idea for graph

a9fa6ae

Show bars

edc3114

Merge page_tree_archived into page_tree

0308511

Fix translation format

562f952

Add template for statistics about viewed pages

836506b

Add _generic_page_tree.html

bc93bc6

Add additional column to page tree on statistics

ce301cc

Fix missing backend titles for root pages in page tree and deleting t…

4f4c99d

…ags for archive nodes

Merging page_tree_node and page_tree_archived_node

4f91410

JoeyStk marked this pull request as draft January 11, 2025 17:40

Update lucide version

120f93c

JoeyStk marked this pull request as ready for review January 13, 2025 14:50

JoeyStk marked this pull request as draft January 13, 2025 15:02

jarlhengstmengel added 2 commits January 20, 2025 13:32

Fix existing tests

1864936

Fix format

1223224

jarlhengstmengel marked this pull request as ready for review January 20, 2025 13:51

JoeyStk assigned PeterNerlich and MizukiTemma Jan 22, 2025

charludo self-assigned this Jan 22, 2025

jarlhengstmengel reviewed Jan 24, 2025

View reviewed changes

integreat_cms/static/src/js/analytics/statistics-page-accesses.ts Show resolved Hide resolved

JoeyStk added 2 commits January 24, 2025 17:32

Add percentage to accesses

9e6374b

Add comments to statistics-page-accesses

70f7f62

charludo requested changes Jan 27, 2025

View reviewed changes

MizukiTemma reviewed Jan 27, 2025

View reviewed changes

Apply suggestions from code review

d7842be

Co-authored-by: Charlotte <[email protected]>

Update statistics_actions and utils

7d82374

JoeyStk force-pushed the feature/refactor_page_tree_and_add_statistics_to_collapsible_box branch from 2957a54 to 7d82374 Compare January 28, 2025 11:56

jarlhengstmengel mentioned this pull request Feb 4, 2025

Export page access statistics #3394

Open

MizukiTemma added the blocker This issue blocks another issue label Feb 10, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor page tree and add page based statistics #3325

Refactor page tree and add page based statistics #3325

JoeyStk commented Jan 11, 2025 •

edited by jarlhengstmengel

Loading

JoeyStk commented Jan 19, 2025

jarlhengstmengel commented Jan 20, 2025

MizukiTemma commented Jan 20, 2025

JoeyStk commented Jan 20, 2025

charludo left a comment

charludo Jan 27, 2025

charludo Jan 27, 2025

charludo Jan 27, 2025

charludo Jan 27, 2025

charludo Jan 27, 2025

jarlhengstmengel Feb 22, 2025

jarlhengstmengel Feb 22, 2025

MizukiTemma Jan 27, 2025 •

edited

Loading

MizukiTemma Jan 27, 2025 •

edited

Loading

MizukiTemma commented Jan 27, 2025

JoeyStk commented Jan 28, 2025

JoeyStk commented Jan 28, 2025

charludo commented Feb 5, 2025

charludo commented Feb 5, 2025

Refactor page tree and add page based statistics #3325

Are you sure you want to change the base?

Refactor page tree and add page based statistics #3325

Conversation

JoeyStk commented Jan 11, 2025 • edited by jarlhengstmengel Loading

Short description

Proposed changes

Side effects

Resolved issues

JoeyStk commented Jan 19, 2025

jarlhengstmengel commented Jan 20, 2025

MizukiTemma commented Jan 20, 2025

JoeyStk commented Jan 20, 2025

charludo left a comment

Choose a reason for hiding this comment

charludo Jan 27, 2025

Choose a reason for hiding this comment

charludo Jan 27, 2025

Choose a reason for hiding this comment

charludo Jan 27, 2025

Choose a reason for hiding this comment

charludo Jan 27, 2025

Choose a reason for hiding this comment

charludo Jan 27, 2025

Choose a reason for hiding this comment

jarlhengstmengel Feb 22, 2025

Choose a reason for hiding this comment

jarlhengstmengel Feb 22, 2025

Choose a reason for hiding this comment

MizukiTemma Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

MizukiTemma Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

MizukiTemma commented Jan 27, 2025

JoeyStk commented Jan 28, 2025

JoeyStk commented Jan 28, 2025

charludo commented Feb 5, 2025

charludo commented Feb 5, 2025

JoeyStk commented Jan 11, 2025 •

edited by jarlhengstmengel

Loading

MizukiTemma Jan 27, 2025 •

edited

Loading

MizukiTemma Jan 27, 2025 •

edited

Loading