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.

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

Safe join company table #9973

Merged
merged 5 commits into from
May 15, 2021
Merged

Safe join company table #9973

merged 5 commits into from
May 15, 2021

Conversation

snoblucha
Copy link
Contributor

Q A
Branch? "features" for all features, enhancements and bug fixes (until 3.3.0 is released)
Bug fix? yes
New feature? no
Deprecations? no
BC breaks? no
Automated tests included? no
Related user documentation PR URL mautic/mautic-documentation#...
Related developer documentation PR URL mautic/developer-documentation#...
Issue(s) addressed Fixes #9785

Description:

As described in #9785 - when generating reports from Leads with Company data the report will throw 500.

It is called at least twice

  • from docker/data/mautic/app/bundles/LeadBundle/EventListener/ReportSubscriber.php:250
  • and from docker/data/mautic/app/bundles/LeadBundle/EventListener/ReportSubscriber.php:382

This allows this function to be called more than once safely.

Steps to test this PR:

  1. Load up
  2. Create report from Contact dataset and add field from company
  3. report shows without a problem

As described in mautic#9785 - when generating reports from Leads with Company data the report will throw 500. 

It is called at least twice 

- from docker/data/mautic/app/bundles/LeadBundle/EventListener/ReportSubscriber.php:250
- and from docker/data/mautic/app/bundles/LeadBundle/EventListener/ReportSubscriber.php:382

This allows this function to be called more than once safely.
@cla-bot
Copy link

cla-bot bot commented May 6, 2021

Thank you for your contribution! We require all contributors to sign our Contributor License Agreement, and we do not have a record of your signature on file. In order for us to review and merge your code, please head over to https://www.mautic.org/contributor-agreement and complete the form. There may be a short delay while the team add you as a contributor - please be patient :). Any problems contact the Product Team on Slack (get an invite at https://mautic.org/slack). CLA has not been signed by @snoblucha.

@RCheesley
Copy link
Sponsor Member

Thanks for making the PR to fix this @snoblucha - if you can sign the contributors agreement that would be awesome!

@RCheesley RCheesley added bug Issues or PR's relating to bugs reports Anything related to reports T1 Low difficulty to fix (issue) or test (PR) ready-to-test PR's that are ready to test labels May 6, 2021
@RCheesley RCheesley added this to the 4.0-rc milestone May 6, 2021
@stevedrobinson
Copy link
Contributor

Tested on a 3.3 instance and it works. Thank you, @snoblucha!

@RCheesley
Copy link
Sponsor Member

@cla-bot check

@RCheesley RCheesley closed this May 7, 2021
@cla-bot cla-bot bot added the cla-signed The PR contributors have signed the contributors agreement label May 7, 2021
@cla-bot
Copy link

cla-bot bot commented May 7, 2021

The CLA Bot has been sent on a mission to check against the latest list and will be back shortly with its findings!

@RCheesley RCheesley reopened this May 7, 2021
@codecov
Copy link

codecov bot commented May 7, 2021

Codecov Report

Merging #9973 (d791996) into features (36c4fc7) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##             features    #9973   +/-   ##
===========================================
  Coverage       41.19%   41.19%           
- Complexity      34555    34556    +1     
===========================================
  Files            2060     2060           
  Lines          111478   111480    +2     
===========================================
+ Hits            45921    45927    +6     
+ Misses          65557    65553    -4     
Impacted Files Coverage Δ Complexity Δ
...undles/ReportBundle/Event/ReportGeneratorEvent.php 66.43% <100.00%> (+3.31%) 61.00 <0.00> (+1.00)

@RCheesley
Copy link
Sponsor Member

RCheesley commented May 7, 2021

@snoblucha could you take care of the code style issue please?

We will also need to take care of the test coverage as this PR will result in decreased coverage. Are you able to take a look at the relevant tests and update/improve them?

Copy link
Member

@kuzmany kuzmany left a comment

Choose a reason for hiding this comment

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

Meet same issue and this PR fixed it 👍

@kuzmany kuzmany added pending-test-confirmation PR's that require one test before they can be merged and removed ready-to-test PR's that are ready to test labels May 12, 2021
@npracht
Copy link
Member

npracht commented May 12, 2021

@all-contributors please add @kuzmany for userTesting

@allcontributors
Copy link
Contributor

@npracht

I've put up a pull request to add @kuzmany! 🎉

@npracht
Copy link
Member

npracht commented May 12, 2021

@all-contributors please add @kuzmany for review

@allcontributors
Copy link
Contributor

@npracht

I've updated the pull request to add @kuzmany! 🎉

Copy link
Sponsor Member

@RCheesley RCheesley left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @snoblucha - was able to reproduce the issue and the PR has addressed it! 🎉

LGTM 🚀

@RCheesley
Copy link
Sponsor Member

@all-contributors please add @stevedrobinson for userTesting

@allcontributors
Copy link
Contributor

@RCheesley

I've put up a pull request to add @stevedrobinson! 🎉

@RCheesley
Copy link
Sponsor Member

@all-contributors please add @snoblucha for code and bug

@allcontributors
Copy link
Contributor

@RCheesley

I've put up a pull request to add @snoblucha! 🎉

@RCheesley RCheesley merged commit 94191f6 into mautic:features May 15, 2021
ts-navghane added a commit to ts-navghane/mautic that referenced this pull request May 17, 2021
…riteria-ab-tests

* 'features' of github.com:mautic/mautic: (269 commits)
  Add background-position and background-image to section content and wrapper (mautic#7211)
  Fix API set multiselect empty value (mautic#9506)
  docs: add hluchas as a contributor (mautic#10039)
  Sync leaking memory (mautic#9299)
  docs: add incentfit as a contributor (mautic#10038)
  docs: add gabepri as a contributor (mautic#10037)
  form actions that register gotowebinar registrants now save join urls (mautic#9477)
  docs: add kuzmany as a contributor (mautic#10036)
  Add readme for Tag Manager repo (mautic#10035)
  Safe join company table (mautic#9973)
  docs: add fedys as a contributor (mautic#10034)
  docs: add anton-vlasenko as a contributor (mautic#10033)
  docs: add rcheesley as a contributor (mautic#10032)
  docs: add luguenth as a contributor (mautic#10031)
  docs: add snoblucha as a contributor (mautic#10030)
  docs: add stevedrobinson as a contributor (mautic#10029)
  Add stage name and stage date added to contacts report (mautic#8173)
  Fix DNC report channel link (mautic#10010)
  Revert the incorrect changes from previous commits.
  Fix PR comments.
  ...
@RCheesley RCheesley modified the milestones: 4.0-rc, 4.0-beta May 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues or PR's relating to bugs cla-signed The PR contributors have signed the contributors agreement pending-test-confirmation PR's that require one test before they can be merged reports Anything related to reports T1 Low difficulty to fix (issue) or test (PR)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using Company ID in a report throws a 500 error
5 participants