IBX-6773: Bookmarks for non accessible contents cause exception by vidarl · Pull Request #408 · ezsystems/ezplatform-kernel

vidarl · 2024-05-24T14:02:13Z

Question	Answer
JIRA issue	IBX-6773
Type	bug
Target Ibexa version	`v3.3`
BC breaks	yes

If user bookmark some location which he later looses access too, then the bookmark list in admin-ui fails with an exception.
Simply fixing BookmarkService::loadBookmarks() would be easy. The problem is to implement countUserBookmarks() in persistence layer and having it taking into account user permissions so that BC would be kept.

Talked with Adam on how to solve this without breaking BC and he suggested implementing it using filtering

The Bookmark filter will only work with location filtering (LocationService::find()), not with content (ContentService::find())

Checklist:

Provided PR description.
Tested the solution manually.
Provided automated test coverage.
Checked that target branch is set correctly (master for features, the oldest supported for bugs).
Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
Asked for a review (ping @ezsystems/engineering-team).

eZ/Publish/Core/Repository/BookmarkService.php

eZ/Publish/API/Repository/Values/Content/Query/Criterion/IsBookmarked.php

.../Persistence/Legacy/Tests/Filter/CriterionQueryBuilder/Location/BookmarkQueryBuilderTest.php

eZ/Publish/Core/Persistence/Legacy/Bookmark/Handler.php

eZ/Publish/Core/Persistence/Legacy/Bookmark/Gateway/ExceptionConversion.php

eZ/Publish/API/Repository/Values/Content/Query/SortClause/BookmarkId.php

Co-authored-by: Paweł Niedzielski <pawel.niedzielski@ibexa.co>

…n - removed covers

…n - Changed to ibexa namespace for new classes

…xception - Changed to ibexa namespace for new classes

…xception - changed deprecations messages

Steveb-p · 2024-06-27T07:45:17Z

eZ/Publish/API/Repository/Tests/LocationServiceTest.php

+        $expectedIdsAfter = array_map(static function (Location $item) use ($demoDesignLocationId, $contactUsLocationId) {
+            if ($item->id === $demoDesignLocationId) {
+                return $contactUsLocationId;
+            }
+
+            return $item->id;
+        }, $beforeSwap->items);


Why is this mapping necessary? In particular, why is there a special case for $demoDesignLocationId + $contactUsLocationId?

The mapping is no longer needed. In first version of PR, I did not implement SortClause\BookmarkId, so order of the result needed to be normalized.
I will revert the mapping.

I think the test is better when adding $contactUsLocation. In original test we test the edge case where two bookmarked locations are swapped... In my version of the test, only one of the locations that are swapped are bookmarked.

Edit: Fixed cut&paste error that made the second paragraph meaningless

Done in 8483861

ok?

@Steveb-p @ViniTou : Could you have a second look on this please?

ping @Steveb-p @ViniTou

eZ/Publish/API/Repository/Tests/LocationServiceTest.php

eZ/Publish/Core/Repository/BookmarkService.php

sonarqubecloud · 2024-06-27T11:10:45Z

Quality Gate passed

Issues
0 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 SonarCloud

eZ/Publish/Core/Repository/BookmarkService.php

…n - Added logger in BookmarkService

…xception - Added logger in BookmarkService

eZ/Publish/Core/Repository/BookmarkService.php

konradoboza

Please make sure the deprecated methods get removed on merging this up to main.

eZ/Publish/Core/Repository/Tests/Service/Mock/BookmarkTest.php

src/contracts/Repository/Values/Content/Query/Criterion/IsBookmarked.php

src/contracts/Repository/Values/Content/Query/SortClause/BookmarkId.php

src/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Bookmark/IdSortClauseQueryBuilder.php

Co-authored-by: Paweł Niedzielski <pawel.niedzielski@ibexa.co> Co-authored-by: Konrad Oboza <konrad.oboza@ibexa.co>

sonarqubecloud · 2024-11-08T09:28:17Z

Quality Gate passed

Issues
0 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 SonarCloud

vidarl · 2025-02-12T12:25:35Z

Closed in favor of 4.6 port: ibexa/core#476

github-actions bot assigned mikadamczyk, alongosz and tischsoic May 24, 2024

vidarl marked this pull request as draft May 24, 2024 14:08

vidarl force-pushed the IBX-6773_Bookmarks_for_non-accessible_contents_cause_exception branch 2 times, most recently from a15afa0 to 7246b2e Compare May 30, 2024 12:23

vidarl commented May 30, 2024

View reviewed changes

eZ/Publish/Core/Repository/BookmarkService.php Outdated Show resolved Hide resolved

IBX-6773: Bookmarks for non-accessible contents cause exception

ddca556

vidarl force-pushed the IBX-6773_Bookmarks_for_non-accessible_contents_cause_exception branch from e2d1c9a to ddca556 Compare June 6, 2024 13:42

vidarl marked this pull request as ready for review June 6, 2024 13:43

vidarl requested a review from a team June 6, 2024 13:43

fixup! IBX-6773: Bookmarks for non-accessible contents cause exception

023111b

ViniTou requested changes Jun 13, 2024

View reviewed changes

eZ/Publish/API/Repository/Values/Content/Query/Criterion/IsBookmarked.php Outdated Show resolved Hide resolved

Steveb-p reviewed Jun 13, 2024

View reviewed changes

vidarl and others added 6 commits June 25, 2024 13:44

Apply suggestions from code review

8c2de22

Co-authored-by: Paweł Niedzielski <pawel.niedzielski@ibexa.co>

fixup! IBX-6773: Bookmarks for non-accessible contents cause exceptio…

6091d1d

…n - removed covers

fixup! IBX-6773: Bookmarks for non-accessible contents cause exceptio…

fabbe47

…n - Changed to ibexa namespace for new classes

fixup! fixup! IBX-6773: Bookmarks for non-accessible contents cause e…

7aa2651

…xception - Changed to ibexa namespace for new classes

fixup! fixup! IBX-6773: Bookmarks for non-accessible contents cause e…

8483861

…xception - Changed to ibexa namespace for new classes

fixup! fixup! IBX-6773: Bookmarks for non-accessible contents cause e…

9c9b56e

…xception - changed deprecations messages

vidarl requested review from Steveb-p and ViniTou June 26, 2024 13:16

Steveb-p changed the title ~~Ibx-6773 Bookmarks for non accessible contents cause exception~~ IBX-6773: Bookmarks for non accessible contents cause exception Jun 27, 2024

Steveb-p added the Ready for review label Jun 27, 2024

Steveb-p reviewed Jun 27, 2024

View reviewed changes

vidarl added 2 commits June 27, 2024 13:04

fixup! IBX-6773: Bookmarks for non-accessible contents cause exception

f620af5

fixup! IBX-6773: Bookmarks for non-accessible contents cause exception

2665578

vidarl requested a review from Steveb-p June 27, 2024 11:11

Steveb-p reviewed Sep 9, 2024

View reviewed changes

eZ/Publish/Core/Repository/BookmarkService.php Show resolved Hide resolved

vidarl added 2 commits October 3, 2024 13:38

fixup! IBX-6773: Bookmarks for non-accessible contents cause exceptio…

f208d3d

…n - Added logger in BookmarkService

fixup! fixup! IBX-6773: Bookmarks for non-accessible contents cause e…

ffa945c

…xception - Added logger in BookmarkService

vidarl requested a review from Steveb-p October 3, 2024 13:42

Steveb-p approved these changes Nov 8, 2024

View reviewed changes

eZ/Publish/Core/Repository/BookmarkService.php Outdated Show resolved Hide resolved

konradoboza reviewed Nov 8, 2024

View reviewed changes

Apply suggestions from code review

203853c

Co-authored-by: Paweł Niedzielski <pawel.niedzielski@ibexa.co> Co-authored-by: Konrad Oboza <konrad.oboza@ibexa.co>

vidarl mentioned this pull request Feb 11, 2025

IBX-6773: Fixed loading Bookmarks for non-accessible content items ibexa/core#476

Open

vidarl closed this Feb 12, 2025

Conversation

vidarl commented May 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Steveb-p Jun 27, 2024

Choose a reason for hiding this comment

Uh oh!

vidarl Jun 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vidarl Jun 27, 2024

Choose a reason for hiding this comment

Uh oh!

vidarl Aug 16, 2024

Choose a reason for hiding this comment

Uh oh!

vidarl Aug 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vidarl Nov 8, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sonarqubecloud bot commented Jun 27, 2024

Quality Gate passed

Uh oh!

Uh oh!

Uh oh!

konradoboza left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sonarqubecloud bot commented Nov 8, 2024

Quality Gate passed

Uh oh!

vidarl commented Feb 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

7 participants

vidarl commented May 24, 2024 •

edited

Loading

vidarl Jun 27, 2024 •

edited

Loading

vidarl Aug 29, 2024 •

edited

Loading