Skip to content

Add week_number_academic_year and is_week_ytd to OKRTS referrals#3491

Open
anthonygwalters wants to merge 5 commits intomainfrom
claude/optimize-referrals-yoy-Qp158
Open

Add week_number_academic_year and is_week_ytd to OKRTS referrals#3491
anthonygwalters wants to merge 5 commits intomainfrom
claude/optimize-referrals-yoy-Qp158

Conversation

@anthonygwalters
Copy link
Copy Markdown
Member

@anthonygwalters anthonygwalters commented Mar 19, 2026

Pull Request

Summary & Motivation

When merged, this pull request will add two new columns to the OKRTS referrals Tableau extract:

  1. week_number_academic_year — The week number within the academic year, exposed from the underlying int_extracts__student_enrollments_weeks model
  2. is_current_week_mon_sun — A boolean flag indicating the current week (Monday-Sunday)
  3. is_week_ytd — A boolean flag indicating whether a week falls within the year-to-date range for the current academic year

These additions enable more granular time-based filtering and analysis in Tableau dashboards, particularly for tracking referrals by week within the academic year.

Self-review

dbt

  • Updated rpt_tableau__okrts_referrals.yml properties file with new column definitions
  • Updated int_extracts__student_enrollments_weeks.yml to document the week_number_academic_year column
  • No breaking changes — all additions are new columns with no modifications to existing columns

CI checks

  • Trunk — pending
  • dbt Cloud — pending
  • Dagster Cloud — not triggered (no Dagster changes)

https://claude.ai/code/session_015Z6wMSceyyfCNnsdK2nCL9


claude added 2 commits March 19, 2026 19:50
Passes week_number_academic_year from int_powerschool__calendar_week
(already selected in int_extracts__student_enrollments_weeks) through
to rpt_tableau__okrts_referrals, enabling clean YoY BAN calculations
in Tableau by filtering to a consistent academic week cutoff across years.

Also documents the column in int_extracts__student_enrollments_weeks.yml
where it was previously selected but undocumented.

https://claude.ai/code/session_015Z6wMSceyyfCNnsdK2nCL9
…eferrals

Pre-computes YoY as-of-current-week alignment in dbt so Tableau BANs
only need to filter on is_week_ytd = true rather than doing date math
or using a parameter. Also surfaces is_current_week_mon_sun for
flagging the current/latest week row.

https://claude.ai/code/session_015Z6wMSceyyfCNnsdK2nCL9
@anthonygwalters anthonygwalters requested a review from a team as a code owner March 19, 2026 21:20
@anthonygwalters anthonygwalters requested a review from a team March 19, 2026 21:20
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 19, 2026

Claude finished @anthonygwalters's task in 2m 18s —— View job


Code Review

  • Gather context (read CLAUDE.md files)
  • Read changed files
  • Review SQL model changes
  • Review YAML properties changes
  • Post final review

Summary

The changes look solid overall — additive, non-breaking, and correctly documented in the YAML. One column ordering violation to address and a couple of minor observations.


Issues

1. Column ordering violation — rpt_tableau__okrts_referrals.sql:217

co.is_current_week_mon_sun is a plain column reference but is placed after the referral_tier CASE statement and immediately before the window function block. Per the SQL column ordering convention in src/dbt/CLAUDE.md, plain column refs (type 1) must come before simple functions, logicals, CASE statements, and window functions.

-- current placement (after a CASE statement, before window functions) ❌
    end as referral_tier,

    co.is_current_week_mon_sun,

    count(distinct co.student_number) over (...)

It should be moved up into the column enumeration section alongside the other co.* plain refs — ideally grouped with the other week-related columns (week_start_monday, week_end_sunday, week_number_academic_year):

-- correct placement ✓
    co.week_start_monday,
    co.week_end_sunday,
    co.week_number_academic_year,
    co.is_current_week_mon_sun,
    co.date_count as days_in_session,

The is_current_week_mon_sun entry in the YAML properties should then be moved to follow week_number_academic_year as well (to keep YAML order aligned with SQL output order).


Observations

is_week_ytd logic — looks correct

The window function computes the max week_number_academic_year for the current academic year per school (zeroing out prior-year rows via if), then compares each row's week number against that max. This correctly marks prior-year weeks as YTD only if they fall within the same week range as the current year — the right approach for YoY comparisons. Partition by schoolid alone (not schoolid, academic_year) is intentional and works correctly given the if condition.

PR title vs. body mismatch

The PR title says "week_number_academic_year and is_week_ytd" but three columns were added (week_number_academic_year, is_current_week_mon_sun, is_week_ytd). Minor, but worth keeping accurate for the squash merge commit message.

int_extracts__student_enrollments_weeks.yml

The week_number_academic_year addition to this file is appropriate — documents that the column was added to the underlying intermediate model. Other week-related columns (week_start_monday, week_end_sunday, is_current_week_mon_sun) already existed in the model and aren't required in the YAML since this intermediate model doesn't enforce a contract.

Anthony Walters and others added 3 commits March 19, 2026 21:23
Per SQL column ordering convention, plain column refs must precede
window functions and CASE statements. Moves is_current_week_mon_sun
from after referral_tier to the week columns block, and updates the
YAML to match.

https://claude.ai/code/session_015Z6wMSceyyfCNnsdK2nCL9
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.

2 participants