Skip to content

add refund rate metric [sample PR for demo]#91

Open
mrMakaronka wants to merge 1 commit intotyoma/mock-de-journeyfrom
tyoma/add-refund-rate-metric
Open

add refund rate metric [sample PR for demo]#91
mrMakaronka wants to merge 1 commit intotyoma/mock-de-journeyfrom
tyoma/add-refund-rate-metric

Conversation

@mrMakaronka
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a MetricFlow semantic layer + metrics for Shopify orders (including a new refund_rate ratio metric) in the mock-cli/test-shopify dbt project, along with local Databao fixture/config updates.

Changes:

  • Added staging models for orders and a MetricFlow time spine.
  • Added an orders semantic model plus order_count, refunded_order_count, and refund_rate metrics.
  • Added Databao fixture files (test question + config), and also committed dbt target/ artifacts.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
mock-cli/test-shopify/models/staging/stg_orders.sql Adds a staging model referencing the orders source.
mock-cli/test-shopify/models/staging/metricflow_time_spine.sql Adds a time spine table used by MetricFlow queries.
mock-cli/test-shopify/models/semantic_models/orders.yml Defines orders semantic model (entities/dimensions/measures).
mock-cli/test-shopify/models/metrics/orders_metrics.yml Defines refund_rate ratio metric and supporting metrics.
mock-cli/test-shopify/.databao/test_questions.csv Adds a coverage question for refund_rate using mf query.
mock-cli/test-shopify/.databao/databao.yml Adds Databao project/connection/user config (currently contains local paths + token).
mock-cli/test-shopify/.user.yml Adds a per-user identifier file.
mock-cli/test-shopify/target/run/shopify_analytics/models/staging/stg_orders.sql dbt run artifact for the staging model.
mock-cli/test-shopify/target/run/shopify_analytics/models/staging/metricflow_time_spine.sql dbt run artifact for the time spine.
mock-cli/test-shopify/target/compiled/shopify_analytics/models/staging/stg_orders.sql dbt compiled artifact for the staging model.
mock-cli/test-shopify/target/compiled/shopify_analytics/models/staging/metricflow_time_spine.sql dbt compiled artifact for the time spine.

Comment on lines +3 to +5
path: /Users/Artem.Trofimov/Work/databao-cli/mock-cli/test-shopify
databao:
path: /Users/Artem.Trofimov/Work/databao-cli/mock-cli/test-shopify/.databao
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

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

This file appears to contain local, machine-specific absolute paths (e.g., under /Users/...) which will break for other environments and can leak developer workstation details. Consider storing paths relative to the dbt project root, or using env vars/placeholders, and avoid committing the generated per-user config to the repo.

Suggested change
path: /Users/Artem.Trofimov/Work/databao-cli/mock-cli/test-shopify
databao:
path: /Users/Artem.Trofimov/Work/databao-cli/mock-cli/test-shopify/.databao
path: .
databao:
path: .databao

Copilot uses AI. Check for mistakes.
Comment on lines +11 to +13
user:
email: artem@databao.app
token: mock_oauth_token_a1b2c3d4
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

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

Committing user credentials (email/token), even if "mock", is risky and trains insecure patterns; it can also be accidentally treated as real by tooling. Recommend removing the user block from the committed file and loading credentials from environment variables or a local-only config that is gitignored.

Suggested change
user:
email: artem@databao.app
token: mock_oauth_token_a1b2c3d4

Copilot uses AI. Check for mistakes.
@@ -0,0 +1 @@
id: 7e32390b-2360-4c50-993c-408e4855247f
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

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

.user.yml looks like a per-user/per-install identifier and is likely a generated local artifact. Consider not committing it (and adding it to ignore rules) to avoid sharing identifiers across developers/CI runs.

Suggested change
id: 7e32390b-2360-4c50-993c-408e4855247f
# Local per-user identifier intentionally omitted from source control.

Copilot uses AI. Check for mistakes.
Comment on lines +1 to +5


create view "shopify"."main"."stg_orders__dbt_tmp" as (
select * from "shopify"."main"."orders"
);
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

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

target/run is dbt build output (includes __dbt_tmp objects) and is usually not committed because it is environment-specific and causes noisy diffs. Recommend removing this file from the PR and ignoring mock-cli/test-shopify/target/ (or **/target/) in version control.

Suggested change
create view "shopify"."main"."stg_orders__dbt_tmp" as (
select * from "shopify"."main"."orders"
);

Copilot uses AI. Check for mistakes.
Comment on lines +1 to +21





create table
"shopify"."main"."metricflow_time_spine__dbt_tmp"

as (


select
cast(range as date) as date_day
from generate_series(
cast('2020-01-01' as date),
cast('2030-01-01' as date),
interval '1 day'
) as t(range)
);


No newline at end of file
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

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

target/run is dbt build output and is generally not committed (it is regenerated and environment-specific). Recommend removing this artifact from the PR and ignoring the target/ directory to keep the repo clean and diffs stable.

Suggested change
create table
"shopify"."main"."metricflow_time_spine__dbt_tmp"
as (
select
cast(range as date) as date_day
from generate_series(
cast('2020-01-01' as date),
cast('2030-01-01' as date),
interval '1 day'
) as t(range)
);
-- Generated dbt build artifact removed from version-controlled source.
-- This file under target/run should be regenerated by dbt and not committed.

Copilot uses AI. Check for mistakes.
@@ -0,0 +1 @@
select * from "shopify"."main"."orders" No newline at end of file
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

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

target/compiled is generated dbt output and is usually not committed since it is derived from sources and can differ by adapter/config. Recommend removing this file from the PR and ignoring the target/ directory.

Suggested change
select * from "shopify"."main"."orders"

Copilot uses AI. Check for mistakes.
Comment on lines +1 to +9


select
cast(range as date) as date_day
from generate_series(
cast('2020-01-01' as date),
cast('2030-01-01' as date),
interval '1 day'
) as t(range) No newline at end of file
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

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

target/compiled is generated dbt output and is typically excluded from version control. Recommend removing this artifact from the PR and ignoring target/ to avoid churn.

Suggested change
select
cast(range as date) as date_day
from generate_series(
cast('2020-01-01' as date),
cast('2030-01-01' as date),
interval '1 day'
) as t(range)

Copilot uses AI. Check for mistakes.
agg: count
expr: id
- name: refunded_order_count
description: "Number of orders with financial_status = 'refunded'"
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

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

The refunded_order_count measure is described as refunded-only, but it currently counts id without any refund filter; the refund condition is applied later at the metric level. This is misleading and can cause incorrect reuse of the measure—consider either (a) renaming it to a generic count measure and reuse order_count with a metric filter, or (b) updating the measure description/name so it matches its actual semantics.

Suggested change
description: "Number of orders with financial_status = 'refunded'"
description: "Unfiltered count of orders; refunded-only semantics must be applied downstream via a metric-level filter on financial_status = 'refunded'"

Copilot uses AI. Check for mistakes.
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