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

Fix pending count on segment email #5697

Merged
merged 1 commit into from Mar 22, 2018
Merged

Fix pending count on segment email #5697

merged 1 commit into from Mar 22, 2018

Conversation

Dcoutelle
Copy link
Contributor

@Dcoutelle Dcoutelle commented Feb 9, 2018 •
edited

Q A
Bug fix? Y
New feature? N
Automated tests included? N
Related user documentation PR URL
Related developer documentation PR URL
Issues addressed (#s or URLs) #5553
BC breaks? N
Deprecations? N

Description:

When a lead is deleted, lead_id in email_stat table are set to NULL.
If one lead_id is NULL in segment email stat, pending count is not longer calculated proprely

Steps to reproduce the bug:

  1. Create a new segment
  2. Create a new segment email
  3. Add two contacts A and B to segment
  4. Shoot the email
  5. add a new contact C to the segment
  6. check in email interface, there is one pending email.
  7. Delete Contact A
  8. check in email interface, there is no pending email.

Steps to test this PR:

  1. apply PR
  2. check in email interface, there is one pending email ( contact C)

@mautibot mautibot added the code-review-needed PR's that require a code review before merging label Feb 9, 2018
@Dcoutelle Dcoutelle mentioned this pull request Feb 9, 2018
Closed
@npracht
Copy link
Member

npracht commented Feb 9, 2018

Label: Bug

@mautibot mautibot added the bug Issues or PR's relating to bugs label Feb 9, 2018
@npracht
Copy link
Member

npracht commented Feb 9, 2018

Label: Ready to test

@mautibot mautibot added the ready-to-test PR's that are ready to test label Feb 9, 2018
@Dcoutelle
Copy link
Contributor Author

I have updated the PR and the test process.

I understood how a lead_id can be null in email_stat table, it's when you delete a contact.

@dbhurley dbhurley added this to the 2.13.0 milestone Mar 4, 2018
npracht
npracht approved these changes Mar 20, 2018
View reviewed changes
Copy link
Member

@npracht npracht left a comment

Choose a reason for hiding this comment

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

Tested on a live instance to see the difference and it works properly !

@mautibot mautibot removed the code-review-needed PR's that require a code review before merging label Mar 20, 2018
@npracht
Copy link
Member

npracht commented Mar 20, 2018

Label: Pending test confirmation

@mautibot mautibot added the pending-test-confirmation PR's that require one test before they can be merged label Mar 20, 2018
@javjim
Copy link

javjim commented Mar 22, 2018

Tested, was able to reproduce bug and fix

@javjim javjim removed the ready-to-test PR's that are ready to test label Mar 22, 2018
@escopecz escopecz self-assigned this Mar 22, 2018
@mautibot mautibot added the code-review-needed PR's that require a code review before merging label Mar 22, 2018
escopecz
escopecz approved these changes Mar 22, 2018
View reviewed changes
Copy link
Sponsor 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.

Works correctly. Thanks! 👍

@escopecz escopecz removed code-review-needed PR's that require a code review before merging pending-test-confirmation PR's that require one test before they can be merged labels Mar 22, 2018
@escopecz escopecz merged commit bb907b0 into mautic:staging Mar 22, 2018
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

@npracht npracht npracht approved these changes

Assignees

@escopecz escopecz

Labels
bug Issues or PR's relating to bugs
Projects
None yet
Milestone
2.13.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Dcoutelle @npracht @javjim @escopecz @dbhurley @mautibot