IBX-5905: Implemented LoadContent events #250

mateuszdebinski · 2023-07-26T10:28:57Z

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

PR is currently introducing one of the 3 things needed to solve the problem described in JIRA. Thanks to the content type identifier in ContentInfo, we will be able to decide whether the content type belongs to a taxonomy. If so, it will allow us to verify whether the user has the appropriate permissions. The permissions will be checked by introducing a new Event triggered after the content is loaded.
Work on the fix includes:

Adding content type identifier to ContentInfo
Creating an event that will be triggered after loading the content
Adding verification whether the content belongs to taxonomy and if so, verification of permissions to them

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 (ping @ibexa/engineering).

alongosz

Looks almost ok, but I have some naming & query building remarks.

Also ideally if we keep only necessary legacy persistence fixtures changes.

We need to either add new integration test or modify existing one which checks properties after creating and/or loading content type to see that it's set.

src/lib/Persistence/Legacy/Content/Gateway/DoctrineDatabase.php

src/contracts/Persistence/Content/ContentInfo.php

src/lib/Persistence/Legacy/Content/Gateway/DoctrineDatabase.php

src/lib/Persistence/Legacy/Content/Gateway/DoctrineDatabase/QueryBuilder.php

src/lib/Persistence/Legacy/Content/Mapper.php

src/lib/Search/Legacy/Content/Gateway/DoctrineDatabase.php

tests/lib/Persistence/Legacy/Content/Gateway/DoctrineDatabaseTest.php

alongosz

Looks ok. Let's make CI green.
Small remarks:

tests/lib/Persistence/Legacy/Content/Gateway/DoctrineDatabaseTest.php

tests/lib/Persistence/ValueObject/ContentInfoTest.php

lserwatka · 2023-07-31T06:46:16Z

Just an thought, would be good to test it on large database to see how this change is performing. @mateuszdebinski I think you have access to large datasets.

tests/lib/Persistence/ValueObject/ContentInfoTest.php

mateuszdebinski · 2023-08-07T09:59:44Z

@lserwatka from what I've verified, there shouldn't be any problems with performance. I checked all SQL queries to which I added a new column and didn't see any changes in performance. I tested on a database that has a size of 43GB before uploading it to the database

sonarcloud · 2023-08-07T10:05:30Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

adamwojs · 2023-08-25T17:52:06Z

@mateuszdebinski Could you please add description to PR explaining how this change is related to linked issue?

adamwojs · 2023-10-05T06:53:40Z

@mateuszdebinski Rebase is needed here

sonarcloud · 2023-10-09T09:36:56Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

alongosz · 2024-06-17T10:48:54Z

@mateuszdebinski what is the status here? I see CI is failing and it was never sent to QA. At this point it would need to be rebased to 4.6, if still valid.

mateuszdebinski · 2024-06-17T11:01:45Z

@alongosz There's one more thing missing, I'll take care of it this week and do a rebase to 4.6

alongosz · 2024-07-09T11:55:38Z

Hi @mateuszdebinski, any updates on this one?

mateuszdebinski · 2024-07-25T12:09:01Z

@alongosz I've added the final things to this PR. Please verify the changes

alongosz

Seems that you're trying to solve too much at once. The scope here was to add identifier to ContentInfo. Where that new event is used?

phpstan-baseline.neon

src/contracts/Repository/Events/Content/BeforeLoadContentEvent.php

alongosz · 2024-07-26T12:52:37Z

tests/lib/Event/AbstractServiceTest.php

