Commit 96ada461 authored by Nicolas Dular's avatar Nicolas Dular

Fix crashing in-product-marketing email worker

This is a temporary fix for the in-product-marketing-email worker.
It is a bug that consists of multiple layers. We expect to only have
root namespaces in `onboarding_progresses`. However, groups can be moved
to sub-groups. Threfore we also have sub-groups that would receieve the
emails.

This alone would not be an issue. But we need to call `.root_ancestor`
for these sub-groups and it doesn't play well together with
`each_batches` when there is an other JOIN condition on the namespace.
It currently generates a non-working query that fails.

To prevent this for now we do not send emails to sub-groups as a
temporary solution. In the long run, we need to remove all sub-groups
fron `onboarding_progresses` and fix the issue of calling
`.root_ancestor` in an `each_batches` call.
parent 4ee0c13f
...@@ -63,7 +63,10 @@ module Namespaces ...@@ -63,7 +63,10 @@ module Namespaces
.completed_actions_with_latest_in_range(completed_actions, range) .completed_actions_with_latest_in_range(completed_actions, range)
.incomplete_actions(incomplete_action) .incomplete_actions(incomplete_action)
Group.joins(:onboarding_progress).merge(onboarding_progress_scope) # Filtering out sub-groups is a temporary fix to prevent calling
# `.root_ancestor` on groups that are not root groups.
# See https://gitlab.com/groups/gitlab-org/-/epics/5594 for more information.
Group.where(parent_id: nil).joins(:onboarding_progress).merge(onboarding_progress_scope)
end end
def users_for_group(group) def users_for_group(group)
......
...@@ -159,4 +159,20 @@ RSpec.describe Namespaces::InProductMarketingEmailsService, '#execute' do ...@@ -159,4 +159,20 @@ RSpec.describe Namespaces::InProductMarketingEmailsService, '#execute' do
it { expect { subject }.to raise_error(NotImplementedError, 'No ability defined for track foo') } it { expect { subject }.to raise_error(NotImplementedError, 'No ability defined for track foo') }
end end
context 'when group is a sub-group' do
let(:root_group) { create(:group) }
let(:group) { create(:group) }
before do
group.parent = root_group
group.save!
allow(Ability).to receive(:allowed?).and_call_original
end
it 'does not raise an exception' do
expect { execute_service }.not_to raise_error
end
end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment