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

IBX-6815: [REST] Fixed isBookmarked property in load-subtree response #1139

Merged
merged 8 commits into from
Feb 6, 2024

Conversation

alongosz
Copy link
Member

@alongosz alongosz commented Jan 30, 2024

Question Answer
JIRA issue IBX-6815
Type bug
Target Ibexa version v4.5
BC breaks no

A merge up caused the regression which resulted in Location bookmark information not being propagated when building ContentTree REST response (/load-subtree endpoint).

We need some minimal test coverage for that endpoint, as the issue was almost impossible to detect due to the complexity of the changes.

TODO

  • tests

Checklist:

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

@alongosz alongosz added the Bug Something isn't working label Jan 30, 2024
@adamwojs
Copy link
Member

What about performance ?

@alongosz
Copy link
Member Author

What about performance ?

The performance of loading isBookmarked information for a Location has been improved via #744 and then removed in total by the mentioned merge up. This PR just restores #744 fix.

Obviously, it will impact time & memory complexity as this is additional operation, however I'm not sure if you can avoid it. Content Tree needs that information to work properly.

The edge case I see in #744 fix is when user has a lot of [1] bookmarks. However I expect it to be more performant than what was there before the fix, which was $this->bookmarkService->isBookmarked($location).

[1] Not sure how many bookmarks means "a lot" or "too many".

Alternatively, we could:

  • change LocationService and Location ValueObject to include OOTB isBookmarked information. That however impacts the whole system, not just load-subtree endpoint.
  • iterate over created Node structure for ContentTree and add bulk isBookmarked(LocationList) method to BookmarkService and check it there

Regardless of the choice, I don't think this is the scope of this bugfix.

@alongosz alongosz force-pushed the ibx-6815-fix-is-bookmarked-in-load-subtree branch from 5fb668b to 11eb2be Compare February 5, 2024 15:29
@alongosz alongosz force-pushed the ibx-6815-fix-is-bookmarked-in-load-subtree branch from 11eb2be to 95c72fd Compare February 5, 2024 16:22
Copy link

sonarcloud bot commented Feb 5, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@mnocon mnocon self-assigned this Feb 6, 2024
@ViniTou ViniTou merged commit 11849cf into 4.5 Feb 6, 2024
19 checks passed
@ViniTou ViniTou deleted the ibx-6815-fix-is-bookmarked-in-load-subtree branch February 6, 2024 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants