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.

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

IBX-1310: Created slots for assigning/unassigning user to groups #3125

Merged
merged 8 commits into from
Dec 10, 2021
Merged

IBX-1310: Created slots for assigning/unassigning user to groups #3125

merged 8 commits into from
Dec 10, 2021

Conversation

barw4
Copy link
Member

@barw4 barw4 commented Oct 22, 2021 •
edited

Question Answer
JIRA issue IBX-1310
Type bug
Target eZ Platform version v2.5
BC breaks no

The related Slots were missing for assignUserToUserGroup and unAssignUserToUserGroup UserService's methods which resulted in not indexing the related subtrees.

Required PR: ezsystems/ezplatform-solr-search-engine#223

3.3. PR: ezsystems/ezplatform-kernel#269

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).

@barw4 barw4 added the Bug label Oct 22, 2021
@barw4 barw4 self-assigned this Oct 22, 2021
@barw4 barw4 changed the title IBX-1310: Created slots for assigning/unassigning user to groups [WIP] IBX-1310: Created slots for assigning/unassigning user to groups Oct 22, 2021
@barw4 barw4 changed the title [WIP] IBX-1310: Created slots for assigning/unassigning user to groups IBX-1310: Created slots for assigning/unassigning user to groups Oct 22, 2021
@barw4 barw4 mentioned this pull request Oct 23, 2021
Merged
@barw4 barw4 requested a review from a team October 26, 2021 09:28
@Nattfarinn Nattfarinn requested a review from a team October 27, 2021 07:35
@adamwojs
Copy link
Member

@barw4 Could you please provide PRs for ezplatform-kernel and ezplatform-elastic-search-engine?

alongosz
alongosz requested changes Nov 26, 2021
View reviewed changes
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

As stated in #3125 (comment) and by the other Reviewers.

alongosz
alongosz requested changes Nov 30, 2021
View reviewed changes
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

It looks ok now. What I'm missing is test coverage, for the use case the integration one.

@barw4 barw4 requested a review from alongosz December 1, 2021 09:17
alongosz
alongosz requested changes Dec 1, 2021
View reviewed changes
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

Looks complete now, CS remark (more refactoring needed to align with it):

eZ/Publish/API/Repository/Tests/UserServiceTest.php Outdated Show resolved Hide resolved
eZ/Publish/API/Repository/Tests/UserServiceTest.php Outdated Show resolved Hide resolved
@barw4 barw4 requested a review from alongosz December 1, 2021 09:25
@barw4
Copy link
Member Author

barw4 commented Dec 1, 2021 •
edited

Solr test (https://app.travis-ci.com/github/ezsystems/ezpublish-kernel/jobs/550349178) most likely failing from lack of ezsystems/ezplatform-solr-search-engine#223

@barw4 barw4 mentioned this pull request Dec 2, 2021
Merged
6 tasks
@barw4
Copy link
Member Author

barw4 commented Dec 3, 2021

@adamwojs @alongosz PR for 3.3 has been created: ezsystems/ezplatform-kernel#269, Elastic PR wasn't necessary.

Steveb-p
Steveb-p approved these changes Dec 4, 2021
View reviewed changes
Copy link
Contributor

@Steveb-p Steveb-p left a comment

Choose a reason for hiding this comment

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

Noticed that the comments requesting to add final to introduced classes were discarded without explanation. Why is that? final protects our interests first & foremost, because we can safely change the classes in question without fear of breaking.

I know that this is 7.5 version and those classes will be dropped - or rather, they will be discarded when performing merge up - but still, it's a 20 second effort at most.

Unless, of course, there was a reason that was not posted here?

eZ/Publish/API/Repository/Tests/UserServiceTest.php Outdated Show resolved Hide resolved
@barw4
Copy link
Member Author

barw4 commented Dec 6, 2021 •
edited

Noticed that the comments requesting to add final to introduced classes were discarded without explanation. Why is that? final protects our interests first & foremost, because we can safely change the classes in question without fear of breaking.

I know that this is 7.5 version and those classes will be dropped - or rather, they will be discarded when performing merge up - but still, it's a 20 second effort at most.

Unless, of course, there was a reason that was not posted here?

final remarks were discarded as they applied to my previous solution which has been scrapped and I forgot to apply them afterwards.

@sonarcloud
Copy link

sonarcloud bot commented Dec 6, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@tomaszszopinski tomaszszopinski self-assigned this Dec 7, 2021
tomaszszopinski
tomaszszopinski approved these changes Dec 10, 2021
View reviewed changes
Copy link

@tomaszszopinski tomaszszopinski left a comment

Choose a reason for hiding this comment

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

QA approved on eZPlatform 2.5.

@adamwojs adamwojs merged commit da90d00 into 7.5 Dec 10, 2021
@adamwojs adamwojs deleted the ibx-1310-index-user-service-actions branch December 10, 2021 09:40
@barw4
Copy link
Member Author

barw4 commented Dec 10, 2021

Merged into:
ezplatform-kernel: ezsystems/ezplatform-kernel@4be1233
ibexa/core/main PR: ibexa/core#36

alongosz added a commit to ibexa/core that referenced this pull request Dec 14, 2021
@alongosz
Merged branch '1.3' of ezsystems/ezplatform-kernel into main
29035f2 
For more details see: https://issues.ibexa.co/browse/IBX-1310

Merged:

* da90d00 IBX-1310: Created slots for assigning/unassigning user to groups (ezsystems/ezpublish-kernel#3125)

* 5615ef7 IBX-1310: Created subscriber methods for assigning/unassigning user to groups (ezsystems/ezplatform-kernel#269)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamwojs adamwojs adamwojs approved these changes

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

@alongosz alongosz alongosz approved these changes

@tomaszszopinski tomaszszopinski tomaszszopinski approved these changes

@Nattfarinn Nattfarinn Awaiting requested review from Nattfarinn

Assignees

@barw4 barw4

@tomaszszopinski tomaszszopinski

Labels
Bug QA approved Ready for MERGE
Milestone
No milestone
6 participants
@barw4 @adamwojs @Steveb-p @Nattfarinn @alongosz @tomaszszopinski