Skip to content

UUID implementation fix + added to roles and permissions #14751

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 5 commits into from
Mar 19, 2025
Merged

UUID implementation fix + added to roles and permissions #14751

merged 5 commits into from
Mar 19, 2025

Conversation

dadarya0
Copy link
Contributor

Cf 583: Copy Roles via RC/GT.

Q A
Bug fix? (use the a.b branch) ❌
New feature/enhancement? (use the a.x branch) ✔️
Deprecations? ❌
BC breaks? (use the c.x branch) ❌
Automated tests included? ❌
Related user documentation PR URL mautic/user-documentation#...
Related developer documentation PR URL mautic/developer-documentation-new#...
Issue(s) addressed Fixes #...

Description

UUID implementation. This is necessary for global identification of entities for the campaign library:

#14504

The UUID will be used to decide whether an entity should be updated or inserted during the import of entities.

📋 Steps to test this PR:

  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)
  2. Run bin/console doctrine:migrations:migrate to apply the migration.
  3. You should see in the changed entities/tables in role and permission and so on that there is new UUID column in the table with filled in hash.

@dadarya0 dadarya0 changed the title Merge pull request #1474 from acquia/CF-583 UUID implementation for roles and permissions Mar 18, 2025
@codecov Codecov
Copy link

codecov bot commented Mar 18, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.72%. Comparing base (12c2a02) to head (068c3b0).
Report is 6 commits behind head on 6.x.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##                6.x   #14751   +/-   ##
=========================================
  Coverage     64.71%   64.72%           
- Complexity    34710    34720   +10     
=========================================
  Files          2274     2275    +1     
  Lines        103689   103708   +19     
=========================================
+ Hits          67105    67127   +22     
+ Misses        36584    36581    -3
Files with missing lines Coverage Δ
...es/CoreBundle/Entity/DynamicContentEntityTrait.php 92.85% <ø> (ø)
...ndles/CoreBundle/Entity/TranslationEntityTrait.php 86.11% <ø> (ø)
app/bundles/CoreBundle/Entity/UuidTrait.php 100.00% <ø> (+25.00%) ⬆️
.../bundles/CoreBundle/EventListener/UUIDListener.php 100.00% <100.00%> (ø)
app/bundles/UserBundle/Entity/Permission.php 76.19% <100.00%> (+0.58%) ⬆️
app/bundles/UserBundle/Entity/Role.php 85.89% <100.00%> (+0.18%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@dadarya0 dadarya0 added the unforking Used for PRs in the Acquia's unforking initiative label Mar 18, 2025
@dadarya0 dadarya0 requested review from a team, escopecz and nileshlohar and removed request for a team March 18, 2025 09:27
@escopecz
Copy link
Member

@dadarya0 the UUID will be used for the Campaign Library. I just learned from @levente999 that the UUIDs aren't getting populated on entity creation. Could you please also push that part? Either here or new PR.

@escopecz escopecz requested a review from levente999 March 18, 2025 10:46
escopecz
escopecz requested changes Mar 18, 2025
View reviewed changes
Copy link
Member

@escopecz escopecz left a comment

Choose a reason for hiding this comment

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

We have to add the code that is populating the UUIDs on entity creation otheriwse we'll have to execute the update queries again once we do. Let's get that done before M6-stable release.

@dadarya0
Copy link
Contributor Author

@dadarya0 the UUID will be used for the Campaign Library. I just learned from @levente999 that the UUIDs aren't getting populated on entity creation. Could you please also push that part? Either here or new PR.

set UUIDs on entity creation code is pushed in last commit

@dadarya0 dadarya0 requested a review from escopecz March 19, 2025 07:18
@escopecz escopecz added the enhancement Any improvement to an existing feature or functionality label Mar 19, 2025
@escopecz
Copy link
Member

Just thinking that we've merged the most of UUID changes into M6 in #14558 and this would go to M7. I think this should be rebased to the 6.x branch as a bug fix of #14558 as users would get many rows without UUIDs filled in when using M6. What do you think? Shall we rebase?

@dadarya0 dadarya0 changed the base branch from 7.x to 6.x March 19, 2025 10:12
@dadarya0 dadarya0 changed the base branch from 6.x to 7.x March 19, 2025 10:14
@dadarya0 dadarya0 changed the base branch from 7.x to 6.x March 19, 2025 10:19
@dadarya0 dadarya0 force-pushed the uuid-for-role-and-permission branch from f2cf7f1 to 068c3b0 Compare March 19, 2025 10:19
@dadarya0
Copy link
Contributor Author

Just thinking that we've merged the most of UUID changes into M6 in #14558 and this would go to M7. I think this should be rebased to the 6.x branch as a bug fix of #14558 as users would get many rows without UUIDs filled in when using M6. What do you think? Shall we rebase?

I have changed target branch from 7.x to 6.x

@escopecz escopecz added this to the 6.0.0 milestone Mar 19, 2025
@escopecz escopecz added bug Issues or PR's relating to bugs and removed enhancement Any improvement to an existing feature or functionality labels Mar 19, 2025
@escopecz escopecz changed the title UUID implementation for roles and permissions UUID implementation fix + added to roles and permissions Mar 19, 2025
escopecz
escopecz approved these changes Mar 19, 2025
View reviewed changes
Copy link
Member

@escopecz escopecz left a comment

Choose a reason for hiding this comment

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

Thanks Saurabh! The code looks great. I tested the migrations and also created a new segment and checked in the database that the UUID value was filled in.

@escopecz escopecz added the pending-test-confirmation PR's that require one test before they can be merged label Mar 19, 2025
@escopecz escopecz added code-review-passed PRs which have passed code review user-testing-passed PRs which have been successfully tested by the required number of people. labels Mar 19, 2025
levente999
levente999 approved these changes Mar 19, 2025
View reviewed changes
Copy link
Contributor

@levente999 levente999 left a comment

Choose a reason for hiding this comment

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

Looks good.

@escopecz escopecz removed the user-testing-passed PRs which have been successfully tested by the required number of people. label Mar 19, 2025
@escopecz escopecz merged commit 1bcae7d into mautic:6.x Mar 19, 2025
17 checks passed
@escopecz escopecz mentioned this pull request Mar 20, 2025
Merged
@mautibot
Copy link
Contributor

mautibot commented May 1, 2025

This pull request has been mentioned on Mautic Forums. There might be relevant details there:

https://forum.mautic.org/t/issue-with-logging-in-to-mautic-6-0-after-upgrade-from-5-2-5/35610/3

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

@escopecz escopecz escopecz approved these changes

@nileshlohar nileshlohar Awaiting requested review from nileshlohar nileshlohar was automatically assigned from mautic/acquia-engineers

+1 more reviewer

@levente999 levente999 levente999 approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
bug Issues or PR's relating to bugs code-review-passed PRs which have passed code review pending-test-confirmation PR's that require one test before they can be merged unforking Used for PRs in the Acquia's unforking initiative
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@dadarya0 @escopecz @mautibot @levente999 @gunnrryy