Skip to content

Fix error when saving contacts without points available #14714

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 9 commits into from
Jun 30, 2025
Merged

Fix error when saving contacts without points available #14714

merged 9 commits into from
Jun 30, 2025
+25 −0

Conversation

npracht
Copy link
Member

@npracht npracht commented Mar 10, 2025 •
edited by kuzmany
Loading

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
Related developer documentation PR URL
Issue(s) addressed

Description

Save Lead entity just If points log exist to avoid error.

📋 Steps to test this PR:

  1. Technical, code review should be enough
  2. You can test any random point action If works like before

@npracht npracht added T3 Hard difficulty to fix (issue) or test (PR) bug Issues or PR's relating to bugs points-scoring Anything related to points contacts Anything related to contacts labels Mar 10, 2025
@npracht npracht moved this to 🧑🏻‍💻 Needs a code review in Open Source Fridays Mar 10, 2025
@codecov Codecov
Copy link

codecov bot commented Mar 10, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.72%. Comparing base (8bcd215) to head (6c6c52c).
Report is 25 commits behind head on 5.2.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                5.2   #14714      +/-   ##
============================================
- Coverage     63.72%   63.72%   -0.01%     
- Complexity    34687    34688       +1     
============================================
  Files          2274     2274              
  Lines        103777   103778       +1     
============================================
  Hits          66132    66132              
- Misses        37645    37646       +1
Files with missing lines Coverage Δ
app/bundles/PointBundle/Model/PointModel.php 76.19% <100.00%> (+0.32%) ⬆️

... and 2 files 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.

escopecz
escopecz approved these changes Mar 12, 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.

From the code perspective it makes sense to stop here if the $availablePoints are empty. But knowing how to reproduce this state and having a test covering it would be really great.

@RCheesley
Copy link
Member

@npracht I have no idea how to reproduce this bug or how to test. Please provide extended instructions if you'd like us to merge? Plus see comment regarding test coverage.

Thanks!

kuzmany added 3 commits March 14, 2025 14:28
@kuzmany
test: Add PointModelTest unit test file
f2224ec
@kuzmany
The test file looks good and covers the key scenarios for the early r…
92a2e25 
…eturn functionality in the PointModel. Let me break down what the tests verify:

1. `testTriggerActionReturnsEarlyWhenNoAvailablePoints`:
   - Checks that when no points are available for a given type
   - The method returns early
   - The IP lookup helper is never called

2. `testTriggerActionContinuesWhenPointsAreAvailable`:
   - Checks that when points are available for a given type
   - The method continues execution
   - The IP lookup helper is called

The test uses mocking to isolate the behavior and verify the specific interactions.

Recommendations:
- The tests look comprehensive for the specific change
- No additional files are needed at this point

Would you like me to elaborate on any part of the test or discuss any specific aspects of the implementation?
@kuzmany
refactor: Clean up test file and update method call signature
1cdc69d
@kuzmany
Copy link
Member

kuzmany commented Mar 14, 2025

@RCheesley I believe it creates anonymous leads when forms are submitted, but it was a long time ago, so I’m not sure. Just test If points working properly.

@kuzmany
Copy link
Member

kuzmany commented Mar 14, 2025

@escopecz tests added

escopecz
escopecz requested changes Mar 14, 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.

It would be better to use a functional test which actually fails when a bug happens and not when we refactor the internal code and have to refactor the unit test mocks along with it. I can see you got lost in implementing the mocked methods. A functional test is usually even faster way how to cover the code.

@github-project-automation github-project-automation bot moved this from 🧑🏻‍💻 Needs a code review to 🚨 Developer revision needed in Open Source Fridays Mar 14, 2025
@kuzmany
Copy link
Member

kuzmany commented Mar 14, 2025 •
edited
Loading

It would be better to use a functional test which actually fails when a bug happens and not when we refactor the internal code and have to refactor the unit test mocks along with it. I can see you got lost in implementing the mocked methods. A functional test is usually even faster way how to cover the code.

What should we check in functional tests? For example, there are already tests for the triggerAction method

mautic/app/bundles/PageBundle/Tests/EventListener/PointSubscriberTest.php

Line 74 in 58cb5ee

$pointModel->expects(self::once())->method('triggerAction')->with('page.hit', $hit, null, $lead);

@escopecz
Copy link
Member

The functional tests should configure the scenario that replicates the bug. I can't tell as the steps to test are missing.

@kuzmany
Copy link
Member

kuzmany commented Mar 14, 2025

@escopecz it's very specific case with our private plugin.

@kuzmany kuzmany closed this Mar 14, 2025
@github-project-automation github-project-automation bot moved this from 🚨 Developer revision needed to 🥳 Done in Open Source Fridays Mar 14, 2025
@kuzmany kuzmany reopened this Mar 14, 2025
kuzmany added 3 commits June 20, 2025 12:57
@kuzmany
Merge remote-tracking branch 'origin/5.2' into fix/save-lead-entity-i…
9b20434 
…f-points-log-exists
@kuzmany
Remove PointModelTest.php file
08da6ea
@kuzmany
Add test for early return scenario when no points available
7fb721e 
- Added testPointActionEarlyReturnWhenNoPointsAvailable() method
- Tests that PointModel::triggerAction() returns early when no published point actions exist
- Verifies points remain unchanged (0) when no point actions are configured
- Covers the optimization added in the PR fix
@kuzmany kuzmany added the ready-to-test PR's that are ready to test label Jun 20, 2025
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@kuzmany
Copy link
Member

kuzmany commented Jun 30, 2025

@escopecz can you consider approve it? we need get rid similar PRs like this. It can break nothing

JonasLudwig1998
JonasLudwig1998 approved these changes Jun 30, 2025
View reviewed changes
Copy link
Contributor

@JonasLudwig1998 JonasLudwig1998 left a comment

Choose a reason for hiding this comment

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

The code changes look fine to me. Can be merged before the last bug fix release from my point of view

@kuzmany kuzmany merged commit 4420f87 into mautic:5.2 Jun 30, 2025
18 of 19 checks passed
@kuzmany kuzmany changed the title check if points available Fix error when saving contacts without points available Jun 30, 2025
@kuzmany kuzmany added this to the 6.0.3 milestone Jul 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@escopecz escopecz escopecz requested changes

+1 more reviewer

@JonasLudwig1998 JonasLudwig1998 JonasLudwig1998 approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
bug Issues or PR's relating to bugs contacts Anything related to contacts points-scoring Anything related to points ready-to-test PR's that are ready to test T3 Hard difficulty to fix (issue) or test (PR)
Projects
Open Source Fridays
Status: 🥳 Done
Milestone
6.0.3
Development

Successfully merging this pull request may close these issues.
5 participants
@npracht @RCheesley @kuzmany @escopecz @JonasLudwig1998