@@ -15,11 +15,14 @@

 abstract class AbstractServiceTest extends TestCase
 {
-    public function getEventDispatcher(string $beforeEventName, string $eventName): TraceableEventDispatcher
+    public function getEventDispatcher(string $beforeEventName, ?string $eventName): TraceableEventDispatcher


I have a feeling this is here on purpose so you won't forget to implement a pair of events - before event and event.
Any POV @Nattfarinn or @ibexa/php-dev?

I didn't add the after event for the loadContent function here, but I will add it if necessary :)

I would say event layer is complete only if it dispatches Before and After events. Before for pipeline interception and input refinement, After as a reaction and postprocessing.

I would add After event. Thanks @alongosz for catching this 👍 .

tests/lib/Event/ContentServiceTest.php

sonarcloud · 2024-07-26T14:31:25Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

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

See analysis details on SonarCloud

mateuszdebinski · 2024-07-26T14:35:13Z

@alongosz This PR is part of the solution to the problem and for this, we will need this Event to verify in taxonomy whether the content type points to taxonomy to verify the appropriate permissions. PR for taxonomy will be added soon. First, I wanted to have a completed PR for core (without merge but confirmed that everything is ok)

alongosz

This PR is part of the solution to the problem and for this, we will need this Event to verify in taxonomy whether the content type points to taxonomy to verify the appropriate permissions. PR for taxonomy will be added soon. First, I wanted to have a completed PR for core (without merge but confirmed that everything is ok)

@mateuszdebinski in that case the essence of the PR changed. I've changed the title accordingly. If you feel that both of these things (events + content type identifier) are separate, then I'd just extract one of those to separate PR and target one PR on top of another. However ATM for me it's not a requirement.

mateuszdebinski added the Work in progress label Jul 26, 2023

mateuszdebinski self-assigned this Jul 26, 2023

github-actions bot assigned alongosz, kisztof and mikadamczyk Jul 26, 2023

mateuszdebinski force-pushed the IBX-5905_content_type_identifier_in_contentInfo branch from 4f3f5da to f36b357 Compare July 26, 2023 11:35

alongosz reviewed Jul 26, 2023

View reviewed changes

alongosz reviewed Jul 27, 2023

View reviewed changes

mateuszdebinski force-pushed the IBX-5905_content_type_identifier_in_contentInfo branch 5 times, most recently from e1884e3 to b8ab8df Compare July 28, 2023 13:14

alongosz approved these changes Jul 28, 2023

View reviewed changes

alongosz requested a review from a team July 28, 2023 14:26

webhdx approved these changes Jul 31, 2023

View reviewed changes

konradoboza reviewed Jul 31, 2023

View reviewed changes

tests/lib/Persistence/ValueObject/ContentInfoTest.php Show resolved Hide resolved

kisztof approved these changes Jul 31, 2023

View reviewed changes

mateuszdebinski requested a review from konradoboza August 7, 2023 10:01

konradoboza approved these changes Aug 7, 2023

View reviewed changes

mateuszdebinski force-pushed the IBX-5905_content_type_identifier_in_contentInfo branch from 98d2ff3 to be0793c Compare October 9, 2023 09:32

alongosz added the Support 🚨 label Jul 9, 2024

mateuszdebinski force-pushed the IBX-5905_content_type_identifier_in_contentInfo branch from be0793c to a9c82ec Compare July 9, 2024 11:59

mateuszdebinski changed the base branch from 4.5 to 4.6 July 9, 2024 11:59

github-actions bot assigned tischsoic Jul 9, 2024

mateuszdebinski added 7 commits July 25, 2024 10:41

IBX-5905: Added content type identifier to ContentInfo

4eae46a

Changed naming & query building, tests adapted to changes

f313a56

Removed unnecessary tests, Added exclusions for sonar cloud

4085781

Removed unnecessary tests, Added exclusions for sonar cloud

4460b5c

Removed sonar-project.properties

472b69b

Added declaring strict types.

04c4bac

Added BeforeLoadContentEvent

a94c6b8

mateuszdebinski force-pushed the IBX-5905_content_type_identifier_in_contentInfo branch from a9c82ec to a94c6b8 Compare July 25, 2024 11:22

mateuszdebinski requested review from kisztof, alongosz and konradoboza July 25, 2024 12:08

mateuszdebinski added Ready for review and removed Work in progress labels Jul 25, 2024

alongosz reviewed Jul 26, 2024

View reviewed changes

Corrected CS

63485b4

mateuszdebinski requested a review from alongosz July 29, 2024 08:25

alongosz changed the title ~~IBX-5905: Added content type identifier to ContentInfo~~ IBX-5905: Implemented LoadContent events Aug 6, 2024

alongosz reviewed Aug 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IBX-5905: Implemented LoadContent events #250

IBX-5905: Implemented LoadContent events #250

mateuszdebinski commented Jul 26, 2023 •

edited

Loading

alongosz left a comment

alongosz left a comment

lserwatka commented Jul 31, 2023

mateuszdebinski commented Aug 7, 2023

sonarcloud bot commented Aug 7, 2023

adamwojs commented Aug 25, 2023

adamwojs commented Oct 5, 2023

sonarcloud bot commented Oct 9, 2023

alongosz commented Jun 17, 2024

mateuszdebinski commented Jun 17, 2024

alongosz commented Jul 9, 2024

mateuszdebinski commented Jul 25, 2024

alongosz left a comment

alongosz Jul 26, 2024

mateuszdebinski Jul 26, 2024

Nattfarinn Aug 6, 2024

sonarcloud bot commented Jul 26, 2024

mateuszdebinski commented Jul 26, 2024

alongosz left a comment

IBX-5905: Implemented LoadContent events #250

Are you sure you want to change the base?

IBX-5905: Implemented LoadContent events #250

Conversation

mateuszdebinski commented Jul 26, 2023 • edited Loading

Checklist:

alongosz left a comment

Choose a reason for hiding this comment

alongosz left a comment

Choose a reason for hiding this comment

lserwatka commented Jul 31, 2023

mateuszdebinski commented Aug 7, 2023

sonarcloud bot commented Aug 7, 2023

adamwojs commented Aug 25, 2023

adamwojs commented Oct 5, 2023

sonarcloud bot commented Oct 9, 2023

alongosz commented Jun 17, 2024

mateuszdebinski commented Jun 17, 2024

alongosz commented Jul 9, 2024

mateuszdebinski commented Jul 25, 2024

alongosz left a comment

Choose a reason for hiding this comment

alongosz Jul 26, 2024

Choose a reason for hiding this comment

mateuszdebinski Jul 26, 2024

Choose a reason for hiding this comment

Nattfarinn Aug 6, 2024

Choose a reason for hiding this comment

sonarcloud bot commented Jul 26, 2024

Quality Gate passed

mateuszdebinski commented Jul 26, 2024

alongosz left a comment

Choose a reason for hiding this comment

mateuszdebinski commented Jul 26, 2023 •

edited

Loading