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

🤖 backported "Keep collection_id of dashboard subscriptions in sync with same field on dashboard" #17586

Merged
merged 1 commit into from
Aug 25, 2021

Conversation

github-actions[bot]
Copy link

@noahmoss
Copy link
Member

@flamber this is now merged to master -- if you have time to try it out soon and determine whether it's safe to include in the 40.3 release, that would be great :)

@codecov
Copy link

codecov bot commented Aug 24, 2021

Codecov Report

Merging #17586 (55c4ca9) into release-x.40.x (ca559f4) will increase coverage by 0.06%.
The diff coverage is 88.88%.

Impacted file tree graph

@@                Coverage Diff                 @@
##           release-x.40.x   #17586      +/-   ##
==================================================
+ Coverage           85.01%   85.07%   +0.06%     
==================================================
  Files                 416      416              
  Lines               33291    33317      +26     
  Branches             2377     2379       +2     
==================================================
+ Hits                28301    28346      +45     
+ Misses               2613     2592      -21     
- Partials             2377     2379       +2     
Impacted Files Coverage Δ
src/metabase/cmd.clj 42.57% <50.00%> (+13.86%) ⬆️
src/metabase/models/pulse.clj 87.41% <73.68%> (-0.70%) ⬇️
...end/src/metabase_enterprise/serialization/load.clj 85.07% <87.50%> (+0.07%) ⬆️
...kend/src/metabase_enterprise/serialization/cmd.clj 84.61% <90.00%> (+6.39%) ⬆️
...d/src/metabase_enterprise/serialization/upsert.clj 86.74% <100.00%> (+2.65%) ⬆️
src/metabase/api/field.clj 84.18% <100.00%> (+0.54%) ⬆️
src/metabase/models/card.clj 94.59% <100.00%> (ø)
src/metabase/models/dashboard.clj 82.12% <100.00%> (+0.15%) ⬆️
src/metabase/models/metric.clj 90.76% <100.00%> (+0.44%) ⬆️
src/metabase/models/permissions_group.clj 81.81% <100.00%> (-0.73%) ⬇️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f052ce...55c4ca9. Read the comment docs.

Copy link
Contributor

@flamber flamber left a comment

Choose a reason for hiding this comment

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

This works as expected for all three appdb types. I would probably just have used the H2 SQL for all, since it's the lowest denominator and works for others too.

@noahmoss
Copy link
Member

@flamber Thanks for reviewing!

I originally had the nested query for all DBs, but Cam was concerned about its efficiency if there are a large number of pulses.

@noahmoss noahmoss merged commit 4ae78f0 into release-x.40.x Aug 25, 2021
@noahmoss noahmoss deleted the backport-pulse-collection-id branch August 25, 2021 15:49
@rlotun rlotun added this to the 0.40.3 milestone Aug 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants