+
Skip to content

IBX-9060: Added $query parameter for filtering notifications count and loading #520

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 16 commits into from
Jun 24, 2025
Merged

IBX-9060: Added $query parameter for filtering notifications count and loading #520

merged 16 commits into from
Jun 24, 2025

Conversation

tbialcz
Copy link
Contributor

@tbialcz tbialcz commented Apr 3, 2025

🎫 Issue IBX-9060

Description:

  • Introduced an optional $query parameter to getNotificationCount, loadNotifications, and related methods to enable filtering by notification type or data.
  • Adjusted query logic to apply filtering when $query is provided.
  • Updated related tests to cover the new filtering behavior.

@tbialcz tbialcz force-pushed the ibx_9060-notification-filtering branch 2 times, most recently from 5a21d61 to 3b8e815 Compare April 10, 2025 06:35
@tbialcz tbialcz force-pushed the ibx_9060-notification-filtering branch from 3b8e815 to 8bd2a39 Compare April 10, 2025 07:03
@adamwojs
Copy link
Member

In order to be consistent and avoid leaking implementation details, we need to approach problem differently.

Introduce Ibexa\Contracts\Core\Repository\NotificationService::findNotifications method:

interface NotificationService
{
    # ...
    public function findNotifications(?NotificationQuery $query = null): NotificationList;
}

Ibexa\Contracts\Core\Repository\Values\Notification\NotificationQuery which will hold pagination details, criteria and potentially in the future sort clauses or aggregations.

You will also need to introduce criteria classes for type, status, creation date alongside with logical operator: and, or, not.

Feel free to reach out to me if you have any questions 😉

@tbialcz tbialcz force-pushed the ibx_9060-notification-filtering branch 4 times, most recently from 6961c45 to d3df305 Compare May 8, 2025 09:16
@tbialcz tbialcz force-pushed the ibx_9060-notification-filtering branch from d3df305 to b5a96d8 Compare May 8, 2025 09:37
@tbialcz tbialcz requested a review from adamwojs May 20, 2025 11:53
@tbialcz
IBX-9060: Refactored notification loading logic by splitting `loadNot…
852c013 
…ifications` and `findNotifications`, updated related tests and handlers
@tbialcz tbialcz mentioned this pull request Jun 10, 2025
Merged
alongosz
alongosz requested changes Jun 10, 2025
View reviewed changes
Copy link
Member

@alongosz alongosz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

API-wise it looks okay, however implementation of criteria dispatching needs some improvements (see the diff comment) :-)

@tbialcz tbialcz requested a review from alongosz June 10, 2025 11:52
@tbialcz
IBX-9060: Refactored notification filtering with criterion handlers, …
626debe 
…updated NotificationQuery, Gateway, tests and services
@tbialcz tbialcz force-pushed the ibx_9060-notification-filtering branch from 156ffd1 to 626debe Compare June 10, 2025 12:02
tbialcz added 3 commits June 10, 2025 14:49
@tbialcz
IBX-9060: Replaced Criterion abstract class with `CriterionInterfac…
70061b3 
…e`, updated handlers, NotificationQuery, and Gateway to align with new interface structure
@tbialcz
IBX-9060: Replaced Criterion abstract class with `CriterionInterfac…
f722a69 
…e`, updated handlers, NotificationQuery, and Gateway to align with new interface structure
@tbialcz
Merge remote-tracking branch 'origin/ibx_9060-notification-filtering'…
9a12a9d 
… into ibx_9060-notification-filtering
adamwojs
adamwojs reviewed Jun 19, 2025
View reviewed changes
@tbialcz tbialcz requested a review from adamwojs June 20, 2025 06:18
@tbialcz
IBX-9060: Refactored NotificationQuery namespace, improved encapsul…
2b79cf5 
…ation and added getters/setters for criteria properties
barw4
barw4 reviewed Jun 20, 2025
View reviewed changes
Steveb-p
Steveb-p requested changes Jun 20, 2025
View reviewed changes
@tbialcz tbialcz requested a review from Steveb-p June 23, 2025 06:20
@tbialcz
IBX-9060: Added ibexa/polyfill-php82 dependency and utilized `itera…
8b26279 
…tor_to_array` in DoctrineDatabase Gateway
@tbialcz tbialcz force-pushed the ibx_9060-notification-filtering branch from bcdf9e9 to 8b26279 Compare June 23, 2025 08:30
Steveb-p
Steveb-p approved these changes Jun 23, 2025
View reviewed changes
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@adamwojs adamwojs merged commit 07187f8 into 4.6 Jun 24, 2025
24 checks passed
@adamwojs adamwojs deleted the ibx_9060-notification-filtering branch June 24, 2025 06:46
alongosz
alongosz reviewed Jun 24, 2025
View reviewed changes
phpstan-baseline.neon
-
message: '#^Method Ibexa\\Core\\Persistence\\Legacy\\Notification\\Gateway\:\:loadUserNotifications\(\) return type has no value type specified in iterable type array\.$#'
identifier: missingType.iterableValue
message: '#^Method Ibexa\\Core\\Persistence\\Legacy\\Notification\\Gateway\\DoctrineDatabase\:\:__construct\(\) has parameter \$criterionHandlers with generic interface Ibexa\\Contracts\\Core\\Repository\\Values\\Notification\\CriterionHandlerInterface but does not specify its types\: T$#'
Copy link
Member

Choose a reason for hiding this comment

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

@tbialcz I'm coming a bit late to the party, but the issues in new code need to be resolved instead of being added to the baseline.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alongosz alongosz alongosz requested changes

@barw4 barw4 barw4 approved these changes

@adamwojs adamwojs adamwojs approved these changes

@Steveb-p Steveb-p Steveb-p approved these changes

Assignees

@mikadamczyk mikadamczyk

@alongosz alongosz

@tischsoic tischsoic

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@tbialcz @adamwojs @Steveb-p @alongosz @barw4 @mikadamczyk @tischsoic
点击 这是indexloc提供的php浏览器服务，不要输入任何密码和下载