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

Ensure bug with duplicate periods does not happen #74

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

pjaspers
Copy link
Contributor

Fix: Analytics call ran multiple times for same period

If you had multiple packages, which had formulas which returned a
similar set of periods, the overlapping periods would result in a
double call.

This only happened if you had more than 5 periods (the point at which
we split the analytics call into multiple calls), so even if you had
overlapping ones, if the total was not over 5, it would work.

The reason the test didn't catch this was that all the
package_arguments were set up with the same array of periods, so when
we ran:

    map(&:periods).uniq

It compared the arrays, instead of the elements, and because the
arrays were exactly the same, we had no problems.

I've made the spec a bit more wordy about this, and set up a scenario
where we are triggering this behavior.

I've also sorted the resulting periods, mostly because this reads
nicer when debugging the calls. (And much easier to stub the requests)

If you had multiple packages, which had formulas which returned a
similar set of periods, the overlapping periods would result in a
double call.

This only happened if you had more than 5 periods (the point at which
we split the analytics call into multiple calls), so even if you had
overlapping ones, if the total was not over 5, it would work.

The reason the test didn't catch this was that all the
package_arguments were set up with the same array of periods, so when
we ran:

    map(&:periods).uniq

It compared the arrays, instead of the elements, and because the
arrays were exactly the same, we had no problems.

I've made the spec a bit more wordy about this, and set up a scenario
where we are triggering this behavior.

I've also sorted the resulting periods, mostly because this reads
nicer when debugging the calls. (And much easier to stub the requests)
@pjaspers pjaspers marked this pull request as ready for review November 14, 2020 11:14
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.

1 participant