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

♻️ [#1930] Category personalisation fixes #907

Merged
merged 7 commits into from
Dec 27, 2023

Conversation

stevenbal
Copy link
Contributor

@stevenbal stevenbal commented Dec 18, 2023

@stevenbal stevenbal marked this pull request as draft December 18, 2023 11:13
@stevenbal stevenbal changed the title 🔥 [#1930] Remove visible_for_authenticated flag from Category ♻️ [#1930] Category personalisation fixes Dec 18, 2023
@stevenbal stevenbal force-pushed the issue/1930-personalized-categories branch 2 times, most recently from ad36116 to b0f1194 Compare December 18, 2023 13:07
@codecov-commenter
Copy link

codecov-commenter commented Dec 18, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9cac5a4) 92.93% compared to head (780d86b) 92.96%.
Report is 3 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #907      +/-   ##
===========================================
+ Coverage    92.93%   92.96%   +0.02%     
===========================================
  Files          819      820       +1     
  Lines        28250    28358     +108     
===========================================
+ Hits         26255    26363     +108     
  Misses        1995     1995              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@stevenbal stevenbal force-pushed the issue/1930-personalized-categories branch 2 times, most recently from fba0b28 to b03d51d Compare December 19, 2023 12:52
@stevenbal
Copy link
Contributor Author

@alextreme I noticed the following:

  • To determine the categories shown in the menu, I only use the root nodes and then do the permission filter for the user
  • To determine the categories shown in the homepage plugin, I take all nodes (including child nodes), then do permission filtering and then uitgelicht/zaaktypen filtering. This way subcategories can show up in the plugin on the homepage if uitgelicht, but not in the menu, is this what we want? Or do we want to show child nodes in the menu / list directly as well?

@stevenbal stevenbal marked this pull request as ready for review December 19, 2023 13:25
@stevenbal stevenbal requested a review from alextreme December 19, 2023 13:25
@stevenbal stevenbal force-pushed the issue/1930-personalized-categories branch from 7522d12 to e5c4a1e Compare December 19, 2023 13:30
task: https://taiga.maykinmedia.nl/project/open-inwoner/task/1930

this flag is redundant since the addition of visible for DigiD/eHerkenning
task: https://taiga.maykinmedia.nl/project/open-inwoner/task/1930

* use the `hide_categories_from_anonymous_users` feature flag to determine visibility
* show categories on homepage that are `highlighted` or found through zaaktypen filtering
* remove limit of 4 for homepage categories plugin
* remove fallbacks if no categories are found
task: https://taiga.maykinmedia.nl/project/open-inwoner/task/1930

the menu and the category list should only show root nodes, and the CMS plugin should show subcategories if highlighted/linked to relevant zaken
@stevenbal stevenbal force-pushed the issue/1930-personalized-categories branch from e5c4a1e to 4d623be Compare December 19, 2023 13:35
@alextreme
Copy link
Member

@alextreme I noticed the following:

* To determine the categories shown in the menu, I only use the root nodes and then do the permission filter for the user

* To determine the categories shown in the homepage plugin, I take all nodes (including child nodes), then do permission filtering and then `uitgelicht`/zaaktypen filtering. This way subcategories can show up in the plugin on the homepage if uitgelicht, but not in the menu, is this what we want? Or do we want to show child nodes in the menu / list directly as well?

We don't want to show child nodes in the menu, the idea is that from the category detailpage you can click through and browse the subcategories (if there are any).

I would expect that we also need to update CategoryDetailView in order to make use of the visible_for_user() manager for the subcategories. I don't see that in your PR or on develop, so double-check this just in case.

Copy link
Member

@alextreme alextreme left a comment

Choose a reason for hiding this comment

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

Double-check CategoryDetailView

Copy link
Contributor

@Bartvaderkin Bartvaderkin left a comment

Choose a reason for hiding this comment

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

Nice, and more test code then functional code 😆

@stevenbal stevenbal requested a review from alextreme December 21, 2023 09:09
@stevenbal stevenbal force-pushed the issue/1930-personalized-categories branch from 071b124 to 6e5ce50 Compare December 21, 2023 09:28
@stevenbal stevenbal force-pushed the issue/1930-personalized-categories branch from 6e5ce50 to 780d86b Compare December 21, 2023 09:38
@alextreme alextreme merged commit aeff3b3 into develop Dec 27, 2023
@alextreme alextreme deleted the issue/1930-personalized-categories branch December 27, 2023 15:53
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.

4 participants