8000 Fix group by If you use count columns for assets download 2 by kuzmany · Pull Request #10833 · mautic/mautic · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix group by If you use count columns for assets download 2 #10833

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

Conversation

kuzmany
Copy link
Member
@kuzmany kuzmany commented Feb 4, 2022
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/mautic-documentation#...
Related developer documentation PR URL mautic/developer-documentation#...
Issue(s) addressed Fixes #...

Description:

This is regression issue after merge #10693

Steps to test this PR:

  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)
  2. Create assets: test 1 and test 2
  3. Open download link for test 1 in incognito window
  4. Close incognito window and do the same for asset test 1 again
  5. Then close incognito window and open download link for test 2 in incognito window
  6. Now you should have 2 downloads for test 1 and 1 download for test 2
  7. Create report for assets and choose alias and contact ID as columns
  8. You should see 3 rows in report
  9. Then add download count and unique download count
  10. You should see just two lines in report - what is wrong, because this is report of assets downloads.
  11. After PR you should see 3 rows in report
  12. If you want report by assets, then you can add group by by Asset ID and you should see two lines, what is correct

@kuzmany kuzmany added bug Issues or PR's relating to bugs ready-to-test PR's that are ready to test regression A bug that broke something in the last release labels Feb 4, 2022
@kuzmany kuzmany added this to the 4.2 milestone Feb 4, 2022
@cla-bot cla-bot bot added the cla-signed The PR contributors have signed the contributors agreement label Feb 4, 2022
@kuzmany kuzmany mentioned this pull request Feb 4, 2022
39 tasks
@codecov
Copy link
codecov bot commented Feb 4, 2022

Codecov Report

Merging #10833 (7b17630) into 4.x (b869901) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##                4.x   #10833   +/-   ##
=========================================
  Coverage     47.56%   47.56%           
  Complexity    35064    35064           
=========================================
  Files          2104     2104           
  Lines        117810   117810           
=========================================
  Hits          56040    56040           
  Misses        61770    61770           
Impacted Files Coverage Δ
...les/AssetBundle/EventListener/ReportSubscriber.php 59.20% <100.00%> (ø)

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 nice to have tests that would ensure it works correctly, but let's get this merged and fix the regression

@escopecz escopecz 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 Feb 11, 2022
@kuzmany kuzmany merged commit 01944ae into mautic:4.x Feb 15, 2022
@kuzmany kuzmany deleted the fix-Fix_group_by_If_you_use_count_columns_for_assets_download branch February 15, 2022 14:41
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 regression A bug that broke something in the last release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0