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.

Sign up for GitHub

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

Jump to bottom

512 Added max-recency boundries #526

Merged

aaryansinha16 merged 11 commits into main from 512-align-data-recency

Oct 5, 2024

Contributor

aaryansinha16 commented Sep 16, 2024

Added the data limit for maximum recency allowed by each tier

aaryansinha16 requested a review from trixobird

September 16, 2024 13:15

github-actions bot added release server web labels

trixobird reviewed

View reviewed changes

Collaborator

trixobird left a comment

this should work, let's fix the comments. You also have some eslint errors. You probably need to configure vscode to show them. Mine for example are

packages/channel-utils/src/date-utils.ts Outdated Show resolved Hide resolved

packages/mappings/src/tier-mapping.ts Outdated Show resolved Hide resolved

packages/channel-utils/src/date-utils.ts Outdated Show resolved Hide resolved

aaryansinha16 force-pushed the 512-align-data-recency branch from bd9c67a to b2e39ac Compare

September 19, 2024 06:58

aaryansinha16 commented

View reviewed changes

packages/channel-utils/src/date-utils.ts Outdated Show resolved Hide resolved

aaryansinha16 force-pushed the 512-align-data-recency branch 3 times, most recently from 3e764b7 to 1dfbc65 Compare

September 19, 2024 18:29

trixobird reviewed

View reviewed changes

Collaborator

trixobird left a comment

let's create some tests for the timeRanges. In order not o mess with db calls let's create another function that start after the db call.

Let me know if you want to talk about it.

packages/channel-utils/src/date-utils.ts Show resolved Hide resolved

packages/channel-utils/src/date-utils.ts Outdated Show resolved Hide resolved

aaryansinha16 force-pushed the 512-align-data-recency branch 2 times, most recently from 54864ff to 486645c Compare

September 23, 2024 06:19

trixobird reviewed

View reviewed changes

packages/channel-utils/src/date-utils.ts Outdated

Comment on lines 21 to 42

+                const organizations = latestInsight?.adAccount?.organizations ?? [];
+                const organizationWithHighestTier =
+                  organizations.length > 0
+                    ? organizations.sort((a, b) => tierConstraints[b.tier].order - tierConstraints[a.tier].order)[0].tier
+                    : undefined;
+                const maxRecency = tierConstraints[organizationWithHighestTier ?? Tier.Launch].maxRecency;
+                if (initial) {
+                  const range = getLastXMonths();
+                  if (!organizationWithHighestTier) {
+                    return splitTimeRange(tierConstraints[Tier.Launch].maxRecency, range.since, range.until);
+                  }
+                  return splitTimeRange(maxRecency, range.since, range.until);
+                }
+                const furthestDate = addInterval(new Date(), 'day', -maxRecency);
+                const insightDateWithinLimit = latestInsight && latestInsight.date > furthestDate ? latestInsight.date : furthestDate;
+                const range = getDayPriorTillTomorrow(insightDateWithinLimit);
+                return splitTimeRange(maxRecency, range.since, range.until);

Collaborator

trixobird Sep 27, 2024

Suggested change

      
              const organizations = latestInsight?.adAccount?.organizations ?? [];
          
              const organizationWithHighestTier =
          
                organizations.length > 0
          
                  ? organizations.sort((a, b) => tierConstraints[b.tier].order - tierConstraints[a.tier].order)[0].tier
          
                  : undefined;
          
              const maxRecency = tierConstraints[organizationWithHighestTier ?? Tier.Launch].maxRecency;
          
              if (initial) {
          
                const range = getLastXMonths();
          
                if (!organizationWithHighestTier) {
          
                  return splitTimeRange(tierConstraints[Tier.Launch].maxRecency, range.since, range.until);
          
                }
          
                return splitTimeRange(maxRecency, range.since, range.until);
          
              }
          
              const furthestDate = addInterval(new Date(), 'day', -maxRecency);
          
              const insightDateWithinLimit = latestInsight && latestInsight.date > furthestDate ? latestInsight.date : furthestDate;
          
              const range = getDayPriorTillTomorrow(insightDateWithinLimit);
          
              return splitTimeRange(maxRecency, range.since, range.until);
          
              return testTimeRange(initial, latestInsight);

I am a bit confused. You have created a testTimeRange (probably a name that we need to change) function that you test, but the product code does not use it.

packages/channel-utils/test/date-util.test.ts Outdated Show resolved Hide resolved

packages/channel-utils/test/date-util.test.ts Outdated

+                  };
+                  const ranges = testTimeRange(false, dummyInsight);
+                  assert.strictEqual(ranges.length, 2);

Collaborator

trixobird Sep 27, 2024

it seems that these tests will fail as time passes. This should help you.

packages/channel-utils/test/date-util.test.ts Outdated

@@ @@ -57,4 +94,31 @@ void describe('splitTimeRange tests', () => { @@
                   assert.strictEqual(ranges[2].since.toISOString(), '2021-06-30T00:00:00.000Z');
                   assert.strictEqual(ranges[2].until.toISOString(), '2021-09-25T00:00:00.000Z');
                 });
+                void it('returns split time range when passed with false initial', () => {

Collaborator

trixobird Sep 27, 2024

Could we have a test where the last insight was 3 months back and the tier is Launch?

aaryansinha16 force-pushed the 512-align-data-recency branch 6 times, most recently from fd09815 to 57df657 Compare

October 4, 2024 08:28

trixobird reviewed

View reviewed changes

packages/channel-utils/test/date-util.test.ts Show resolved Hide resolved

packages/channel-utils/test/date-util.test.ts Outdated

+                  assert.strictEqual(ranges.length, 26);
+                  assert.strictEqual(ranges[0].since.toISOString(), '2024-01-01T00:00:00.000Z');
+                  assert.strictEqual(ranges[0].until.toISOString(), '2024-01-07T00:00:00.000Z');
+                  assert.strictEqual(ranges[1].since.toISOString(), '2024-01-08T00:00:00.000Z');

Collaborator

trixobird Oct 4, 2024

it is find not to check all 26 ranges, but please check the first and the last at least.

packages/channel-utils/test/date-util.test.ts Show resolved Hide resolved

trixobird reviewed

View reviewed changes

packages/channel-utils/test/date-util.test.ts

+                  assert.strictEqual(ranges[0].until.toISOString(), '2024-06-25T18:30:00.000Z');
+                });
+                void it('returns correct time range when last insight was 3 months ago and tier is Launch', (context) => {

Collaborator

trixobird Oct 4, 2024 •

edited

Loading

these last two test should produce the same result. I would expect one range from midnight 18 till 26 midnight. This means that the existing code has a bug.

aaryansinha16 force-pushed the 512-align-data-recency branch from 0fa1648 to b6d4ab5 Compare

October 5, 2024 07:00

aaryansinha16 added 7 commits

October 5, 2024 15:24


          Added max-recency boundries

6a7c98e


          getting the highest tier org/refactoring

d90e775


          updated/fixed tests

15622c4


          prettier fix

e1cb599


          updated saveAccounts query

1aba797


          added furthest date conditoin

9903cca


          added tests for time-range function

cb78287

aaryansinha16 added 2 commits

October 5, 2024 15:24


          test updates

04c25ad


          test-updates

f13697c

aaryansinha16 force-pushed the 512-align-data-recency branch from b6d4ab5 to f13697c Compare

October 5, 2024 09:54

aaryansinha16 added 2 commits

October 5, 2024 15:25


          updated splitTimeRange function

3a1cfb2


          timeRangeHelper function fixed

5f01d2b

trixobird approved these changes

View reviewed changes

aaryansinha16 merged commit 13a5b91 into main

4 checks passed

aaryansinha16 deleted the 512-align-data-recency branch

October 5, 2024 12:55

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release server web