diff --git a/.github/workflows/preview-deploy.yml b/.github/workflows/preview-deploy.yml index 63ea9590d..62d2266af 100644 --- a/.github/workflows/preview-deploy.yml +++ b/.github/workflows/preview-deploy.yml @@ -25,6 +25,7 @@ jobs: head_sha: ${{ steps.preview.outputs.head_sha }} is_fork: ${{ steps.preview.outputs.is_fork }} pr_number: ${{ steps.preview.outputs.pr_number }} + resolution_source: ${{ steps.preview.outputs.resolution_source }} should_deploy: ${{ steps.preview.outputs.should_deploy }} steps: @@ -105,6 +106,7 @@ jobs: HEAD_SHA: ${{ needs.preview-gate.outputs.head_sha }} IS_FORK: ${{ needs.preview-gate.outputs.is_fork }} PR_NUMBER: ${{ needs.preview-gate.outputs.pr_number }} + RESOLUTION_SOURCE: ${{ needs.preview-gate.outputs.resolution_source }} steps: - name: Checkout trusted preview tooling @@ -202,6 +204,9 @@ jobs: cp trusted/workers/preview/tsconfig.json "$preview_dir/tsconfig.json" cp trusted/workers/preview/wrangler.toml "$preview_dir/wrangler.toml" cp -R trusted/workers/preview/src "$preview_dir/src" + mkdir -p "$preview_dir/deploy" + cp trusted/workers/preview/deploy/redact_preview_log.sh "$preview_dir/deploy/redact_preview_log.sh" + chmod 0755 "$preview_dir/deploy/redact_preview_log.sh" diagnostics_nonce="$(openssl rand -hex 32)" sed -i "s/\${PR_NUMBER}/${PR_NUMBER}/g" "$preview_dir/wrangler.toml" @@ -245,32 +250,54 @@ jobs: cd "$RUNNER_TEMP/sure-preview-worker" config_path="$RUNNER_TEMP/sure-preview-worker/wrangler.toml" image_tag="sure-preview-pr-${PR_NUMBER}:${HEAD_SHA}" + temporary_image_ref="registry.cloudflare.com/${CLOUDFLARE_ACCOUNT_ID}/${image_tag}" push_log="$RUNNER_TEMP/wrangler-containers-push.log" clean_log="$RUNNER_TEMP/wrangler-containers-push.clean.log" + push_status=0 # wrangler containers push validates wrangler.toml, so point the trusted - # config at the loaded CI image before replacing it with the registry ref. - LOCAL_IMAGE_TAG="$image_tag" node - "$config_path" <<'NODE' + # config at a registry-shaped ref while it pushes the verified local image. + TEMPORARY_IMAGE_REF="$temporary_image_ref" node - "$config_path" <<'NODE' const fs = require('node:fs'); const configPath = process.argv[2]; - const imageTag = process.env.LOCAL_IMAGE_TAG; + const imageRef = process.env.TEMPORARY_IMAGE_REF; - if (!/^sure-preview-pr-[1-9][0-9]*:[a-f0-9]{40}$/.test(imageTag || '')) { - throw new Error('Expected local preview image tag for wrangler containers push'); + if (!/^registry\.cloudflare\.com\/[A-Za-z0-9_-]+\/sure-preview-pr-[1-9][0-9]*:[a-f0-9]{40}$/.test(imageRef || '')) { + throw new Error('Expected registry-shaped preview image ref before wrangler containers push'); } const original = fs.readFileSync(configPath, 'utf8'); - const updated = original.replace(/image = "[^"]+"/, `image = ${JSON.stringify(imageTag)}`); + const updated = original.replace(/image = "[^"]+"/, `image = ${JSON.stringify(imageRef)}`); if (updated === original) { throw new Error('Expected wrangler.toml to contain an image entry to rewrite before push'); } fs.writeFileSync(configPath, updated); NODE - ./node_modules/.bin/wrangler containers push "$image_tag" 2>&1 | tee "$push_log" + set +e + ./node_modules/.bin/wrangler containers push "$image_tag" 2>&1 | tee "$push_log" | ./deploy/redact_preview_log.sh + push_status=${PIPESTATUS[0]} + set -e perl -pe 's/\e\[[0-9;]*[A-Za-z]//g' "$push_log" > "$clean_log" - image_ref=$(grep -Eo 'registry\.cloudflare\.com/[^[:space:]]+' "$clean_log" | tail -n 1 | tr -d '\r') + + if [ "$push_status" -ne 0 ]; then + exit "$push_status" + fi + + image_ref="$(node - "$clean_log" <<'NODE' + const fs = require('node:fs'); + + const logPath = process.argv[2]; + const log = fs.readFileSync(logPath, 'utf8'); + const expectedSuffix = `sure-preview-pr-${process.env.PR_NUMBER}:${process.env.HEAD_SHA}`; + const pattern = /registry\.cloudflare\.com\/[A-Za-z0-9_-]+\/sure-preview-pr-[1-9][0-9]*:[a-f0-9]{40}/g; + const matches = [...log.matchAll(pattern)].map((match) => match[0]); + const imageRef = matches.findLast((candidate) => candidate.endsWith(`/${expectedSuffix}`)); + + if (imageRef) process.stdout.write(imageRef); + NODE + )" if [ -z "$image_ref" ]; then echo "Could not find Cloudflare registry image reference in wrangler output" >&2 @@ -292,11 +319,16 @@ jobs: const configPath = process.argv[2]; const imageRef = process.env.IMAGE_REF; + const expectedSuffix = `sure-preview-pr-${process.env.PR_NUMBER}:${process.env.HEAD_SHA}`; - if (!imageRef || !imageRef.startsWith('registry.cloudflare.com/')) { + if (!/^registry\.cloudflare\.com\/[A-Za-z0-9_-]+\/sure-preview-pr-[1-9][0-9]*:[a-f0-9]{40}$/.test(imageRef || '')) { throw new Error('Expected a Cloudflare registry image reference'); } + if (!imageRef.endsWith(`/${expectedSuffix}`)) { + throw new Error('Cloudflare registry image reference does not match this preview artifact'); + } + const original = fs.readFileSync(configPath, 'utf8'); const updated = original.replace(/image = "[^"]+"/, `image = ${JSON.stringify(imageRef)}`); if (updated === original) { @@ -305,7 +337,10 @@ jobs: fs.writeFileSync(configPath, updated); NODE - cat "$config_path" + # Print a redacted copy for logs without mutating the config used by deploy. + redacted_config="$RUNNER_TEMP/wrangler-redacted.toml" + "$RUNNER_TEMP/sure-preview-worker/deploy/redact_preview_log.sh" < "$config_path" > "$redacted_config" + cat "$redacted_config" - name: Deploy to Cloudflare Containers id: deploy @@ -321,7 +356,11 @@ jobs: clean_deploy_log="$RUNNER_TEMP/wrangler-deploy.clean.log" deploy_once() { - ./node_modules/.bin/wrangler deploy --config wrangler.toml --var "PR_NUMBER:${PR_NUMBER}" 2>&1 | tee "$deploy_log" + set +e + ./node_modules/.bin/wrangler deploy --config wrangler.toml --var "PR_NUMBER:${PR_NUMBER}" 2>&1 | tee "$deploy_log" | ./deploy/redact_preview_log.sh + local deploy_status=${PIPESTATUS[0]} + set -e + return "$deploy_status" } if ! deploy_once; then @@ -406,12 +445,90 @@ jobs: if-no-files-found: error retention-days: 3 + - name: Collect preview failure diagnostics + if: failure() + run: | + set -euo pipefail + + diagnostics_dir="$RUNNER_TEMP/preview-failure-diagnostics" + manifest_file="$RUNNER_TEMP/preview-image/sure-preview-image.manifest.json" + redaction_helper="$RUNNER_TEMP/sure-preview-worker/deploy/redact_preview_log.sh" + mkdir -p "$diagnostics_dir" + + jq -n \ + --arg artifactName "$ARTIFACT_NAME" \ + --arg headSha "$HEAD_SHA" \ + --arg isFork "$IS_FORK" \ + --arg prNumber "$PR_NUMBER" \ + --arg resolutionSource "$RESOLUTION_SOURCE" \ + '{ + artifactName: $artifactName, + headSha: $headSha, + isFork: $isFork, + prNumber: $prNumber, + resolutionSource: $resolutionSource + }' > "$diagnostics_dir/preview-request.json" + + sanitize_copy() { + local source="$1" + local destination="$2" + if [ -f "$source" ]; then + if [ -x "$redaction_helper" ]; then + "$redaction_helper" < "$source" > "$destination" + else + cp "$source" "$destination" + fi + fi + } + + if [ -f "$manifest_file" ]; then + jq '{ + artifactVersion, + archivePath, + archiveSha256, + headSha, + imageId, + imageTag, + prNumber + }' "$manifest_file" > "$diagnostics_dir/preview-image-manifest.json" + fi + + sanitize_copy "$RUNNER_TEMP/sure-preview-worker/wrangler.toml" "$diagnostics_dir/wrangler.toml" + sanitize_copy "$RUNNER_TEMP/wrangler-containers-push.clean.log" "$diagnostics_dir/wrangler-containers-push.log" + if [ -f "$RUNNER_TEMP/wrangler-deploy.clean.log" ]; then + sanitize_copy "$RUNNER_TEMP/wrangler-deploy.clean.log" "$diagnostics_dir/wrangler-deploy.log" + else + sanitize_copy "$RUNNER_TEMP/wrangler-deploy.log" "$diagnostics_dir/wrangler-deploy.log" + fi + + find "$diagnostics_dir" -maxdepth 1 -type f -print + + - name: Upload preview failure diagnostics + if: failure() + uses: actions/upload-artifact@b7c566a772e6b6bfb58ed0dc250532a479d7789f # v6 + with: + name: preview-failure-diagnostics-pr-${{ env.PR_NUMBER }}-${{ env.HEAD_SHA }} + path: ${{ runner.temp }}/preview-failure-diagnostics + if-no-files-found: error + retention-days: 3 + + - name: Prepare cleanup metadata + if: success() + run: | + set -euo pipefail + + metadata_dir="$RUNNER_TEMP/preview-cleanup-metadata" + mkdir -p "$metadata_dir" + "$RUNNER_TEMP/sure-preview-worker/deploy/redact_preview_log.sh" \ + < "$RUNNER_TEMP/sure-preview-worker/wrangler.toml" \ + > "$metadata_dir/wrangler.toml" + - name: Store cleanup metadata if: success() uses: actions/upload-artifact@b7c566a772e6b6bfb58ed0dc250532a479d7789f # v6 with: name: preview-cleanup-pr-${{ env.PR_NUMBER }} - path: ${{ runner.temp }}/sure-preview-worker/wrangler.toml + path: ${{ runner.temp }}/preview-cleanup-metadata/wrangler.toml retention-days: 2 deployment_status: diff --git a/.rubocop.yml b/.rubocop.yml index 1d2341687..33542a43c 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -12,4 +12,8 @@ Layout/IndentationConsistency: Enabled: true Layout/SpaceInsidePercentLiteralDelimiters: - Enabled: true \ No newline at end of file + Enabled: true + +Layout/SpaceInsideArrayLiteralBrackets: + Exclude: + - "db/schema.rb" diff --git a/app/assets/images/google-icon.svg b/app/assets/images/google-icon.svg new file mode 100644 index 000000000..31f21e61d --- /dev/null +++ b/app/assets/images/google-icon.svg @@ -0,0 +1,6 @@ + diff --git a/app/assets/tailwind/application.css b/app/assets/tailwind/application.css index ddbda9b04..035b80946 100644 --- a/app/assets/tailwind/application.css +++ b/app/assets/tailwind/application.css @@ -10,7 +10,6 @@ @import "./simonweb_pickr.css"; -@import "./google-sign-in.css"; @import "./date-picker-dark-mode.css"; @import "./print-report.css"; @import "./privacy-mode.css"; diff --git a/app/assets/tailwind/google-sign-in.css b/app/assets/tailwind/google-sign-in.css deleted file mode 100644 index 151b14fe3..000000000 --- a/app/assets/tailwind/google-sign-in.css +++ /dev/null @@ -1,106 +0,0 @@ -@layer components { - .gsi-material-button { - -moz-user-select: none; - -webkit-user-select: none; - -ms-user-select: none; - -webkit-appearance: none; - background-color: WHITE; - background-image: none; - border: 1px solid #747775; - -webkit-border-radius: 4px; - border-radius: 4px; - -webkit-box-sizing: border-box; - box-sizing: border-box; - color: #1f1f1f; - cursor: pointer; - font-family: 'Roboto', arial, sans-serif; - font-size: 14px; - height: 40px; - letter-spacing: 0.25px; - outline: none; - overflow: hidden; - padding: 0 12px; - position: relative; - text-align: center; - -webkit-transition: background-color .218s, border-color .218s, box-shadow .218s; - transition: background-color .218s, border-color .218s, box-shadow .218s; - vertical-align: middle; - white-space: nowrap; - width: auto; - max-width: 400px; - min-width: min-content; - display: inline-flex; - } - - .gsi-material-button .gsi-material-button-icon { - height: 20px; - margin-right: 12px; - min-width: 20px; - width: 20px; - } - - .gsi-material-button .gsi-material-button-content-wrapper { - -webkit-align-items: center; - align-items: center; - display: flex; - -webkit-flex-direction: row; - flex-direction: row; - -webkit-flex-wrap: nowrap; - flex-wrap: nowrap; - height: 100%; - justify-content: space-between; - position: relative; - width: 100%; - } - - .gsi-material-button .gsi-material-button-contents { - -webkit-flex-grow: 1; - flex-grow: 1; - font-family: 'Roboto', arial, sans-serif; - font-weight: 500; - overflow: hidden; - text-overflow: ellipsis; - vertical-align: top; - } - - .gsi-material-button .gsi-material-button-state { - -webkit-transition: opacity .218s; - transition: opacity .218s; - bottom: 0; - left: 0; - opacity: 0; - position: absolute; - right: 0; - top: 0; - } - - .gsi-material-button:disabled { - cursor: default; - background-color: #ffffff61; - border-color: #1f1f1f1f; - } - - .gsi-material-button:disabled .gsi-material-button-contents { - opacity: 38%; - } - - .gsi-material-button:disabled .gsi-material-button-icon { - opacity: 38%; - } - - .gsi-material-button:not(:disabled):active .gsi-material-button-state, - .gsi-material-button:not(:disabled):focus .gsi-material-button-state { - background-color: #303030; - opacity: 12%; - } - - .gsi-material-button:not(:disabled):hover { - -webkit-box-shadow: 0 1px 2px 0 rgba(60, 64, 67, .30), 0 1px 3px 1px rgba(60, 64, 67, .15); - box-shadow: 0 1px 2px 0 rgba(60, 64, 67, .30), 0 1px 3px 1px rgba(60, 64, 67, .15); - } - - .gsi-material-button:not(:disabled):hover .gsi-material-button-state { - background-color: #303030; - opacity: 8%; - } -} diff --git a/app/components/DS/button.html.erb b/app/components/DS/button.html.erb index 557db0bd9..8186a4349 100644 --- a/app/components/DS/button.html.erb +++ b/app/components/DS/button.html.erb @@ -1,6 +1,6 @@ <%= container do %> <% if icon && (icon_position != :right) %> - <%= helpers.icon(icon, size: size, color: icon_color, class: icon_classes) %> + <%= helpers.icon(icon, size: size, color: icon_color, custom: icon_custom, class: icon_classes) %> <% end %> <% unless icon_only? %> @@ -8,6 +8,6 @@ <% end %> <% if icon && icon_position == :right %> - <%= helpers.icon(icon, size: size, color: icon_color) %> + <%= helpers.icon(icon, size: size, color: icon_color, custom: icon_custom) %> <% end %> <% end %> diff --git a/app/components/DS/buttonish.rb b/app/components/DS/buttonish.rb index f1e864511..d7fa580a6 100644 --- a/app/components/DS/buttonish.rb +++ b/app/components/DS/buttonish.rb @@ -55,13 +55,14 @@ class DS::Buttonish < DesignSystemComponent } }.freeze - attr_reader :variant, :size, :href, :icon, :icon_position, :text, :full_width, :extra_classes, :frame, :opts + attr_reader :variant, :size, :href, :icon, :icon_custom, :icon_position, :text, :full_width, :extra_classes, :frame, :opts - def initialize(variant: :primary, size: :md, href: nil, text: nil, icon: nil, icon_position: :left, full_width: false, frame: nil, **opts) + def initialize(variant: :primary, size: :md, href: nil, text: nil, icon: nil, icon_custom: false, icon_position: :left, full_width: false, frame: nil, **opts) @variant = variant.to_s.underscore.to_sym @size = size.to_sym @href = href @icon = icon + @icon_custom = icon_custom @icon_position = icon_position.to_sym @text = text @full_width = full_width diff --git a/app/components/DS/pill.rb b/app/components/DS/pill.rb index a868d8baa..b04ef61d6 100644 --- a/app/components/DS/pill.rb +++ b/app/components/DS/pill.rb @@ -32,6 +32,13 @@ class DS::Pill < DesignSystemComponent # # Other options: # + # - `show_dot:` defaults per mode. Stage markers (`marker: true`) keep + # their dot; status / category badges (`marker: false`) are clean by + # default — the pill shape + tone + label already carry the signal, so + # a leading dot is usually redundant and noisy in dense lists. Pass + # `show_dot: true` to opt a badge back in where the dot is genuinely + # additive: live / temporal status ("Syncing", "Active"), or a single + # sparse pill where the dot anchors it as a discrete element. # - `dot_only: true` renders only the colored dot (no label, no border). # Use on the collapsed sidebar nav, where there's no room for the label. # - `icon:` overrides the dot with a Lucide icon (sized xs, current color). @@ -44,13 +51,15 @@ class DS::Pill < DesignSystemComponent # - Sure has full violet / indigo / fuchsia / amber / green / gray / # red ramps in the design system; this component picks named tokens # at render time. No raw hex. - def initialize(label: nil, tone: :violet, style: :soft, size: :sm, show_dot: true, dot_only: false, title: nil, icon: nil, marker: true, custom_color: nil) + def initialize(label: nil, tone: :violet, style: :soft, size: :sm, show_dot: nil, dot_only: false, title: nil, icon: nil, marker: true, custom_color: nil) resolved_tone = SEMANTIC_TONE_ALIASES.fetch(tone.to_sym, tone.to_sym) @label = label || I18n.t("ds.pill.default_label", default: "Beta") @tone = TONES.include?(resolved_tone) ? resolved_tone : :violet @style = STYLES.include?(style.to_sym) ? style.to_sym : :soft @size = SIZES.include?(size.to_sym) ? size.to_sym : :sm - @show_dot = show_dot + # Default per mode: markers keep their dot, badges are dot-less. An + # explicit show_dot: true/false always wins. + @show_dot = show_dot.nil? ? marker : show_dot @dot_only = dot_only @title = title @icon = icon diff --git a/app/components/DS/progress_ring.html.erb b/app/components/DS/progress_ring.html.erb new file mode 100644 index 000000000..d3940cca8 --- /dev/null +++ b/app/components/DS/progress_ring.html.erb @@ -0,0 +1,20 @@ +<%= tag.div class: "relative inline-flex shrink-0", + style: "width: #{size}px; height: #{size}px;", + **wrapper_aria do %> + + <% if show_percent %> + + <% end %> +<% end %> diff --git a/app/components/DS/progress_ring.rb b/app/components/DS/progress_ring.rb new file mode 100644 index 000000000..eb904164c --- /dev/null +++ b/app/components/DS/progress_ring.rb @@ -0,0 +1,80 @@ +# A single-arc circular progress ring, decoupled from any domain model. +# +# Extracted from the goal card's inline (issue #1899) so goals, loans, +# sub-account funding, etc. stop each hand-rolling the same two-circle SVG with +# slightly different chrome / colors / a11y. Pass a percent and a tone; the +# component owns the geometry (radius, circumference, dash offset) and the +# accessible progressbar markup. +# +# Not a segmented donut — that's the `donut-chart` Stimulus controller's job +# (budget/dashboard breakdowns, and the goals/show ring). This is the simple +# "X% of one thing" ring. +class DS::ProgressRing < DesignSystemComponent + TONES = { + success: "var(--color-success)", + warning: "var(--color-warning)", + destructive: "var(--color-destructive)", + neutral: "var(--color-gray-400)" + }.freeze + + # Track (unfilled remainder) color. Reuses the existing token to keep the + # goal card pixel-identical. TODO(#1899 follow-up): rename this to a generic + # --color-progress-track-fill in the token source — that change also touches + # the budget donut surfaces, so it's deferred out of this extraction. + DEFAULT_TRACK = "var(--budget-unused-fill)".freeze + + attr_reader :size, :stroke_width, :label, :show_percent + + def initialize(percent:, size: 64, stroke_width: 6, tone: :neutral, label: nil, show_percent: true, track: DEFAULT_TRACK) + @percent = percent + @size = size + @stroke_width = stroke_width + @tone = tone.to_sym + @label = label + @show_percent = show_percent + @track = track + end + + def clamped_percent + [ [ @percent.to_i, 0 ].max, 100 ].min + end + + def stroke_color + TONES.fetch(@tone, TONES[:neutral]) + end + + def track_color + @track + end + + def center + size / 2.0 + end + + def radius + (size - stroke_width) / 2.0 + end + + def circumference + 2 * Math::PI * radius + end + + # Length of the dash gap that hides the unfilled portion of the arc. + def dash_offset + circumference * (1 - clamped_percent / 100.0) + end + + # Center label scales with the ring so 64px reads ~11px (the goal card's size) + # and a 180px ring reads ~30px without a per-callsite font class. + def percent_font_px + (size * 0.17).round + end + + # role=progressbar + value/label only when a label is supplied; otherwise the + # ring is decorative (aria-hidden via the inner svg) and the caller labels it. + def wrapper_aria + return {} if label.blank? + + { role: "progressbar", aria: { valuenow: clamped_percent, valuemin: 0, valuemax: 100, label: label } } + end +end diff --git a/app/components/goals/card_component.html.erb b/app/components/goals/card_component.html.erb index 7e2174885..c42340d19 100644 --- a/app/components/goals/card_component.html.erb +++ b/app/components/goals/card_component.html.erb @@ -18,29 +18,7 @@

<%= secondary_line %>

-
- - -
+ <%= render DS::ProgressRing.new(percent: progress_percent, tone: ring_tone) %>
diff --git a/app/components/goals/card_component.rb b/app/components/goals/card_component.rb index 0f68d4839..7ebca94dd 100644 --- a/app/components/goals/card_component.rb +++ b/app/components/goals/card_component.rb @@ -1,7 +1,4 @@ class Goals::CardComponent < ApplicationComponent - RING_SIZE = 64 - RING_STROKE = 6 - def initialize(goal:, filterable: true) @goal = goal @filterable = filterable @@ -13,11 +10,13 @@ class Goals::CardComponent < ApplicationComponent goal.progress_percent end - def ring_color + # Maps goal status to a DS::ProgressRing tone (the ring geometry/colors now + # live in that primitive — see #1899). + def ring_tone case goal.status - when :reached, :on_track then "var(--color-success)" - when :behind then "var(--color-warning)" - else "var(--color-gray-400)" + when :reached, :on_track then :success + when :behind then :warning + else :neutral end end @@ -66,19 +65,6 @@ class Goals::CardComponent < ApplicationComponent end end - def ring_circumference - @ring_circumference ||= 2 * Math::PI * ring_radius - end - - def ring_radius - @ring_radius ||= (RING_SIZE - RING_STROKE) / 2.0 - end - - def ring_offset - pct = [ [ progress_percent.to_i, 0 ].max, 100 ].min - ring_circumference * (1 - pct / 100.0) - end - def pace_line return nil if goal.archived? || goal.paused? || goal.completed? || goal.status == :reached diff --git a/app/controllers/api/v1/imports_controller.rb b/app/controllers/api/v1/imports_controller.rb index 5f1e9cf3e..3c92bff20 100644 --- a/app/controllers/api/v1/imports_controller.rb +++ b/app/controllers/api/v1/imports_controller.rb @@ -35,14 +35,14 @@ class Api::V1::ImportsController < Api::V1::BaseController rescue StandardError => e Rails.logger.error "ImportsController#index error: #{e.message}" - render json: { error: "internal_server_error", message: e.message }, status: :internal_server_error + render json: { error: "internal_server_error", message: "An unexpected error occurred." }, status: :internal_server_error end def show render :show rescue StandardError => e Rails.logger.error "ImportsController#show error: #{e.message}" - render json: { error: "internal_server_error", message: e.message }, status: :internal_server_error + render json: { error: "internal_server_error", message: "An unexpected error occurred." }, status: :internal_server_error end def rows @@ -58,7 +58,7 @@ class Api::V1::ImportsController < Api::V1::BaseController render :rows rescue StandardError => e Rails.logger.error "ImportsController#rows error: #{e.message}" - render json: { error: "internal_server_error", message: e.message }, status: :internal_server_error + render json: { error: "internal_server_error", message: "An unexpected error occurred." }, status: :internal_server_error end def create @@ -133,7 +133,7 @@ class Api::V1::ImportsController < Api::V1::BaseController rescue StandardError => e Rails.logger.error "ImportsController#create error: #{e.message}" - render json: { error: "internal_server_error", message: e.message }, status: :internal_server_error + render json: { error: "internal_server_error", message: "An unexpected error occurred." }, status: :internal_server_error end def preflight @@ -156,7 +156,7 @@ class Api::V1::ImportsController < Api::V1::BaseController render json: { error: "internal_server_error", - message: "Error: #{e.message}" + message: "Import preflight could not be completed." }, status: :internal_server_error end @@ -242,7 +242,7 @@ class Api::V1::ImportsController < Api::V1::BaseController end begin - @import.publish_later if @import.publishable? && params[:publish] == "true" + @import.publish_later if params[:publish] == "true" rescue Import::MaxRowCountExceededError render json: { error: "max_row_count_exceeded", @@ -250,6 +250,22 @@ class Api::V1::ImportsController < Api::V1::BaseController import_id: @import.id }, status: :unprocessable_entity return + rescue SureImport::PreflightError + render json: { + error: "preflight_failed", + message: "Import was uploaded but did not pass Sure NDJSON preflight.", + errors: sure_import_error_lines, + import_id: @import.id + }, status: :unprocessable_entity + return + rescue SureImport::NotPublishableError => e + Rails.logger.warn "Sure import not publishable for import #{@import.id}: #{e.message}" + render json: { + error: "not_publishable", + message: "Import was uploaded but has no publishable records.", + import_id: @import.id + }, status: :unprocessable_entity + return rescue StandardError => e Rails.logger.error "Sure import publish failed for import #{@import.id}: #{e.message}" restore_pending_sure_import_after_publish_failure @@ -284,6 +300,8 @@ class Api::V1::ImportsController < Api::V1::BaseController @import.update_column(:status, "pending") if @import&.persisted? && @import.importing? end + def sure_import_error_lines = @import.error.to_s.lines.map(&:strip).reject(&:blank?) + def clean_up_failed_sure_import(import) return unless import diff --git a/app/controllers/reports_controller.rb b/app/controllers/reports_controller.rb index c31df1eb0..d1b76204e 100644 --- a/app/controllers/reports_controller.rb +++ b/app/controllers/reports_controller.rb @@ -111,17 +111,18 @@ class ReportsController < ApplicationController @previous_period = build_previous_period # Get aggregated data - @current_income_totals = Current.family.income_statement.income_totals(period: @period) - @current_expense_totals = Current.family.income_statement.expense_totals(period: @period) + @income_statement = Current.family.income_statement(user: Current.user) + @current_income_totals = @income_statement.income_totals(period: @period) + @current_expense_totals = @income_statement.expense_totals(period: @period) - @previous_income_totals = Current.family.income_statement.income_totals(period: @previous_period) - @previous_expense_totals = Current.family.income_statement.expense_totals(period: @previous_period) + @previous_income_totals = @income_statement.income_totals(period: @previous_period) + @previous_expense_totals = @income_statement.expense_totals(period: @previous_period) # Calculate summary metrics @summary_metrics = build_summary_metrics # Build trend data (last 6 months) - @trends_data = build_trends_data + @trends_data = build_trends_data(income_statement: @income_statement) # Net worth metrics @net_worth_metrics = build_net_worth_metrics @@ -320,7 +321,7 @@ class ReportsController < ApplicationController nil end - def build_trends_data + def build_trends_data(income_statement:) # Generate month-by-month data based on the current period filter trends = [] @@ -337,8 +338,8 @@ class ReportsController < ApplicationController period = Period.custom(start_date: month_start, end_date: month_end) - income = Current.family.income_statement.income_totals(period: period).total - expenses = Current.family.income_statement.expense_totals(period: period).total + income = income_statement.income_totals(period: period).total + expenses = income_statement.expense_totals(period: period).total trends << { month: month_start.strftime("%b %Y"), @@ -379,8 +380,12 @@ class ReportsController < ApplicationController trades = apply_entry_filters(trades) # Get sort parameters - sort_by = params[:sort_by] || "amount" - sort_direction = params[:sort_direction] || "desc" + sort_by = %w[amount count].include?(params[:sort_by]) ? params[:sort_by] : "amount" + sort_direction = %w[asc desc].include?(params[:sort_direction]) ? params[:sort_direction] : "desc" + sort_logic = ->(item) do + value = (sort_by == "count") ? item[:count] : item[:total] + sort_direction == "asc" ? (value || 0) : -(value || 0) + end # Group by category (tracking parent relationship) and type # Structure: { [parent_category_id, type] => { parent_data, subcategories: { subcategory_id => data } } } @@ -447,16 +452,12 @@ class ReportsController < ApplicationController # Convert to array and sort subcategories result = grouped_data.values.map do |parent_data| - subcategories = parent_data[:subcategories].values.sort_by { |s| sort_direction == "asc" ? s[:total] : -s[:total] } + subcategories = parent_data[:subcategories].values.sort_by(&sort_logic) parent_data.merge(subcategories: subcategories) end - # Sort by amount (total) with the specified direction - if sort_direction == "asc" - result.sort_by { |g| g[:total] } - else - result.sort_by { |g| -g[:total] } - end + # Sort by the chosen key with the specified direction + result.sort_by(&sort_logic) end def build_investment_metrics @@ -466,7 +467,6 @@ class ReportsController < ApplicationController return { has_investments: false } unless investment_accounts.any? period_totals = investment_statement.totals(period: @period) - { has_investments: true, portfolio_value: investment_statement.portfolio_value_money, diff --git a/app/javascript/controllers/sankey_chart_controller.js b/app/javascript/controllers/sankey_chart_controller.js index da4e7fb5d..b7d12c036 100644 --- a/app/javascript/controllers/sankey_chart_controller.js +++ b/app/javascript/controllers/sankey_chart_controller.js @@ -1,6 +1,7 @@ import { Controller } from "@hotwired/stimulus"; import * as d3 from "d3"; import { sankey } from "d3-sankey"; +import { CHART_TOOLTIP_CLASSES } from "utils/chart_tooltip"; import { sankeyNodeHasChildren, zoomSankeyData } from "utils/sankey_zoom"; // Connects to data-controller="sankey-chart" @@ -509,10 +510,9 @@ export default class extends Controller { this.tooltip = d3 .select(dialog || document.body) .append("div") - .attr( - "class", - "bg-container text-primary text-sm font-sans absolute p-3 rounded-xl shadow-lg shadow-border-xs pointer-events-none z-50 top-0 privacy-sensitive", - ) + // Shared visual contract + this chart's positioning class; opacity is + // toggled via inline style below. + .attr("class", `${CHART_TOOLTIP_CLASSES} top-0`) .style("opacity", 0) .style("pointer-events", "none"); } diff --git a/app/javascript/controllers/time_series_chart_controller.js b/app/javascript/controllers/time_series_chart_controller.js index 23b75e3eb..1bf6f8d89 100644 --- a/app/javascript/controllers/time_series_chart_controller.js +++ b/app/javascript/controllers/time_series_chart_controller.js @@ -1,5 +1,6 @@ import { Controller } from "@hotwired/stimulus"; import * as d3 from "d3"; +import { CHART_TOOLTIP_CLASSES } from "utils/chart_tooltip"; const parseLocalDate = d3.timeParse("%Y-%m-%d"); @@ -287,10 +288,8 @@ export default class extends Controller { this._d3Tooltip = d3 .select(`#${this.element.id}`) .append("div") - .attr( - "class", - "bg-container text-primary text-sm font-sans absolute p-3 rounded-xl shadow-lg shadow-border-xs pointer-events-none opacity-0 top-0 z-50 privacy-sensitive", - ); + // Shared visual contract + this chart's initial-hidden / positioning classes. + .attr("class", `${CHART_TOOLTIP_CLASSES} opacity-0 top-0`); } _trackMouseForShowingTooltip() { diff --git a/app/javascript/utils/chart_tooltip.js b/app/javascript/utils/chart_tooltip.js new file mode 100644 index 000000000..e0dad3199 --- /dev/null +++ b/app/javascript/utils/chart_tooltip.js @@ -0,0 +1,25 @@ +// Single source of truth for the cursor-following tooltip used by the chart +// controllers (time-series, sankey, and goal-projection once it lands from the +// goals work). Keeping the visual contract here stops the bg / text / border / +// privacy-sensitive classes from drifting apart across the controllers, the way +// they had before (time-series was missing `text-primary` and `z-50`). +// +// This is the VISUAL contract only. Callers append their own behavioural +// classes (initial `opacity-0`, `top-0`, …) or set them via inline styles, +// because how each chart shows/hides and positions its tooltip differs. +// +// Not to be confused with DS::Tooltip — that is the info-icon hint primitive +// (bg-inverse, aria-describedby, anchored to a static trigger). This is a +// data-card surface created and updated inside D3 handler code. +export const CHART_TOOLTIP_CLASSES = + "bg-container text-primary text-sm font-sans absolute p-3 rounded-xl shadow-lg shadow-border-xs pointer-events-none z-50 privacy-sensitive"; + +// Convenience factory for the raw-DOM idiom (no d3.select). Creates a hidden +// tooltip div carrying the shared contract and appends it to `parent`. +export function createChartTooltip(parent) { + const tooltip = document.createElement("div"); + tooltip.className = CHART_TOOLTIP_CLASSES; + tooltip.style.display = "none"; + parent.appendChild(tooltip); + return tooltip; +} diff --git a/app/models/family/data_exporter.rb b/app/models/family/data_exporter.rb index 0435e1ec3..404218056 100644 --- a/app/models/family/data_exporter.rb +++ b/app/models/family/data_exporter.rb @@ -300,27 +300,38 @@ class Family::DataExporter }.to_json end - # Export transactions with full data (exclude split parents, export children instead) - exportable_transactions.includes(:category, :merchant, :tags, entry: :account).find_each do |transaction| + ndjson_exportable_transactions.includes( + :category, + :merchant, + :tags, + entry: [ + :account, + { child_entries: { entryable: :tags } } + ] + ).find_each do |transaction| + transaction_data = { + id: transaction.id, + entry_id: transaction.entry.id, + account_id: transaction.entry.account_id, + date: transaction.entry.date, + amount: transaction.entry.amount, + currency: transaction.entry.currency, + name: transaction.entry.name, + notes: transaction.entry.notes, + excluded: transaction.entry.excluded, + category_id: transaction.category_id, + merchant_id: transaction.merchant_id, + tag_ids: transaction.tag_ids, + kind: transaction.kind, + created_at: transaction.created_at, + updated_at: transaction.updated_at + } + split_lines = serialize_split_lines_for_export(transaction.entry) + transaction_data[:split_lines] = split_lines if split_lines.any? + lines << { type: "Transaction", - data: { - id: transaction.id, - entry_id: transaction.entry.id, - account_id: transaction.entry.account_id, - date: transaction.entry.date, - amount: transaction.entry.amount, - currency: transaction.entry.currency, - name: transaction.entry.name, - notes: transaction.entry.notes, - excluded: transaction.entry.excluded, - category_id: transaction.category_id, - merchant_id: transaction.merchant_id, - tag_ids: transaction.tag_ids, - kind: transaction.kind, - created_at: transaction.created_at, - updated_at: transaction.updated_at - } + data: transaction_data }.to_json end @@ -456,6 +467,42 @@ class Family::DataExporter @family.transactions.merge(Entry.excluding_split_parents) end + def ndjson_exportable_transactions + @family.transactions.joins(:entry).where(entries: { parent_entry_id: nil }) + end + + def serialize_split_lines_for_export(parent_entry) + child_entries = split_child_entries_for_export(parent_entry) + return [] if child_entries.empty? + + child_entries.map do |child_entry| + transaction = child_entry.entryable + { + id: transaction.id, + entry_id: child_entry.id, + amount: child_entry.amount, + currency: child_entry.currency, + name: child_entry.name, + notes: child_entry.notes, + excluded: child_entry.excluded, + category_id: transaction.category_id, + merchant_id: transaction.merchant_id, + tag_ids: transaction.tag_ids, + kind: transaction.kind, + created_at: transaction.created_at, + updated_at: transaction.updated_at + } + end + end + + def split_child_entries_for_export(parent_entry) + if parent_entry.association(:child_entries).loaded? + parent_entry.child_entries.sort_by { |entry| [ entry.created_at, entry.id ] } + else + parent_entry.child_entries.order(:created_at, :id).to_a + end + end + def family_transaction_ids @family_transaction_ids ||= exportable_transactions.select(:id) end diff --git a/app/models/family/data_importer.rb b/app/models/family/data_importer.rb index 8ee468dc8..4de55a9bc 100644 --- a/app/models/family/data_importer.rb +++ b/app/models/family/data_importer.rb @@ -2,7 +2,15 @@ require "set" class Family::DataImporter SUPPORTED_TYPES = %w[Account Balance Category Tag Merchant RecurringTransaction Transaction Transfer RejectedTransfer Trade Holding Valuation Budget BudgetCategory Rule].freeze - ACCOUNTABLE_TYPES = Accountable::TYPES.freeze + ACCOUNTABLE_TYPE_CLASSES = { + "Depository" => Depository, "Investment" => Investment, "Crypto" => Crypto, + "Property" => Property, "Vehicle" => Vehicle, "OtherAsset" => OtherAsset, + "CreditCard" => CreditCard, "Loan" => Loan, "OtherLiability" => OtherLiability + }.freeze + + def self.accountable_class_for(type) + ACCOUNTABLE_TYPE_CLASSES[type.to_s] + end def initialize(family, ndjson_content) @family = family @@ -78,11 +86,9 @@ class Family::DataImporter accountable_data = data["accountable"] || {} accountable_type = data["accountable_type"] - # Skip if accountable type is not valid - next unless ACCOUNTABLE_TYPES.include?(accountable_type) + accountable_class = self.class.accountable_class_for(accountable_type) + next unless accountable_class - # Build accountable - accountable_class = accountable_type.constantize accountable = accountable_class.new accountable.subtype = accountable_data["subtype"] if accountable.respond_to?(:subtype=) && accountable_data["subtype"] @@ -110,8 +116,9 @@ class Family::DataImporter account.save! # Set opening balance if we have a historical balance and the import - # does not provide an explicit opening-anchor valuation for this account. - if data["balance"].present? && !@imported_opening_anchor_account_ids.include?(old_id) + # does not provide either an explicit opening-anchor valuation or an + # authoritative balance-history stream for this account. + if data["balance"].present? && !skip_opening_balance_import?(old_id, data) manager = Account::OpeningBalanceManager.new(account) result = manager.set_opening_balance( balance: data["balance"].to_d, @@ -190,8 +197,8 @@ class Family::DataImporter classification_unused: data["classification_unused"] || data["classification"] || "expense", lucide_icon: data["lucide_icon"] || "shapes" ) - category.save! + @id_mappings[:categories][old_id] = category.id end @@ -216,8 +223,8 @@ class Family::DataImporter name: data["name"], color: data["color"] || Tag::COLORS.sample ) - tag.save! + @id_mappings[:tags][old_id] = tag.id end end @@ -232,8 +239,8 @@ class Family::DataImporter color: data["color"], logo_url: data["logo_url"] ) - merchant.save! + @id_mappings[:merchants][old_id] = merchant.id end end @@ -324,10 +331,7 @@ class Family::DataImporter end # Map tag IDs (optional) - new_tag_ids = [] - if data["tag_ids"].present? - new_tag_ids = Array(data["tag_ids"]).map { |old_tag_id| @id_mappings[:tags][old_tag_id] }.compact - end + new_tag_ids = mapped_tag_ids(data["tag_ids"]) transaction = Transaction.new( category_id: new_category_id, @@ -348,13 +352,80 @@ class Family::DataImporter entry.save! - # Add tags through the tagging association - new_tag_ids.each do |tag_id| + @id_mappings[:transactions][old_id] = transaction.id + split_rows = importable_split_rows(data) + + if split_rows.any? + @created_entries << entry + import_split_lines!(entry, split_rows, fallback_tag_ids: new_tag_ids) + else + new_tag_ids.each do |tag_id| + transaction.taggings.create!(tag_id: tag_id) + end + + @created_entries << entry + end + end + end + + def mapped_tag_ids(old_tag_ids) + Array(old_tag_ids).map { |old_tag_id| @id_mappings[:tags][old_tag_id] }.compact + end + + def importable_split_rows(data) + rows = data["split_lines"].presence || data["splitLines"].presence || data["splits"].presence + Array(rows).filter_map do |row| + next unless row.is_a?(Hash) + + amount = row["amount"] || row["amount_money"] || row["amount_decimal"] + next if amount.blank? + + category_id = remap_optional_id(:categories, row["category_id"]) + merchant_id = remap_optional_id(:merchants, row["merchant_id"]) + + { + old_id: row["id"], + name: row["name"].presence || row["memo"].presence || row["description"].presence || "Imported split", + amount: amount.to_d, + category_id: category_id, + merchant_id: merchant_id, + merchant_id_provided: row.key?("merchant_id"), + notes: row["notes"], + excluded: boolean_import_value(row, "excluded", default: false), + tag_ids: mapped_tag_ids(row["tag_ids"]), + tag_ids_provided: row.key?("tag_ids"), + kind: row["kind"] + } + end + end + + def import_split_lines!(entry, split_rows, fallback_tag_ids:) + children = entry.split!( + split_rows.map do |row| + { + name: row[:name], + amount: row[:amount], + category_id: row[:category_id], + excluded: row[:excluded] + } + end + ) + + children.zip(split_rows).each do |child_entry, row| + transaction = child_entry.entryable + transaction.update!( + merchant_id: row[:merchant_id_provided] ? row[:merchant_id] : transaction.merchant_id, + kind: row[:kind].presence || transaction.kind + ) + child_entry.update!(notes: row[:notes]) if row[:notes].present? + + tag_ids = row[:tag_ids_provided] ? row[:tag_ids] : fallback_tag_ids + tag_ids.each do |tag_id| transaction.taggings.create!(tag_id: tag_id) end - @created_entries << entry - @id_mappings[:transactions][old_id] = transaction.id + @id_mappings[:transactions][row[:old_id]] = transaction.id if row[:old_id].present? + @created_entries << child_entry end end @@ -540,6 +611,12 @@ class Family::DataImporter end end + def skip_opening_balance_import?(old_id, data) + @imported_opening_anchor_account_ids.include?(old_id) || + truthy?(data["skip_opening_balance_import"]) || + truthy?(data["authoritative_balance_history"]) + end + def opening_balance_date_for(old_id, data) explicit_date = parse_import_date( data["opening_balance_date"] || data["opening_balance_on"] diff --git a/app/models/import/preflight.rb b/app/models/import/preflight.rb index 69051eac3..20199f80b 100644 --- a/app/models/import/preflight.rb +++ b/app/models/import/preflight.rb @@ -232,68 +232,21 @@ class Import::Preflight end def sure_import_preflight_payload(content, filename, content_type) - line_counts = Hash.new(0) - errors = [] - valid_rows_count = 0 - nonblank_rows_count = 0 - - content.each_line.with_index(1) do |line, line_number| - next if line.strip.blank? - - nonblank_rows_count += 1 - record = JSON.parse(line) - - unless record.is_a?(Hash) - errors << { - code: "invalid_ndjson_record", - message: "Line #{line_number} must be a JSON object." - } - next - end - - if record["type"].blank? || !record.key?("data") - errors << { - code: "invalid_ndjson_record", - message: "Line #{line_number} must include type and data." - } - next - end - - valid_rows_count += 1 - line_counts[record["type"]] += 1 - rescue JSON::ParserError => e - errors << { - code: "invalid_json", - message: "Line #{line_number} is not valid JSON: #{e.message}" - } - end - - if nonblank_rows_count.zero? - errors << { - code: "no_data_rows", - message: "No data rows were found." - } - end - - entity_counts = SureImport.dry_run_totals_from_line_type_counts(line_counts) - unsupported_types = line_counts.keys - SureImport.importable_ndjson_types - warnings = [] - warnings << "No importable records were found." if nonblank_rows_count.positive? && entity_counts.values.sum.zero? - warnings << "Some records use unsupported types: #{unsupported_types.join(', ')}" if unsupported_types.any? - warnings << "Row count exceeds this import type's publish limit." if nonblank_rows_count > SureImport.max_row_count + result = SureImport::Preflight.new( + family: family, + content: content + ).call + stats = result.stats + warnings = result.warnings.dup + warnings << "No importable records were found." if stats[:rows_count].positive? && (stats[:entity_counts] || {}).values.sum.zero? + warnings << "Row count exceeds this import type's publish limit." if stats[:rows_count] > SureImport.max_row_count { type: "SureImport", - valid: errors.empty?, + valid: result.valid?, content: content_payload(filename, content_type, content), - stats: { - rows_count: nonblank_rows_count, - valid_rows_count: valid_rows_count, - invalid_rows_count: nonblank_rows_count - valid_rows_count, - entity_counts: entity_counts, - record_type_counts: line_counts - }, - errors: errors, + stats: stats, + errors: result.errors, warnings: warnings } end diff --git a/app/models/income_statement.rb b/app/models/income_statement.rb index dd1cf459a..ff3f3db56 100644 --- a/app/models/income_statement.rb +++ b/app/models/income_statement.rb @@ -143,7 +143,7 @@ class IncomeStatement def build_period_total(classification:, period:) # Exclude pending transactions from budget calculations - totals = totals_query(transactions_scope: family.transactions.visible.excluding_pending.in_period(period), date_range: period.date_range).select { |t| t.classification == classification } + totals = totals_for_period(period).select { |t| t.classification == classification } classification_total = totals.sum(&:total) uncategorized_category = family.categories.uncategorized @@ -186,6 +186,15 @@ class IncomeStatement ) end + def totals_for_period(period) + @totals_for_period ||= {} + @totals_for_period[period_cache_key(period)] ||= + totals_query( + transactions_scope: family.transactions.visible.excluding_pending.in_period(period), + date_range: period.date_range + ) + end + def family_stats(interval: "month") @family_stats ||= {} @family_stats[interval] ||= Rails.cache.fetch([ diff --git a/app/models/recurring_transaction.rb b/app/models/recurring_transaction.rb index 090d31549..f05df6fcb 100644 --- a/app/models/recurring_transaction.rb +++ b/app/models/recurring_transaction.rb @@ -260,29 +260,15 @@ class RecurringTransaction < ApplicationRecord # Find matching transactions for this recurring pattern def matching_transactions - # For manual recurring with amount variance, match within range - # For automatic recurring, match exact amount - base = account.present? ? account.entries : family.entries + # Recurring transfers can't be matched by single-account name/amount — + # future occurrences carry arbitrary names — so match the Transfer pair. + return transfer_matching_transactions if transfer? - entries = if manual? && has_amount_variance? - base - .where(entryable_type: "Transaction") - .where(currency: currency) - .where("entries.amount BETWEEN ? AND ?", expected_amount_min, expected_amount_max) - .where("EXTRACT(DAY FROM entries.date) BETWEEN ? AND ?", - [ expected_day_of_month - 2, 1 ].max, - [ expected_day_of_month + 2, 31 ].min) - .order(date: :desc) - else - base - .where(entryable_type: "Transaction") - .where(currency: currency) - .where("entries.amount = ?", amount) - .where("EXTRACT(DAY FROM entries.date) BETWEEN ? AND ?", - [ expected_day_of_month - 2, 1 ].max, - [ expected_day_of_month + 2, 31 ].min) - .order(date: :desc) - end + # Amount/cadence-scoped Transaction entries on this account (or family). + base = account.present? ? account.entries : family.entries + entries = day_of_month_scope( + amount_window_scope(base.where(entryable_type: "Transaction").where(currency: currency)) + ).order(date: :desc) # Filter by merchant or name if merchant_id.present? @@ -401,6 +387,47 @@ class RecurringTransaction < ApplicationRecord end private + # Issue #1590: a recurring transfer's future occurrences rarely share the + # seed's name (user free-text, importer wording, the auto-matcher's + # "Transfer to ..."), so name-based matching returns [] and the Cleaner + # would wrongly inactivate a still-active transfer. Match the Transfer + # *pair* instead — an outflow on the source account paired with an inflow + # on the destination account, within the usual amount/cadence window — and + # return the outflow entries (the occurrence-date carrier, consistent with + # create_from_transfer). + def transfer_matching_transactions + return Entry.none unless account && destination_account + + outflow_entries = day_of_month_scope( + amount_window_scope(account.entries.where(entryable_type: "Transaction").where(currency: currency)) + ).order(date: :desc) + + paired_outflow_transaction_ids = Transfer + .where(outflow_transaction_id: outflow_entries.select(:entryable_id)) + .where(inflow_transaction_id: + destination_account.entries.where(entryable_type: "Transaction").select(:entryable_id)) + .pluck(:outflow_transaction_id) + + outflow_entries.where(entryable_id: paired_outflow_transaction_ids) + end + + # Transaction entries whose amount fits the pattern: exact, or within the + # configured variance band for manual recurring rows. + def amount_window_scope(relation) + if manual? && has_amount_variance? + relation.where("entries.amount BETWEEN ? AND ?", expected_amount_min, expected_amount_max) + else + relation.where("entries.amount = ?", amount) + end + end + + # Entries whose day-of-month lands within ±2 days of the expected day. + def day_of_month_scope(relation) + relation.where("EXTRACT(DAY FROM entries.date) BETWEEN ? AND ?", + [ expected_day_of_month - 2, 1 ].max, + [ expected_day_of_month + 2, 31 ].min) + end + def monetizable_currency currency end diff --git a/app/models/recurring_transaction/cleaner.rb b/app/models/recurring_transaction/cleaner.rb index dd22dcb89..ec5dfd917 100644 --- a/app/models/recurring_transaction/cleaner.rb +++ b/app/models/recurring_transaction/cleaner.rb @@ -9,18 +9,15 @@ class RecurringTransaction # Mark recurring transactions as inactive if they haven't occurred recently # Uses 2 months for automatic recurring, 6 months for manual recurring. # - # Transfer rows (destination_account_id present) are skipped: their - # `matching_transactions` helper looks at single-account name/amount - # which never matches a Transfer pair, so the Cleaner would - # incorrectly mark a still-recurring transfer inactive at the - # 6-month threshold. Issue #1590 tracks pair-detection-aware - # matching for recurring transfers. + # Transfer rows (destination_account_id present) are included: as of issue + # #1590, `matching_transactions` detects the Transfer pair, so a still-active + # transfer keeps surfacing recent matches and stays active, while one whose + # pair has genuinely stopped is correctly retired. def cleanup_stale_transactions stale_count = 0 family.recurring_transactions .active - .where(destination_account_id: nil) .find_each do |recurring_transaction| next unless recurring_transaction.should_be_inactive? diff --git a/app/models/sure_import.rb b/app/models/sure_import.rb index 0fab88a27..c738e6156 100644 --- a/app/models/sure_import.rb +++ b/app/models/sure_import.rb @@ -1,5 +1,10 @@ class SureImport < Import - MAX_NDJSON_SIZE = 10.megabytes + NotPublishableError = Class.new(StandardError) + PreflightError = Class.new(StandardError) + + DEFAULT_MAX_NDJSON_SIZE_MB = 10 + DEFAULT_MAX_ROW_COUNT = 100_000 + MAX_NDJSON_SIZE = DEFAULT_MAX_NDJSON_SIZE_MB.megabytes IMPORTABLE_NDJSON_TYPES = { "Account" => :accounts, "Balance" => :balances, @@ -30,11 +35,11 @@ class SureImport < Import class << self def max_row_count - 100_000 + positive_integer_env("SURE_IMPORT_MAX_ROWS", DEFAULT_MAX_ROW_COUNT) end def max_ndjson_size - MAX_NDJSON_SIZE + positive_integer_env("SURE_IMPORT_MAX_NDJSON_SIZE_MB", DEFAULT_MAX_NDJSON_SIZE_MB).megabytes end # Counts JSON lines by top-level "type" (used for dry-run summaries and row limits). @@ -90,6 +95,12 @@ class SureImport < Import false end end + + private + def positive_integer_env(name, default) + value = ENV[name].to_i + value.positive? ? value : default + end end def requires_csv_workflow? @@ -133,6 +144,37 @@ class SureImport < Import raise end + def publish_later + raise MaxRowCountExceededError if row_count_exceeded? + + validate_sure_preflight! + raise NotPublishableError, "Import was uploaded but has no publishable records." unless publishable? + + previous_status = status + update! status: :importing + + begin + ImportJob.perform_later(self) + rescue StandardError + update! status: previous_status + raise + end + end + + def publish + raise MaxRowCountExceededError if row_count_exceeded? + + validate_sure_preflight! + + import! + + family.sync_later + + update! status: :complete + rescue StandardError => error + update! status: :failed, error: error.message + end + def uploaded? return false unless ndjson_file.attached? @@ -163,6 +205,13 @@ class SureImport < Import self.class.max_row_count end + def sure_preflight + SureImport::Preflight.new( + family: family, + content: ndjson_blob_string + ).call + end + # Row total for max-row enforcement (counts every parsed line with a "type", including unsupported types). def sync_ndjson_rows_count! return unless ndjson_file.attached? @@ -302,4 +351,12 @@ class SureImport < Import @ndjson_blob_id = blob_id @ndjson_blob_string = ndjson_file.download.force_encoding(Encoding::UTF_8) end + + def validate_sure_preflight! + result = sure_preflight + return if result.valid? + + update! status: :failed, error: result.error_message + raise PreflightError, result.error_message + end end diff --git a/app/models/sure_import/preflight.rb b/app/models/sure_import/preflight.rb new file mode 100644 index 000000000..d12eac05c --- /dev/null +++ b/app/models/sure_import/preflight.rb @@ -0,0 +1,312 @@ +# frozen_string_literal: true + +require "set" + +class SureImport::Preflight + Result = Struct.new(:errors, :warnings, :stats, keyword_init: true) do + def valid? = errors.empty? + def error_messages = errors.map { |error| error[:message] } + def error_message = valid? ? "" : ([ "Sure import preflight failed:" ] + error_messages).join("\n") + def payload = { valid: valid?, stats: stats, errors: errors, warnings: warnings } + end + + REQUIRED_FIELDS = { + "Account" => %w[id name balance accountable_type], + "Balance" => %w[account_id date balance], + "Category" => %w[id name], + "Tag" => %w[id name], + "Merchant" => %w[id name], + "RecurringTransaction" => %w[id amount expected_day_of_month last_occurrence_date next_expected_date], + "Transaction" => %w[id account_id date amount], + "Transfer" => %w[inflow_transaction_id outflow_transaction_id], + "RejectedTransfer" => %w[inflow_transaction_id outflow_transaction_id], + "Trade" => %w[account_id date amount qty price ticker], + "Holding" => %w[account_id date amount qty price ticker], + "Valuation" => %w[account_id date amount], + "Budget" => %w[id start_date end_date], + "BudgetCategory" => %w[budget_id category_id], + "Rule" => %w[name] + }.freeze + + TAXONOMY_TYPES = { "Category" => :categories, "Tag" => :tags, "Merchant" => :merchants }.freeze + + SOURCE_ID_TYPES = TAXONOMY_TYPES.merge( + "Account" => :accounts, + "RecurringTransaction" => :recurring_transactions, + "Transaction" => :transactions, + "Budget" => :budgets + ).freeze + + REFERENCE_FIELDS = { + "Balance" => { accounts: %w[account_id] }, + "Category" => { categories: %w[parent_id] }, + "RecurringTransaction" => { accounts: %w[account_id], merchants: %w[merchant_id] }, + "Transaction" => { accounts: %w[account_id], categories: %w[category_id], merchants: %w[merchant_id] }, + "Transfer" => { transactions: %w[inflow_transaction_id outflow_transaction_id] }, + "RejectedTransfer" => { transactions: %w[inflow_transaction_id outflow_transaction_id] }, + "Trade" => { accounts: %w[account_id] }, + "Holding" => { accounts: %w[account_id] }, + "Valuation" => { accounts: %w[account_id] }, + "BudgetCategory" => { budgets: %w[budget_id], categories: %w[category_id] } + }.freeze + + def initialize(family:, content:) + @family = family + @content = content.to_s + @errors = [] + @warnings = [] + @line_counts = Hash.new(0) + @records = Hash.new { |hash, key| hash[key] = [] } + @source_ids = Hash.new { |hash, key| hash[key] = Set.new } + @source_id_locations = Hash.new { |hash, key| hash[key] = Hash.new { |ids, id| ids[id] = [] } } + @rows_count = 0 + @valid_rows_count = 0 + end + + def call + parse_records + validate_taxonomy_collisions + validate_duplicate_taxonomy_names + validate_duplicate_source_ids + validate_required_fields + validate_accountables + validate_split_lines + validate_references + validate_duplicate_valuations + Result.new( + errors: @errors, + warnings: @warnings, + stats: { + rows_count: @rows_count, + valid_rows_count: @valid_rows_count, + invalid_rows_count: @rows_count - @valid_rows_count, + entity_counts: SureImport.dry_run_totals_from_line_type_counts(@line_counts), + record_type_counts: @line_counts + } + ) + end + + private + attr_reader :family + + def parse_records + @content.each_line.with_index(1) do |line, line_number| + next if line.strip.blank? + @rows_count += 1 + record = JSON.parse(line) + unless record.is_a?(Hash) + add_error(:invalid_ndjson_record, "Line #{line_number} must be a JSON object.") + next + end + + type = record["type"] + data = record["data"] + if type.blank? || !record.key?("data") + add_error(:invalid_ndjson_record, "Line #{line_number} must include type and data.") + next + end + + @line_counts[type] += 1 + unless Family::DataImporter::SUPPORTED_TYPES.include?(type) + add_error(:unsupported_record_type, "Line #{line_number} has unsupported record type #{type}.") + next + end + + unless data.is_a?(Hash) + add_error(:invalid_ndjson_record, "Line #{line_number} data must be a JSON object.") + next + end + + @valid_rows_count += 1 + @records[type] << { line_number: line_number, data: data } + mapping_key = SOURCE_ID_TYPES[type] + track_source_id(mapping_key, data["id"], "Line #{line_number} #{type}") if mapping_key && data["id"].present? + add_split_line_source_ids(data, line_number) if type == "Transaction" + rescue JSON::ParserError => e + add_error(:invalid_json, "Line #{line_number} is not valid JSON: #{e.message}") + end + + add_error(:no_data_rows, "No data rows were found.") if @rows_count.zero? + end + + def track_source_id(mapping_key, id, location) + id = id.to_s + @source_ids[mapping_key].add(id) + @source_id_locations[mapping_key][id] << location + end + + def add_split_line_source_ids(data, line_number) + split_lines = split_lines_value(data) + return unless split_lines.is_a?(Array) + split_lines.each_with_index do |split_line, index| + next unless split_line.is_a?(Hash) && split_line["id"].present? + track_source_id(:transactions, split_line["id"], "Line #{line_number} Transaction split line #{index + 1}") + end + end + + def validate_taxonomy_collisions + TAXONOMY_TYPES.each do |type, association| + existing_names = family.public_send(association).pluck(:name).to_set + @records[type].each do |record| + name = record[:data]["name"].to_s + next if name.blank? || !existing_names.include?(name) + add_error( + :existing_taxonomy_collision, + "Line #{record[:line_number]} #{type} name #{name.inspect} already exists in this family." + ) + end + end + end + + def validate_duplicate_taxonomy_names + TAXONOMY_TYPES.each_key do |type| + grouped = @records[type].group_by { |record| record[:data]["name"].to_s } + grouped.each do |name, records| + next if name.blank? || records.one? + lines = records.map { |record| record[:line_number] }.join(", ") + add_error(:duplicate_taxonomy_name, "#{type} name #{name.inspect} appears more than once in the NDJSON on lines #{lines}.") + end + end + end + + def validate_duplicate_source_ids + @source_id_locations.each do |mapping_key, ids| + ids.each do |id, locations| + next if locations.one? + add_error( + :duplicate_source_id, + "#{mapping_key.to_s.singularize.tr('_', ' ')} source id #{id.inspect} appears more than once (#{locations.join(', ')})." + ) + end + end + end + + def validate_required_fields + @records.each do |type, records| + required_fields = REQUIRED_FIELDS.fetch(type, []) + records.each do |record| + missing = required_fields.select { |field| blank_required_value?(record[:data][field]) } + next if missing.empty? + add_error(:missing_required_fields, "Line #{record[:line_number]} #{type} is missing required field(s): #{missing.join(', ')}.") + end + end + end + + def validate_accountables + @records["Account"].each do |record| + data = record[:data] + accountable_type = data["accountable_type"].to_s + accountable_class = Family::DataImporter.accountable_class_for(accountable_type) + unless accountable_class + add_error(:invalid_accountable_type, "Line #{record[:line_number]} Account has invalid accountable_type #{accountable_type.inspect}.") + next + end + + subtype = data.dig("accountable", "subtype").presence || data["subtype"].presence + next if subtype.blank? + subtype_map = accountable_class.const_defined?(:SUBTYPES) ? accountable_class::SUBTYPES : {} + next if subtype_map.blank? || subtype_map.key?(subtype) + add_error(:invalid_accountable_subtype, "Line #{record[:line_number]} Account has invalid #{accountable_type} subtype #{subtype.inspect}.") + end + end + + def validate_split_lines + @records["Transaction"].each do |record| + split_lines = split_lines_value(record[:data]) + next if split_lines.blank? + unless split_lines.is_a?(Array) + add_error(:invalid_split_lines, "Line #{record[:line_number]} Transaction split_lines must be an array.") + next + end + + complete_amounts = true + split_lines.each_with_index do |split_line, index| + unless split_line.is_a?(Hash) + add_error(:invalid_split_line, "Line #{record[:line_number]} Transaction split line #{index + 1} must be a JSON object.") + complete_amounts = false + next + end + + next unless blank_required_value?(split_line_amount(split_line)) + add_error(:missing_required_fields, "Line #{record[:line_number]} Transaction split line #{index + 1} is missing required field(s): amount.") + complete_amounts = false + end + + validate_split_line_total(record, split_lines) if complete_amounts && record[:data]["amount"].present? + end + end + + def validate_split_line_total(record, split_lines) + expected_amount = record[:data]["amount"].to_d + split_total = split_lines.sum { |split_line| split_line_amount(split_line).to_d } + return if split_total == expected_amount + add_error( + :split_amount_mismatch, + "Line #{record[:line_number]} Transaction split line amounts must sum to transaction amount #{expected_amount.to_s('F')} but sum to #{split_total.to_s('F')}." + ) + end + + def validate_references + @records.each do |type, records| + reference_fields = REFERENCE_FIELDS.fetch(type, {}) + records.each do |record| + reference_fields.each do |mapping_key, fields| + fields.each do |field| + validate_reference(record, type, mapping_key, field, record[:data][field]) + end + end + + validate_tag_references(record, type) + validate_split_line_references(record) if type == "Transaction" + end + end + end + + def validate_reference(record, type, mapping_key, field, value) + return if value.blank? + return if @source_ids[mapping_key].include?(value.to_s) + add_error(:missing_reference, "Line #{record[:line_number]} #{type} references missing #{field} #{value.inspect}.") + end + + def validate_tag_references(record, type) + Array(record[:data]["tag_ids"]).each do |tag_id| + validate_reference(record, type, :tags, "tag_ids", tag_id) + end + end + + def validate_split_line_references(record) + split_lines = split_lines_value(record[:data]) + return unless split_lines.is_a?(Array) + Array(split_lines).each do |split_line| + next unless split_line.is_a?(Hash) + validate_reference(record, "Transaction split line", :categories, "category_id", split_line["category_id"]) + validate_reference(record, "Transaction split line", :merchants, "merchant_id", split_line["merchant_id"]) + Array(split_line["tag_ids"]).each do |tag_id| + validate_reference(record, "Transaction split line", :tags, "tag_ids", tag_id) + end + end + end + + def split_lines_value(data) = data["split_lines"].presence || data["splitLines"].presence || data["splits"].presence + + def split_line_amount(split_line) = split_line["amount"] || split_line["amount_money"] || split_line["amount_decimal"] + + def validate_duplicate_valuations + seen = {} + @records["Valuation"].each do |record| + account_id = record[:data]["account_id"] + date = record[:data]["date"] + next if account_id.blank? || date.blank? + key = [ account_id.to_s, date.to_s ] + if seen.key?(key) + add_error(:duplicate_valuation, "Line #{record[:line_number]} duplicates valuation for account #{account_id.inspect} on #{date}; first seen on line #{seen[key]}.") + else + seen[key] = record[:line_number] + end + end + end + + def blank_required_value?(value) = value.blank? + + def add_error(code, message) = @errors << { code: code.to_s, message: message } +end diff --git a/app/views/budget_categories/_budget_category.html.erb b/app/views/budget_categories/_budget_category.html.erb index ff209f009..6b55b971e 100644 --- a/app/views/budget_categories/_budget_category.html.erb +++ b/app/views/budget_categories/_budget_category.html.erb @@ -26,20 +26,11 @@

<%= budget_category.category.name %>

<% if budget_category.over_budget? %> - - <%= icon("alert-circle", size: "sm", color: "red") %> - <%= t("reports.budget_performance.status.over") %> - + <%= render DS::Pill.new(label: t("reports.budget_performance.status.over"), tone: :error, marker: false, icon: "alert-circle") %> <% elsif budget_category.near_limit? %> - - <%= icon("alert-triangle", size: "sm", color: "yellow") %> - <%= t("reports.budget_performance.status.warning") %> - + <%= render DS::Pill.new(label: t("reports.budget_performance.status.warning"), tone: :warning, marker: false, icon: "alert-triangle") %> <% else %> - - <%= icon("check-circle", size: "sm", color: "green") %> - <%= t("reports.budget_performance.status.good") %> - + <%= render DS::Pill.new(label: t("reports.budget_performance.status.good"), tone: :success, marker: false, icon: "check-circle") %> <% end %>
diff --git a/app/views/sessions/new.html.erb b/app/views/sessions/new.html.erb index c51f4977a..60a9409a7 100644 --- a/app/views/sessions/new.html.erb +++ b/app/views/sessions/new.html.erb @@ -55,34 +55,27 @@ <% providers.each do |provider| %> <% provider_id = provider[:id].to_s %> <% provider_name = provider[:name].to_s %> + <% is_google = provider_id == "google" || provider[:strategy].to_s == "google_oauth2" %> + <% default_label = is_google ? t(".google_auth_connect") : t(".#{provider_id}", default: provider[:name].to_s.titleize) %> - <% if provider_id == "google" || provider[:strategy].to_s == "google_oauth2" %> -
- <%= button_to "/auth/#{provider_name}", method: :post, form: { data: { turbo: false } }, class: "gsi-material-button w-full" do %> -
-
-
- - - - - - - -
- <%= provider[:label].presence || t(".google_auth_connect") %> - <%= provider[:label].presence || t(".google_auth_connect") %> -
- <% end %> -
- <% else %> - <%= button_to "/auth/#{provider_name}", method: :post, form: { data: { turbo: false } }, class: "w-full inline-flex items-center justify-center gap-2 rounded-md border border-secondary bg-container px-4 py-2 text-sm font-medium text-primary hover:bg-secondary transition" do %> - <% if provider[:icon].present? %> - <%= icon provider[:icon], size: "sm" %> - <% end %> - <%= provider[:label].presence || t(".#{provider_id}", default: provider[:name].to_s.titleize) %> - <% end %> - <% end %> + <%# + SSO buttons use the design-system outline button so they stay consistent + with each other and read as secondary to the primary email "Log in" CTA. + Google keeps its official multi-color "G" mark + "Sign in with Google" + wording (brand-compliant) via the custom-icon asset, so no bespoke CSS. + %> + <%= render DS::Button.new( + href: "/auth/#{provider_name}", + method: :post, + variant: :outline, + size: :md, + full_width: true, + icon: is_google ? "google-icon" : provider[:icon].presence, + icon_custom: is_google, + text: provider[:label].presence || default_label, + class: "gap-2", + form: { data: { turbo: false } } + ) %> <% end %> <% elsif !AuthConfig.local_login_form_visible? %> diff --git a/app/views/settings/providers/_status_pill.html.erb b/app/views/settings/providers/_status_pill.html.erb index efabfd989..e0fefc018 100644 --- a/app/views/settings/providers/_status_pill.html.erb +++ b/app/views/settings/providers/_status_pill.html.erb @@ -7,9 +7,12 @@ else :neutral end %> +<%# Provider connection state is genuine live status — keep the indicator dot + (badge mode is dot-less by default; this is a deliberate opt-in). %> <%= render DS::Pill.new( label: t("settings.providers.status.#{status}"), tone: tone, marker: false, + show_dot: true, size: :sm ) %> diff --git a/bin/preview_deploy_security_check.rb b/bin/preview_deploy_security_check.rb index 5b4b5b3b1..72339cd6e 100644 --- a/bin/preview_deploy_security_check.rb +++ b/bin/preview_deploy_security_check.rb @@ -8,6 +8,7 @@ PREVIEW_WORKFLOW_PATH = File.join(ROOT, ".github/workflows/preview-deploy.yml") PR_WORKFLOW_PATH = File.join(ROOT, ".github/workflows/pr.yml") LOCKFILE_PATH = File.join(ROOT, "workers/preview/package-lock.json") RESOLVER_PATH = File.join(ROOT, "workers/preview/deploy/resolve_preview_request.cjs") +REDACTION_HELPER_PATH = File.join(ROOT, "workers/preview/deploy/redact_preview_log.sh") PREVIEW_WORKER_PATH = File.join(ROOT, "workers/preview/src/index.ts") PREVIEW_DOCKERFILE_PATH = File.join(ROOT, "Dockerfile.preview") PINNED_ACTION = /\A[^@\s]+@[a-f0-9]{40}\z/ @@ -51,12 +52,17 @@ EXPECTED_COMMENT_PERMISSIONS = { }.freeze EXPECTED_DEPLOY_SECRET_ENV = %w[CLOUDFLARE_ACCOUNT_ID CLOUDFLARE_API_TOKEN CLOUDFLARE_WORKERS_SUBDOMAIN].freeze EXPECTED_PUSH_SECRET_ENV = %w[CLOUDFLARE_ACCOUNT_ID CLOUDFLARE_API_TOKEN].freeze +EXPECTED_FAILURE_DIAGNOSTICS_PATH = "${{ runner.temp }}/preview-failure-diagnostics" +EXPECTED_CLEANUP_METADATA_PATH = "${{ runner.temp }}/preview-cleanup-metadata/wrangler.toml" REQUIRED_PREPARE_LINES = [ 'cp trusted/workers/preview/package.json "$preview_dir/package.json"', 'cp trusted/workers/preview/package-lock.json "$preview_dir/package-lock.json"', 'cp trusted/workers/preview/tsconfig.json "$preview_dir/tsconfig.json"', 'cp trusted/workers/preview/wrangler.toml "$preview_dir/wrangler.toml"', 'cp -R trusted/workers/preview/src "$preview_dir/src"', + 'mkdir -p "$preview_dir/deploy"', + 'cp trusted/workers/preview/deploy/redact_preview_log.sh "$preview_dir/deploy/redact_preview_log.sh"', + 'chmod 0755 "$preview_dir/deploy/redact_preview_log.sh"', 'diagnostics_nonce="$(openssl rand -hex 32)"', 'sed -i "s/\${PREVIEW_DIAGNOSTICS_NONCE}/${diagnostics_nonce}/g" "$preview_dir/src/index.ts"', "Preview diagnostics nonce placeholder was not replaced", @@ -154,6 +160,7 @@ preview_workflow = YAML.safe_load_file(PREVIEW_WORKFLOW_PATH, aliases: true) pr_workflow = YAML.safe_load_file(PR_WORKFLOW_PATH, aliases: true) lockfile = JSON.parse(File.read(LOCKFILE_PATH)) resolver_script = File.read(RESOLVER_PATH) +redaction_helper_script = File.read(REDACTION_HELPER_PATH) preview_worker_script = File.read(PREVIEW_WORKER_PATH) preview_dockerfile = File.read(PREVIEW_DOCKERFILE_PATH) @@ -195,6 +202,10 @@ deploy = step!(deploy_steps, "Deploy to Cloudflare Containers") warm_preview = step!(deploy_steps, "Warm preview container") collect_diagnostics = step!(deploy_steps, "Collect preview diagnostics") upload_diagnostics = step!(deploy_steps, "Upload preview diagnostics") +collect_failure_diagnostics = step!(deploy_steps, "Collect preview failure diagnostics") +upload_failure_diagnostics = step!(deploy_steps, "Upload preview failure diagnostics") +prepare_cleanup_metadata = step!(deploy_steps, "Prepare cleanup metadata") +store_cleanup_metadata = step!(deploy_steps, "Store cleanup metadata") update_deployment_status = step!(deployment_status_steps, "Update Deployment Status") comment_on_pr = step!(preview_comment_steps, "Comment on PR") @@ -215,6 +226,7 @@ comment_on_pr = step!(preview_comment_steps, "Comment on PR") [ "preview gate head output", gate_job.dig("outputs", "head_sha"), "${{ steps.preview.outputs.head_sha }}" ], [ "preview gate fork output", gate_job.dig("outputs", "is_fork"), "${{ steps.preview.outputs.is_fork }}" ], [ "preview gate PR output", gate_job.dig("outputs", "pr_number"), "${{ steps.preview.outputs.pr_number }}" ], + [ "preview gate resolution source output", gate_job.dig("outputs", "resolution_source"), "${{ steps.preview.outputs.resolution_source }}" ], [ "preview image needs", image_job.fetch("needs"), "ci" ], [ "preview image permissions", image_job.fetch("permissions"), EXPECTED_IMAGE_PERMISSIONS ], [ "preview image timeout", image_job.fetch("timeout-minutes"), 30 ], @@ -240,6 +252,7 @@ comment_on_pr = step!(preview_comment_steps, "Comment on PR") [ "deploy HEAD_SHA env", deploy_job.dig("env", "HEAD_SHA"), "${{ needs.preview-gate.outputs.head_sha }}" ], [ "deploy IS_FORK env", deploy_job.dig("env", "IS_FORK"), "${{ needs.preview-gate.outputs.is_fork }}" ], [ "deploy PR_NUMBER env", deploy_job.dig("env", "PR_NUMBER"), "${{ needs.preview-gate.outputs.pr_number }}" ], + [ "deploy RESOLUTION_SOURCE env", deploy_job.dig("env", "RESOLUTION_SOURCE"), "${{ needs.preview-gate.outputs.resolution_source }}" ], [ "deployment status needs", deployment_status_job.fetch("needs"), [ "preview-gate", "deployment_record", "deploy-preview" ] ], [ "deployment status permissions", deployment_status_job.fetch("permissions"), EXPECTED_DEPLOYMENT_PERMISSIONS ], [ "deployment status timeout", deployment_status_job.fetch("timeout-minutes"), 5 ], @@ -271,6 +284,16 @@ comment_on_pr = step!(preview_comment_steps, "Comment on PR") [ "diagnostics upload name", upload_diagnostics.dig("with", "name"), "preview-diagnostics-pr-${{ env.PR_NUMBER }}-${{ env.HEAD_SHA }}" ], [ "diagnostics upload path", upload_diagnostics.dig("with", "path"), "${{ runner.temp }}/preview-diagnostics.json" ], [ "diagnostics upload retention", upload_diagnostics.dig("with", "retention-days"), 3 ], + [ "failure diagnostics collect if", collect_failure_diagnostics.fetch("if"), "failure()" ], + [ "failure diagnostics upload if", upload_failure_diagnostics.fetch("if"), "failure()" ], + [ "failure diagnostics upload name", upload_failure_diagnostics.dig("with", "name"), "preview-failure-diagnostics-pr-${{ env.PR_NUMBER }}-${{ env.HEAD_SHA }}" ], + [ "failure diagnostics upload path", upload_failure_diagnostics.dig("with", "path"), EXPECTED_FAILURE_DIAGNOSTICS_PATH ], + [ "failure diagnostics upload retention", upload_failure_diagnostics.dig("with", "retention-days"), 3 ], + [ "cleanup metadata prepare if", prepare_cleanup_metadata.fetch("if"), "success()" ], + [ "cleanup metadata upload if", store_cleanup_metadata.fetch("if"), "success()" ], + [ "cleanup metadata upload name", store_cleanup_metadata.dig("with", "name"), "preview-cleanup-pr-${{ env.PR_NUMBER }}" ], + [ "cleanup metadata upload path", store_cleanup_metadata.dig("with", "path"), EXPECTED_CLEANUP_METADATA_PATH ], + [ "cleanup metadata upload retention", store_cleanup_metadata.dig("with", "retention-days"), 2 ], [ "Wrangler binary", wrangler.dig("bin", "wrangler"), "bin/wrangler.js" ] ].each { |label, actual, expected| assert(actual == expected, "#{label}: expected #{actual.inspect} to equal #{expected.inspect}") } @@ -298,7 +321,28 @@ assert(gate_trusted_checkout.dig("with", "sparse-checkout").to_s.include?("worke assert(trusted_checkout.dig("with", "sparse-checkout").to_s.include?("workers/preview"), "trusted checkout must include preview tooling") assert(deploy_step_names.compact.uniq == deploy_step_names.compact, "workflow step names must stay unique for security checks") assert([ gate_trusted_checkout, resolve_preview ].map { |step| gate_steps.index(step) }.each_cons(2).all? { |left, right| left < right }, "gate workflow steps must checkout trusted resolver before use") -assert([ trusted_checkout, download_artifact, verify_checksum, prepare, load_image, push_image, configure_image, deploy, warm_preview, collect_diagnostics, upload_diagnostics ].map { |step| deploy_steps.index(step) }.each_cons(2).all? { |left, right| left < right }, "deploy workflow steps must preserve safe cross-run artifact deploy order") +required_deploy_order = [ + trusted_checkout, + download_artifact, + verify_checksum, + prepare, + load_image, + push_image, + configure_image, + deploy, + warm_preview, + collect_diagnostics, + upload_diagnostics, + collect_failure_diagnostics, + upload_failure_diagnostics, + prepare_cleanup_metadata, + store_cleanup_metadata +] +assert( + required_deploy_order.map { |step| deploy_steps.index(step) } + .each_cons(2).all? { |left, right| left < right }, + "deploy workflow steps must preserve safe cross-run artifact deploy order" +) assert(deploy_steps.none? { |step| step["name"] == "Checkout PR code" }, "privileged deploy job must not checkout PR code") assert(env_hash(deploy_job).keys.none? { |name| name.start_with?("CLOUDFLARE_") }, "Cloudflare secrets must not be job-wide") assert(env_hash(gate_job).keys.none? { |name| name.start_with?("CLOUDFLARE_") }, "preview gate must not receive Cloudflare secrets") @@ -344,6 +388,9 @@ assert_run_includes( "workflowRun.pull_requests?.[0]", "github.rest.repos.listPullRequestsAssociatedWithCommit", "parsePreviewArtifactName", + "artifactPullRequestNumbers.length === 1", + "conflicts with workflow_run PR", + "conflicts with commit-associated PRs", "pullRequest.head.sha !== headSha", "is stale for PR", "preview-cf", @@ -352,9 +399,30 @@ assert_run_includes( "!item.expired", "core.setOutput(\"artifact_name\", artifactName)", "core.setOutput(\"is_fork\", String(isFork))", + "core.setOutput(\"resolution_source\", selected.source)", "core.setOutput(\"should_deploy\", \"true\")" ].each { |needle| assert(resolver_script.include?(needle), "preview resolver must include #{needle.inspect}") } +artifact_resolution_index = resolver_script.index("artifactPullRequestNumbers.length === 1") +workflow_run_fallback_index = resolver_script.index("if (workflowRunPullRequestNumber)") +assert(artifact_resolution_index, "preview resolver must inspect exact preview artifacts") +assert(workflow_run_fallback_index, "preview resolver must retain workflow_run PR fallback") +assert( + artifact_resolution_index < workflow_run_fallback_index, + "preview resolver must prefer exact preview artifacts before workflow_run PR metadata" +) +assert(File.executable?(REDACTION_HELPER_PATH), "preview log redaction helper must be executable") +[ + "registry\\.cloudflare\\.com/[^/]+/", + "Authorization|Proxy-Authorization", + "X-Auth-Key|X-Auth-Email|X-Api-Key|Api-Key", + "api_key|access_token|refresh_token|auth_token|key|private_key", + "CLOUDFLARE_ACCOUNT_ID=", + "CLOUDFLARE_API_TOKEN|API_KEY|ACCESS_TOKEN|REFRESH_TOKEN|AUTH_TOKEN|PRIVATE_KEY", + "", + "" +].each { |needle| assert(redaction_helper_script.include?(needle), "preview log redaction helper must include #{needle.inspect}") } + prepare_run = assert_run_includes(prepare, *REQUIRED_PREPARE_LINES) assert(!prepare_run.include?("npm install"), "prepare step must not use npm install") assert(!prepare_run.include?("CLOUDFLARE_API_TOKEN"), "prepare step must not receive Cloudflare secrets") @@ -371,24 +439,31 @@ assert_run_includes(load_image, 'gzip -dc "$image_archive" | docker load', 'dock push_image_run = assert_run_includes( push_image, "./node_modules/.bin/wrangler containers push", - "registry\\.cloudflare\\.com/", + "registry.cloudflare.com/${CLOUDFLARE_ACCOUNT_ID}/${image_tag}", + "registry\\.cloudflare\\.com\\/", "image_ref=", 'config_path="$RUNNER_TEMP/sure-preview-worker/wrangler.toml"', - 'LOCAL_IMAGE_TAG="$image_tag" node - "$config_path"', - "Expected local preview image tag for wrangler containers push", + 'temporary_image_ref="registry.cloudflare.com/${CLOUDFLARE_ACCOUNT_ID}/${image_tag}"', + 'TEMPORARY_IMAGE_REF="$temporary_image_ref" node - "$config_path"', + "Expected registry-shaped preview image ref before wrangler containers push", "Expected wrangler.toml to contain an image entry to rewrite before push" ) -push_rewrite_index = push_image_run.index('LOCAL_IMAGE_TAG="$image_tag" node - "$config_path"') +push_rewrite_index = push_image_run.index('TEMPORARY_IMAGE_REF="$temporary_image_ref" node - "$config_path"') push_command_index = push_image_run.index("./node_modules/.bin/wrangler containers push") assert( push_rewrite_index < push_command_index, - "push step must rewrite wrangler.toml to the loaded local image tag before wrangler validates it" + "push step must rewrite wrangler.toml to a registry-shaped image ref before wrangler validates it" ) -assert_run_includes(configure_image, "imageRef.startsWith('registry.cloudflare.com/')", 'const original = fs.readFileSync', 'const updated = original.replace(/image = "[^"]+"/', "updated === original", "Expected wrangler.toml to contain an image entry to rewrite", "JSON.stringify(imageRef)") +assert(!push_image_run.include?("LOCAL_IMAGE_TAG"), "push step must not write a local Docker tag into wrangler.toml") +assert(!push_image_run.include?("Expected local preview image tag"), "push step must not accept local Docker tags as wrangler config image refs") +assert_run_includes(push_image, 'tee "$push_log" | ./deploy/redact_preview_log.sh', "push_status=${PIPESTATUS[0]}") +assert_run_includes(configure_image, "registry\\.cloudflare\\.com", "expectedSuffix", "imageRef.endsWith", "Cloudflare registry image reference does not match this preview artifact", 'const original = fs.readFileSync', 'const updated = original.replace(/image = "[^"]+"/', "updated === original", "Expected wrangler.toml to contain an image entry to rewrite", "JSON.stringify(imageRef)", 'redact_preview_log.sh" < "$config_path"') assert_run_includes(create_deployment, "github.rest.repos.createDeployment", "ref: headSha", "preview-pr-${prNumber}") -assert_run_includes(deploy, 'cd "$RUNNER_TEMP/sure-preview-worker"', "deploy_once()", "./node_modules/.bin/wrangler deploy --config wrangler.toml", '--var "PR_NUMBER:${PR_NUMBER}"', "associated with a different durable object namespace", 'if ! ./node_modules/.bin/wrangler delete --name "sure-preview-${PR_NUMBER}" --force', "Preview Worker delete failed", "retrying once") +assert_run_includes(deploy, 'cd "$RUNNER_TEMP/sure-preview-worker"', "deploy_once()", "./node_modules/.bin/wrangler deploy --config wrangler.toml", '--var "PR_NUMBER:${PR_NUMBER}"', 'tee "$deploy_log" | ./deploy/redact_preview_log.sh', "deploy_status=${PIPESTATUS[0]}", "associated with a different durable object namespace", 'if ! ./node_modules/.bin/wrangler delete --name "sure-preview-${PR_NUMBER}" --force', "Preview Worker delete failed", "retrying once") assert_run_includes(warm_preview, "$PREVIEW_URL/_container_status", "--connect-timeout 5", "--max-time 15") assert_run_includes(collect_diagnostics, "$PREVIEW_URL/_container_status", "--connect-timeout 5", "--max-time 15", "seq 1 40", "preview-diagnostics.json", "jq -e '.previewReady == true or .previewFailed == true'", "jq -e '.previewFailed == true'", "Preview diagnostics from _container_status reported previewFailed=true", "jq -e '.previewReady == true'", "Preview diagnostics from _container_status did not reach previewReady=true", ".timings.previewReadyAt != null and .timings.secondsToPreviewReady != null", "Preview diagnostics are missing readiness timing fields", "exit 1") +assert_run_includes(collect_failure_diagnostics, "preview-failure-diagnostics", "preview-request.json", "preview-image-manifest.json", "wrangler.toml", "wrangler-containers-push.log", "wrangler-deploy.log", "redaction_helper=", 'sanitize_copy "$RUNNER_TEMP/sure-preview-worker/wrangler.toml"', "wrangler-deploy.clean.log", "resolutionSource") +assert_run_includes(prepare_cleanup_metadata, "preview-cleanup-metadata", "redact_preview_log.sh", "$RUNNER_TEMP/sure-preview-worker/wrangler.toml", "$metadata_dir/wrangler.toml") assert_run_includes(update_deployment_status, "github.rest.repos.createDeploymentStatus", "process.env.DEPLOY_RESULT === 'success'", "deployment_id: Number(process.env.DEPLOYMENT_ID)") assert_run_includes(comment_on_pr, "github.rest.issues.listComments", "github.rest.issues.updateComment", "github.rest.issues.createComment", "Preview Deployment Ready") diff --git a/db/schema.rb b/db/schema.rb index ddaa60b71..513bd1abd 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -53,7 +53,6 @@ ActiveRecord::Schema[7.2].define(version: 2026_05_31_153000) do t.string "content_type", limit: 100, null: false t.bigint "byte_size", null: false t.string "checksum", limit: 64, null: false - t.string "content_sha256" t.string "source", default: "manual_upload", null: false t.string "upload_status", default: "stored", null: false t.string "institution_name_hint", limit: 200 @@ -70,6 +69,7 @@ ActiveRecord::Schema[7.2].define(version: 2026_05_31_153000) do t.jsonb "sanitized_parser_output", default: {}, null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.string "content_sha256" t.index ["account_id", "period_start_on", "period_end_on"], name: "index_account_statements_on_account_period" t.index ["account_id"], name: "index_account_statements_on_account_id" t.index ["family_id", "checksum"], name: "index_account_statements_on_family_checksum" @@ -117,6 +117,7 @@ ActiveRecord::Schema[7.2].define(version: 2026_05_31_153000) do t.string "institution_domain" t.text "notes" t.uuid "owner_id" + t.integer "account_providers_count", default: 0, null: false t.index ["accountable_id", "accountable_type"], name: "index_accounts_on_accountable_id_and_accountable_type" t.index ["accountable_type"], name: "index_accounts_on_accountable_type" t.index ["currency"], name: "index_accounts_on_currency" @@ -299,9 +300,9 @@ ActiveRecord::Schema[7.2].define(version: 2026_05_31_153000) do t.string "institution_domain" t.string "institution_url" t.string "institution_color" - t.string "status", default: "good" - t.boolean "scheduled_for_deletion", default: false - t.boolean "pending_account_setup", default: false + t.string "status", default: "good", null: false + t.boolean "scheduled_for_deletion", default: false, null: false + t.boolean "pending_account_setup", default: false, null: false t.datetime "sync_start_date" t.jsonb "raw_payload" t.text "api_key" @@ -637,12 +638,12 @@ ActiveRecord::Schema[7.2].define(version: 2026_05_31_153000) do t.index ["account_id", "date"], name: "index_entries_on_account_id_and_date" t.index ["account_id", "source", "external_id"], name: "index_entries_on_account_source_and_external_id", unique: true, where: "((external_id IS NOT NULL) AND (source IS NOT NULL))" t.index ["account_id"], name: "index_entries_on_account_id" + t.index ["currency", "amount", "date", "account_id"], name: "index_entries_on_transfer_match_lookup", where: "((entryable_type)::text = 'Transaction'::text)" t.index ["date"], name: "index_entries_on_date" t.index ["entryable_type"], name: "index_entries_on_entryable_type" t.index ["import_id"], name: "index_entries_on_import_id" t.index ["import_locked"], name: "index_entries_on_import_locked_true", where: "(import_locked = true)" t.index ["parent_entry_id"], name: "index_entries_on_parent_entry_id" - t.index ["currency", "amount", "date", "account_id"], name: "index_entries_on_transfer_match_lookup", where: "((entryable_type)::text = 'Transaction'::text)" t.index ["user_modified"], name: "index_entries_on_user_modified_true", where: "(user_modified = true)" end @@ -847,7 +848,7 @@ ActiveRecord::Schema[7.2].define(version: 2026_05_31_153000) do t.index ["family_id", "state"], name: "index_goals_on_family_id_and_state" t.index ["family_id"], name: "index_goals_on_family_id" t.check_constraint "char_length(name::text) <= 255", name: "chk_savings_goals_name_length" - t.check_constraint "state::text = ANY (ARRAY['active'::character varying::text, 'paused'::character varying::text, 'completed'::character varying::text, 'archived'::character varying::text])", name: "chk_savings_goals_state_enum" + t.check_constraint "state::text = ANY (ARRAY['active'::character varying, 'paused'::character varying, 'completed'::character varying, 'archived'::character varying]::text[])", name: "chk_savings_goals_state_enum" t.check_constraint "target_amount > 0::numeric", name: "chk_savings_goals_target_amount_positive" end @@ -884,9 +885,9 @@ ActiveRecord::Schema[7.2].define(version: 2026_05_31_153000) do t.decimal "current_balance", precision: 19, scale: 4 t.decimal "cash_balance", precision: 19, scale: 4 t.jsonb "institution_metadata" - t.jsonb "raw_holdings_payload", default: [], null: false - t.jsonb "raw_activities_payload", default: {}, null: false - t.jsonb "raw_cash_report_payload", default: [], null: false + t.jsonb "raw_holdings_payload", default: [] + t.jsonb "raw_activities_payload", default: {} + t.jsonb "raw_cash_report_payload", default: [] t.date "report_date" t.datetime "last_holdings_sync" t.datetime "last_activities_sync" @@ -900,8 +901,8 @@ ActiveRecord::Schema[7.2].define(version: 2026_05_31_153000) do create_table "ibkr_items", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| t.uuid "family_id", null: false t.string "name" - t.string "status", default: "good", null: false - t.boolean "scheduled_for_deletion", default: false, null: false + t.string "status", default: "good" + t.boolean "scheduled_for_deletion", default: false t.boolean "pending_account_setup", default: false, null: false t.jsonb "raw_payload" t.string "query_id" diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index 1102d6fc6..3a1adc911 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -4680,7 +4680,7 @@ paths: schema: "$ref": "#/components/schemas/ImportResponse" '422': - description: validation error - file too large + description: validation error or publish rejection content: application/json: schema: diff --git a/spec/requests/api/v1/imports_spec.rb b/spec/requests/api/v1/imports_spec.rb index 7d6691612..dc016df87 100644 --- a/spec/requests/api/v1/imports_spec.rb +++ b/spec/requests/api/v1/imports_spec.rb @@ -250,7 +250,7 @@ RSpec.describe 'API V1 Imports', type: :request do run_test! end - response '422', 'validation error - file too large' do + response '422', 'validation error or publish rejection' do schema oneOf: [ { '$ref' => '#/components/schemas/ErrorResponse' }, { '$ref' => '#/components/schemas/ErrorResponseWithImportId' } @@ -269,9 +269,22 @@ RSpec.describe 'API V1 Imports', type: :request do response '500', 'import uploaded but publish enqueue failed' do schema '$ref' => '#/components/schemas/ErrorResponseWithImportId' + before do + allow(ImportJob).to receive(:perform_later).and_raise(StandardError, 'queue offline') + end + let(:body) do { - raw_file_content: { type: 'Account', data: { id: 'account_1', name: 'Checking' } }.to_json, + raw_file_content: { + type: 'Account', + data: { + id: 'account_1', + name: 'Checking', + balance: '100', + currency: 'USD', + accountable_type: 'Depository' + } + }.to_json, type: 'SureImport', publish: 'true' } diff --git a/test/components/DS/pill_test.rb b/test/components/DS/pill_test.rb index a066e6e94..b66b0a200 100644 --- a/test/components/DS/pill_test.rb +++ b/test/components/DS/pill_test.rb @@ -51,6 +51,26 @@ class DS::PillTest < ViewComponent::TestCase assert_includes pill.palette[:bg], "color-red-50" end + test "marker mode shows the dot by default" do + render_inline(DS::Pill.new(label: "Beta", tone: :violet)) + assert_selector "span.inline-block.rounded-full" + end + + test "badge mode (marker: false) is dot-less by default" do + render_inline(DS::Pill.new(label: "Member", tone: :neutral, marker: false)) + assert_no_selector "span.inline-block.rounded-full" + end + + test "badge mode opts back into the dot with show_dot: true" do + render_inline(DS::Pill.new(label: "Active", tone: :success, marker: false, show_dot: true)) + assert_selector "span.inline-block.rounded-full" + end + + test "marker mode can drop the dot with show_dot: false" do + render_inline(DS::Pill.new(label: "Beta", tone: :violet, show_dot: false)) + assert_no_selector "span.inline-block.rounded-full" + end + test "custom color renders dynamic badge styles" do render_inline(DS::Pill.new(label: "Groceries", marker: false, custom_color: "#f97316")) diff --git a/test/components/DS/progress_ring_test.rb b/test/components/DS/progress_ring_test.rb new file mode 100644 index 000000000..14ca25f95 --- /dev/null +++ b/test/components/DS/progress_ring_test.rb @@ -0,0 +1,45 @@ +require "test_helper" + +class DS::ProgressRingTest < ViewComponent::TestCase + test "renders a track circle and a progress arc" do + render_inline(DS::ProgressRing.new(percent: 50, tone: :success)) + assert_selector "svg circle", count: 2 + end + + test "renders the center percent by default and clamps it" do + render_inline(DS::ProgressRing.new(percent: 140)) + assert_text "100%" + + render_inline(DS::ProgressRing.new(percent: -10)) + assert_text "0%" + end + + test "show_percent: false omits the center label" do + render_inline(DS::ProgressRing.new(percent: 40, show_percent: false)) + assert_no_text "40%" + end + + test "exposes a progressbar role and value only when labelled" do + render_inline(DS::ProgressRing.new(percent: 30, label: "Goal progress")) + assert_selector "[role='progressbar'][aria-valuenow='30'][aria-label='Goal progress']" + + render_inline(DS::ProgressRing.new(percent: 30)) + assert_no_selector "[role='progressbar']" + end + + test "tone selects the arc stroke color token" do + assert_equal "var(--color-success)", DS::ProgressRing.new(percent: 1, tone: :success).stroke_color + assert_equal "var(--color-warning)", DS::ProgressRing.new(percent: 1, tone: :warning).stroke_color + assert_equal "var(--color-destructive)", DS::ProgressRing.new(percent: 1, tone: :destructive).stroke_color + # Unknown tone falls back to neutral. + assert_equal "var(--color-gray-400)", DS::ProgressRing.new(percent: 1, tone: :bogus).stroke_color + end + + test "dash offset runs from full circumference at 0% to zero at 100%" do + ring = DS::ProgressRing.new(percent: 0) + assert_in_delta ring.circumference, ring.dash_offset, 0.001 + + full = DS::ProgressRing.new(percent: 100) + assert_in_delta 0.0, full.dash_offset, 0.001 + end +end diff --git a/test/components/previews/pill_component_preview.rb b/test/components/previews/pill_component_preview.rb index ae036179c..fce55a602 100644 --- a/test/components/previews/pill_component_preview.rb +++ b/test/components/previews/pill_component_preview.rb @@ -39,8 +39,12 @@ class PillComponentPreview < ViewComponent::Preview # @!endgroup # @!group Status badges (marker: false, semantic tones) + # Badge mode is dot-less by default — tone + label carry the signal. Opt the + # dot back in with show_dot: true only where it's genuinely additive (live / + # temporal status, or a single sparse pill). status_active below shows the + # opt-in; status_pending / status_archived show the clean default. def status_active - render DS::Pill.new(label: "Active", tone: :success, marker: false) + render DS::Pill.new(label: "Active", tone: :success, marker: false, show_dot: true) end def status_pending diff --git a/test/components/previews/progress_ring_component_preview.rb b/test/components/previews/progress_ring_component_preview.rb new file mode 100644 index 000000000..b4476500c --- /dev/null +++ b/test/components/previews/progress_ring_component_preview.rb @@ -0,0 +1,68 @@ +class ProgressRingComponentPreview < ViewComponent::Preview + # @param percent number + # @param size number + # @param stroke_width number + # @param tone select ["success", "warning", "destructive", "neutral"] + # @param show_percent toggle + # @param label text + def default(percent: 65, size: 64, stroke_width: 6, tone: "neutral", show_percent: true, label: nil) + render DS::ProgressRing.new( + percent: percent, + size: size, + stroke_width: stroke_width, + tone: tone.to_sym, + show_percent: show_percent, + label: label.presence + ) + end + + # @!group Tones (50%) + def success + render DS::ProgressRing.new(percent: 50, tone: :success) + end + + def warning + render DS::ProgressRing.new(percent: 50, tone: :warning) + end + + def destructive + render DS::ProgressRing.new(percent: 50, tone: :destructive) + end + + def neutral + render DS::ProgressRing.new(percent: 50, tone: :neutral) + end + # @!endgroup + + # @!group Sizes + def small_48 + render DS::ProgressRing.new(percent: 72, size: 48, stroke_width: 5, tone: :success) + end + + def medium_64 + render DS::ProgressRing.new(percent: 72, size: 64, stroke_width: 6, tone: :success) + end + + def large_180 + render DS::ProgressRing.new(percent: 72, size: 180, stroke_width: 10, tone: :success) + end + # @!endgroup + + # @!group Edges + def empty_0 + render DS::ProgressRing.new(percent: 0, tone: :neutral) + end + + def full_100 + render DS::ProgressRing.new(percent: 100, tone: :success) + end + + def clamps_over_100 + render DS::ProgressRing.new(percent: 140, tone: :success) + end + + def without_center_percent + render DS::ProgressRing.new(percent: 40, tone: :warning, show_percent: false) + end + # @!endgroup +end diff --git a/test/controllers/api/v1/imports_controller_test.rb b/test/controllers/api/v1/imports_controller_test.rb index 41d6c2e43..e7bb3579f 100644 --- a/test/controllers/api/v1/imports_controller_test.rb +++ b/test/controllers/api/v1/imports_controller_test.rb @@ -524,7 +524,16 @@ class Api::V1::ImportsControllerTest < ActionDispatch::IntegrationTest end test "should preserve Sure import if publish queueing fails" do - ndjson_content = { type: "Account", data: { id: "account_1", name: "Checking" } }.to_json + ndjson_content = { + type: "Account", + data: { + id: "account_1", + name: "Checking", + balance: "100", + currency: "USD", + accountable_type: "Depository" + } + }.to_json ImportJob.stubs(:perform_later).raises(StandardError, "queue offline") assert_difference("Import.count") do @@ -548,6 +557,91 @@ class Api::V1::ImportsControllerTest < ActionDispatch::IntegrationTest assert_equal "pending", import.status end + test "should preserve Sure import and return preflight errors when auto publish fails preflight" do + @family.categories.create!( + name: "Groceries", + color: "#407706", + lucide_icon: "shopping-basket" + ) + ndjson_content = [ + { type: "Category", data: { id: "category_1", name: "Groceries" } } + ].map(&:to_json).join("\n") + + assert_difference("Import.count") do + post api_v1_imports_url, + params: { + type: "SureImport", + raw_file_content: ndjson_content, + publish: "true" + }, + headers: api_headers(@api_key) + end + + assert_response :unprocessable_entity + json_response = JSON.parse(response.body) + assert_equal "preflight_failed", json_response["error"] + assert_includes json_response["errors"].join("\n"), "Category name \"Groceries\" already exists" + + import = Import.find(json_response["import_id"]) + assert_equal "failed", import.status + assert import.ndjson_file.attached? + end + + test "should preserve Sure import and return not publishable when auto publish has no records" do + ndjson_content = { type: "Account", data: { id: "account_1", name: "Checking" } }.to_json + SureImport.any_instance.stubs(:publish_later).raises( + SureImport::NotPublishableError, + "raw publishability failure with internal state" + ) + + assert_difference("Import.count") do + post api_v1_imports_url, + params: { + type: "SureImport", + raw_file_content: ndjson_content, + publish: "true" + }, + headers: api_headers(@api_key) + end + + assert_response :unprocessable_entity + json_response = JSON.parse(response.body) + assert_equal "not_publishable", json_response["error"] + assert_equal "Import was uploaded but has no publishable records.", json_response["message"] + assert_not json_response.key?("errors") + refute_includes response.body, "raw publishability failure" + + import = Import.find(json_response["import_id"]) + assert_instance_of SureImport, import + assert import.ndjson_file.attached? + assert_equal 1, import.rows_count + assert_equal "pending", import.status + end + + test "should return unsupported Sure record errors during auto publish preflight" do + ndjson_content = [ + { type: "MysteryType", data: { id: "mystery_1" } } + ].map(&:to_json).join("\n") + + assert_difference("Import.count") do + post api_v1_imports_url, + params: { + type: "SureImport", + raw_file_content: ndjson_content, + publish: "true" + }, + headers: api_headers(@api_key) + end + + assert_response :unprocessable_entity + json_response = JSON.parse(response.body) + assert_equal "preflight_failed", json_response["error"] + assert_includes json_response["errors"].join("\n"), "unsupported record type MysteryType" + + import = Import.find(json_response["import_id"]) + assert_equal "failed", import.status + end + test "should preserve Sure import if auto publish exceeds row count" do ndjson_content = { type: "Account", data: { id: "account_1", name: "Checking" } }.to_json SureImport.any_instance.stubs(:publish_later).raises(Import::MaxRowCountExceededError) @@ -740,8 +834,8 @@ class Api::V1::ImportsControllerTest < ActionDispatch::IntegrationTest assert_equal "invalid_csv", json_response["error"] end - test "should include preflight exception message in internal server error response" do - Import::Preflight.any_instance.stubs(:call).raises(StandardError, "boom") + test "should hide preflight exception message in internal server error response" do + Import::Preflight.any_instance.stubs(:call).raises(StandardError, "boom with raw internals") post preflight_api_v1_imports_url, params: { @@ -755,7 +849,8 @@ class Api::V1::ImportsControllerTest < ActionDispatch::IntegrationTest assert_response :internal_server_error json_response = JSON.parse(response.body) assert_equal "internal_server_error", json_response["error"] - assert_equal "Error: boom", json_response["message"] + assert_equal "Import preflight could not be completed.", json_response["message"] + refute_includes response.body, "boom with raw internals" end test "should reject unknown preflight import type" do @@ -816,8 +911,19 @@ class Api::V1::ImportsControllerTest < ActionDispatch::IntegrationTest test "should preflight Sure import without persisting records" do ndjson_content = [ - { type: "Account", data: { id: "account_1", name: "Checking" } }.to_json, - { type: "Transaction", data: { id: "entry_1", account_id: "account_1" } }.to_json + { type: "Account", data: { + id: "account_1", + name: "Checking", + balance: "100", + currency: "USD", + accountable_type: "Depository" + } }.to_json, + { type: "Transaction", data: { + id: "entry_1", + account_id: "account_1", + date: "2024-01-01", + amount: "-5" + } }.to_json ].join("\n") assert_no_difference("Import.count") do @@ -840,6 +946,55 @@ class Api::V1::ImportsControllerTest < ActionDispatch::IntegrationTest assert_empty data["errors"] end + test "should preflight Sure import taxonomy collisions in strict mode" do + @family.tags.create!(name: "Reviewed", color: "#12B76A") + ndjson_content = [ + { type: "Tag", data: { id: "tag_1", name: "Reviewed" } } + ].map(&:to_json).join("\n") + + assert_no_difference("Import.count") do + post preflight_api_v1_imports_url, + params: { + type: "SureImport", + raw_file_content: ndjson_content + }, + headers: api_headers(@api_key) + end + + assert_response :success + data = JSON.parse(response.body)["data"] + assert_equal false, data["valid"] + assert_equal "existing_taxonomy_collision", data["errors"].first["code"] + end + + test "should report invalid Sure import accountable type during preflight" do + ndjson_content = [ + { type: "Account", data: { + id: "account_1", + name: "Checking", + balance: "100", + currency: "USD", + accountable_type: "Kernel" + } }.to_json + ].join("\n") + + assert_no_difference("Import.count") do + post preflight_api_v1_imports_url, + params: { + type: "SureImport", + raw_file_content: ndjson_content + }, + headers: api_headers(@api_key) + end + + assert_response :success + data = JSON.parse(response.body)["data"] + + assert_equal false, data["valid"] + assert_equal "invalid_accountable_type", data["errors"].first["code"] + assert_includes data["errors"].first["message"], "Kernel" + end + test "should report invalid Sure import NDJSON during preflight" do assert_no_difference("Import.count") do post preflight_api_v1_imports_url, diff --git a/test/controllers/sessions_controller_test.rb b/test/controllers/sessions_controller_test.rb index d3ec232ef..e2a201974 100644 --- a/test/controllers/sessions_controller_test.rb +++ b/test/controllers/sessions_controller_test.rb @@ -112,9 +112,10 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest assert_match %r{/auth/openid_connect}, @response.body assert_match /Sign in with Keycloak/, @response.body - # Google-branded button + # Google-branded button — DS outline button carrying Google's official + # multi-color "G" mark (one of its brand hexes proves the inline SVG rendered). assert_match %r{/auth/google_oauth2}, @response.body - assert_match /gsi-material-button/, @response.body + assert_match /#4285F4/i, @response.body assert_match /Sign in with Google/, @response.body end diff --git a/test/javascript/preview_deploy/resolve_preview_request_test.cjs b/test/javascript/preview_deploy/resolve_preview_request_test.cjs index 7dc799c7a..a3fb32a1f 100644 --- a/test/javascript/preview_deploy/resolve_preview_request_test.cjs +++ b/test/javascript/preview_deploy/resolve_preview_request_test.cjs @@ -123,7 +123,7 @@ describe("selectPullRequestNumber", () => { const headSha = "4f1159e99c7785bc370f53510284c251fabdb75b"; const context = contextFor({ id: 123, head_sha: headSha }); - it("falls back to commit association when workflow_run has no PR payload", () => { + it("prefers the preview artifact when commit association matches", () => { const selected = selectPullRequestNumber({ runPullRequest: undefined, artifacts: [previewArtifact(2017, headSha)], @@ -134,7 +134,37 @@ describe("selectPullRequestNumber", () => { assert.deepEqual(selected, { prNumber: 2017, - source: "commit_association", + source: "artifact_name+commit_association", + }); + }); + + it("records workflow_run as a corroborating source when it matches the preview artifact", () => { + const selected = selectPullRequestNumber({ + runPullRequest: { number: 2017 }, + artifacts: [previewArtifact(2017, headSha)], + associatedPullRequests: [], + context, + headSha, + }); + + assert.deepEqual(selected, { + prNumber: 2017, + source: "artifact_name+workflow_run", + }); + }); + + it("records workflow_run and commit association when both match the preview artifact", () => { + const selected = selectPullRequestNumber({ + runPullRequest: { number: 2017 }, + artifacts: [previewArtifact(2017, headSha)], + associatedPullRequests: [openPullRequest(2017, headSha, "Rene0422/sure")], + context, + headSha, + }); + + assert.deepEqual(selected, { + prNumber: 2017, + source: "artifact_name+workflow_run+commit_association", }); }); @@ -152,10 +182,38 @@ describe("selectPullRequestNumber", () => { assert.deepEqual(selected, { prNumber: 2060, - source: "artifact_and_commit_association", + source: "artifact_name+commit_association", }); }); + it("fails closed when workflow metadata disagrees with the preview artifact", () => { + const selected = selectPullRequestNumber({ + runPullRequest: { number: 1985 }, + artifacts: [previewArtifact(1798, headSha)], + associatedPullRequests: [openPullRequest(1798, headSha)], + context, + headSha, + }); + + assert.equal(selected.prNumber, undefined); + assert.equal(typeof selected.error, "string"); + assert.match(selected.error, /conflicts with workflow_run PR 1985/); + }); + + it("fails closed when commit association disagrees with the preview artifact", () => { + const selected = selectPullRequestNumber({ + runPullRequest: undefined, + artifacts: [previewArtifact(1798, headSha)], + associatedPullRequests: [openPullRequest(1985, headSha)], + context, + headSha, + }); + + assert.equal(selected.prNumber, undefined); + assert.equal(typeof selected.error, "string"); + assert.match(selected.error, /conflicts with commit-associated PRs 1985/); + }); + it("refuses ambiguous associated PRs without a single matching artifact", () => { const selected = selectPullRequestNumber({ runPullRequest: undefined, @@ -197,6 +255,7 @@ describe("resolvePreviewRequest", () => { assert.equal(state.outputs.head_sha, headSha); assert.equal(state.outputs.artifact_name, `preview-image-pr-2017-${headSha}`); assert.equal(state.outputs.is_fork, "true"); + assert.equal(state.outputs.resolution_source, "artifact_name+commit_association"); }); it("resolves PRs from artifact names when workflow and commit association metadata are unavailable", async () => { @@ -216,6 +275,7 @@ describe("resolvePreviewRequest", () => { assert.equal(state.outputs.head_sha, headSha); assert.equal(state.outputs.artifact_name, `preview-image-pr-2017-${headSha}`); assert.equal(state.outputs.is_fork, "true"); + assert.equal(state.outputs.resolution_source, "artifact_name"); assert.match(state.messages.join("\n"), /Resolved PR 2017 from artifact_name; fork=true/); }); diff --git a/test/models/family/data_exporter_test.rb b/test/models/family/data_exporter_test.rb index 206c5e9d6..83e4ea8cc 100644 --- a/test/models/family/data_exporter_test.rb +++ b/test/models/family/data_exporter_test.rb @@ -705,16 +705,24 @@ class Family::DataExporterTest < ActiveSupport::TestCase currency: "USD" ) - split_parent_outflow = create_transaction_entry(@account, amount: 60, date: Date.parse("2024-01-25"), name: "Split transfer parent") - split_parent_outflow.split!([ - { name: "Split transfer child", amount: 60, category_id: @category.id } + split_parent_outflow = create_transaction_entry(@account, amount: 104, date: Date.parse("2024-01-25"), name: "Split transfer parent") + split_children = split_parent_outflow.split!([ + { name: "Split transfer child", amount: 100, category_id: @category.id }, + { name: "Split fee child", amount: 4, category_id: @category.id } ]) - transfer_inflow = create_transaction_entry(destination_account, amount: -60, date: Date.parse("2024-01-25"), name: "Split transfer inflow") - transfer = Transfer.create!( + parent_transfer_inflow = create_transaction_entry(destination_account, amount: -104, date: Date.parse("2024-01-25"), name: "Split parent transfer inflow") + parent_transfer = Transfer.create!( outflow_transaction: split_parent_outflow.entryable, - inflow_transaction: transfer_inflow.entryable, + inflow_transaction: parent_transfer_inflow.entryable, status: "confirmed" ) + child_transfer_inflow = create_transaction_entry(destination_account, amount: -100, date: Date.parse("2024-01-25"), name: "Split child transfer inflow") + child_transfer = Transfer.create!( + outflow_transaction: split_children.first.entryable, + inflow_transaction: child_transfer_inflow.entryable, + status: "confirmed", + notes: "Transfer uses split child" + ) zip_data = @exporter.generate_export @@ -728,8 +736,20 @@ class Family::DataExporterTest < ActiveSupport::TestCase .select { |record| record["type"] == "Transfer" } .map { |record| record.dig("data", "id") } - assert_not_includes transaction_ids, split_parent_outflow.entryable.id - assert_not_includes transfer_ids, transfer.id + assert_includes transaction_ids, split_parent_outflow.entryable.id + split_parent_data = ndjson_records.find do |record| + record["type"] == "Transaction" && record.dig("data", "id") == split_parent_outflow.entryable.id + end + assert_equal 2, split_parent_data.dig("data", "split_lines").count + assert_equal [ "Split transfer child", "Split fee child" ], split_parent_data.dig("data", "split_lines").map { |line| line["name"] } + refute_includes transaction_ids, split_children.first.entryable.id + refute_includes transaction_ids, split_children.second.entryable.id + assert_not_includes transfer_ids, parent_transfer.id + + child_transfer_data = ndjson_records.find { |record| record["type"] == "Transfer" && record.dig("data", "id") == child_transfer.id } + assert child_transfer_data + assert_equal split_children.first.entryable.id, child_transfer_data.dig("data", "outflow_transaction_id") + assert_equal child_transfer_inflow.entryable.id, child_transfer_data.dig("data", "inflow_transaction_id") end end diff --git a/test/models/family/data_importer_test.rb b/test/models/family/data_importer_test.rb index c6c75dda1..0ad0c9870 100644 --- a/test/models/family/data_importer_test.rb +++ b/test/models/family/data_importer_test.rb @@ -352,6 +352,42 @@ class Family::DataImporterTest < ActiveSupport::TestCase assert_equal 5000.0, opening_anchors.first.entry.amount.to_f end + test "skips synthesized opening anchor for authoritative balance history imports" do + ndjson = build_ndjson([ + { + type: "Account", + data: { + id: "acct-1", + name: "Imported With Full History", + balance: "5000", + currency: "USD", + accountable_type: "Depository", + authoritative_balance_history: true + } + }, + { + type: "Valuation", + data: { + id: "val-1", + account_id: "acct-1", + date: "2020-04-01", + amount: "4900", + name: "Imported balance", + currency: "USD", + kind: "reconciliation" + } + } + ]) + + Family::DataImporter.new(@family, ndjson).import! + + account = @family.accounts.find_by!(name: "Imported With Full History") + + assert_equal 5000.0, account.balance.to_f + assert_empty account.valuations.opening_anchor + assert_equal 1, account.valuations.reconciliation.count + end + test "imports categories with parent relationships" do ndjson = build_ndjson([ { @@ -732,6 +768,183 @@ class Family::DataImporterTest < ActiveSupport::TestCase assert_equal "Weekly groceries", transaction.entry.notes end + test "imports native split lines and lets transfers reference split children" do + ndjson = build_ndjson([ + { + type: "Account", + data: { + id: "checking", + name: "Checking", + balance: "1000", + currency: "USD", + accountable_type: "Depository" + } + }, + { + type: "Account", + data: { + id: "wallet", + name: "Wallet", + balance: "500", + currency: "USD", + accountable_type: "Depository" + } + }, + { + type: "Category", + data: { + id: "cat-fee", + name: "Bank Fees", + color: "#FF0000", + classification: "expense" + } + }, + { + type: "Tag", + data: { + id: "tag-imported", + name: "Imported" + } + }, + { + type: "Transaction", + data: { + id: "split-parent", + account_id: "checking", + date: "2024-01-15", + amount: "104.00", + name: "ATM withdrawal plus fee", + currency: "USD", + tag_ids: [ "tag-imported" ], + split_lines: [ + { + id: "split-transfer-leg", + amount: "100.00", + name: "Cash movement", + notes: "Transfer portion" + }, + { + id: "split-fee-line", + amount: "4.00", + name: "ATM fee", + category_id: "cat-fee", + notes: "Fee portion" + } + ] + } + }, + { + type: "Transaction", + data: { + id: "wallet-inflow", + account_id: "wallet", + date: "2024-01-15", + amount: "-100.00", + name: "Cash received", + currency: "USD" + } + }, + { + type: "Transfer", + data: { + id: "transfer-1", + inflow_transaction_id: "wallet-inflow", + outflow_transaction_id: "split-transfer-leg", + status: "confirmed", + notes: "Split-linked transfer" + } + } + ]) + + result = Family::DataImporter.new(@family, ndjson).import! + + parent_entry = @family.entries.find_by!(name: "ATM withdrawal plus fee") + assert parent_entry.split_parent? + assert_equal true, parent_entry.excluded + assert_equal 4, result[:entries].count + assert_includes result[:entries].map(&:id), parent_entry.id + + transfer_child = parent_entry.child_entries.find_by!(name: "Cash movement") + fee_child = parent_entry.child_entries.find_by!(name: "ATM fee") + assert_equal "Transfer portion", transfer_child.notes + assert_equal "Fee portion", fee_child.notes + assert_equal "Bank Fees", fee_child.transaction.category.name + assert_equal [ "Imported" ], transfer_child.transaction.tags.map(&:name) + assert_equal [ "Imported" ], fee_child.transaction.tags.map(&:name) + + transfer = Transfer.find_by!(notes: "Split-linked transfer") + assert_equal "confirmed", transfer.status + assert_equal "Cash movement", transfer.outflow_transaction.entry.name + assert_equal "Cash received", transfer.inflow_transaction.entry.name + end + + test "imports split lines without adding omitted parent taxonomy to explicit empty values" do + ndjson = build_ndjson([ + { + type: "Account", + data: { + id: "checking", + name: "Checking", + balance: "1000", + currency: "USD", + accountable_type: "Depository" + } + }, + { + type: "Tag", + data: { + id: "tag-parent", + name: "Parent tag" + } + }, + { + type: "Merchant", + data: { + id: "merchant-parent", + name: "Parent merchant" + } + }, + { + type: "Transaction", + data: { + id: "split-parent", + account_id: "checking", + date: "2024-01-15", + amount: "100.00", + name: "Tagged merchant split", + currency: "USD", + merchant_id: "merchant-parent", + tag_ids: [ "tag-parent" ], + split_lines: [ + { + id: "split-inherits", + amount: "40.00", + name: "Inherits omitted taxonomy" + }, + { + id: "split-empty", + amount: "60.00", + name: "Explicit empty taxonomy", + merchant_id: nil, + tag_ids: [] + } + ] + } + } + ]) + + Family::DataImporter.new(@family, ndjson).import! + + parent_entry = @family.entries.find_by!(name: "Tagged merchant split") + inherited_child = parent_entry.child_entries.find_by!(name: "Inherits omitted taxonomy") + explicit_empty_child = parent_entry.child_entries.find_by!(name: "Explicit empty taxonomy") + + assert_equal "Parent merchant", inherited_child.transaction.merchant.name + assert_equal [ "Parent tag" ], inherited_child.transaction.tags.map(&:name) + assert_nil explicit_empty_child.transaction.merchant + assert_empty explicit_empty_child.transaction.tags + end + test "imports trades with securities" do ndjson = build_ndjson([ { diff --git a/test/models/income_statement_test.rb b/test/models/income_statement_test.rb index 9e2b350f6..0f95fcff0 100644 --- a/test/models/income_statement_test.rb +++ b/test/models/income_statement_test.rb @@ -170,8 +170,8 @@ class IncomeStatementTest < ActiveSupport::TestCase # NOTE: These tests now pass because kind filtering is working after the refactoring! test "excludes regular transfers from income statement calculations" do # Create a regular transfer between accounts - _outflow_transaction = create_transaction(account: @checking_account, amount: 500, kind: "funds_movement") - _inflow_transaction = create_transaction(account: @credit_card_account, amount: -500, kind: "funds_movement") + create_transaction(account: @checking_account, amount: 500, kind: "funds_movement") + create_transaction(account: @credit_card_account, amount: -500, kind: "funds_movement") income_statement = IncomeStatement.new(@family) totals = income_statement.totals(date_range: Period.last_30_days.date_range) @@ -184,7 +184,7 @@ class IncomeStatementTest < ActiveSupport::TestCase test "includes loan payments as expenses in income statement" do # Create a loan payment transaction - _loan_payment = create_transaction(account: @checking_account, amount: 1000, category: nil, kind: "loan_payment") + create_transaction(account: @checking_account, amount: 1000, category: nil, kind: "loan_payment") income_statement = IncomeStatement.new(@family) totals = income_statement.totals(date_range: Period.last_30_days.date_range) @@ -197,7 +197,7 @@ class IncomeStatementTest < ActiveSupport::TestCase test "excludes one-time transactions from income statement calculations" do # Create a one-time transaction - _one_time_transaction = create_transaction(account: @checking_account, amount: 250, category: @groceries_category, kind: "one_time") + create_transaction(account: @checking_account, amount: 250, category: @groceries_category, kind: "one_time") income_statement = IncomeStatement.new(@family) totals = income_statement.totals(date_range: Period.last_30_days.date_range) @@ -210,7 +210,7 @@ class IncomeStatementTest < ActiveSupport::TestCase test "excludes payment transactions from income statement calculations" do # Create a payment transaction (credit card payment) - _payment_transaction = create_transaction(account: @checking_account, amount: 300, category: nil, kind: "cc_payment") + create_transaction(account: @checking_account, amount: 300, category: nil, kind: "cc_payment") income_statement = IncomeStatement.new(@family) totals = income_statement.totals(date_range: Period.last_30_days.date_range) @@ -304,7 +304,7 @@ class IncomeStatementTest < ActiveSupport::TestCase test "includes investment_contribution transactions as expenses in income statement" do # Create a transfer to investment account (marked as investment_contribution) - _investment_contribution = create_transaction( + create_transaction( account: @checking_account, amount: 1000, category: nil, @@ -334,7 +334,7 @@ class IncomeStatementTest < ActiveSupport::TestCase # Provider-imported contribution shows as inflow (negative amount) to the investment account # kind is investment_contribution, which should be treated as expense regardless of sign - _provider_contribution = create_transaction( + create_transaction( account: investment_account, amount: -500, # Negative = inflow to account category: nil, @@ -653,4 +653,22 @@ class IncomeStatementTest < ActiveSupport::TestCase assert_equal Money.new(0, "USD"), totals.income_money assert_equal Money.new(0, "USD"), totals.expense_money end + + test "reuses totals_query for income and expense in the same period" do + income_statement = IncomeStatement.new(@family) + period = Period.last_30_days + + totals_query_calls = 0 + income_statement.singleton_class.prepend(Module.new do + define_method(:totals_query) do |**kwargs| + totals_query_calls += 1 + super(**kwargs) + end + end) + + income_statement.income_totals(period: period) + income_statement.expense_totals(period: period) + + assert_equal 1, totals_query_calls + end end diff --git a/test/models/recurring_transaction_test.rb b/test/models/recurring_transaction_test.rb index 20c8a28ae..75d8ce596 100644 --- a/test/models/recurring_transaction_test.rb +++ b/test/models/recurring_transaction_test.rb @@ -998,12 +998,9 @@ class RecurringTransactionTest < ActiveSupport::TestCase assert_not RecurringTransaction.exists?(rt.id) end - test "Cleaner skips recurring transfers so they aren't mistakenly marked inactive" do - # `matching_transactions` is single-account name/amount-based and never - # matches a Transfer pair, so without the skip the recurring transfer - # would flip to inactive at the 6-month threshold even when the user - # is still doing the transfer monthly. Issue #1590 tracks the proper - # pair-detection fix. + test "Cleaner keeps a recurring transfer active when its pair still occurs (issue #1590)" do + # The seed name rarely matches future occurrences, so pair detection (not + # name matching) is what keeps a live transfer active past the threshold. rt = @family.recurring_transactions.create!( account: @account, destination_account: accounts(:credit_card), name: "Transfer to CC", amount: 250, currency: "USD", @@ -1012,12 +1009,61 @@ class RecurringTransactionTest < ActiveSupport::TestCase next_expected_date: 5.days.from_now.to_date, manual: true ) - assert rt.should_be_inactive?, "guard sanity: row would be marked inactive without the skip" + assert rt.should_be_inactive?, "guard sanity: stale last_occurrence_date" + + # A fresh transfer pair this cycle, carrying a *different* free-text name. + date = 1.month.ago.beginning_of_month + 4.days # day-of-month 5 + outflow = @account.entries.create!( + date: date, amount: 250, currency: "USD", name: "rent transfer", + entryable: Transaction.new(kind: "funds_movement") + ) + inflow = accounts(:credit_card).entries.create!( + date: date, amount: -250, currency: "USD", name: "rent transfer", + entryable: Transaction.new(kind: "funds_movement") + ) + Transfer.create!(outflow_transaction: outflow.entryable, inflow_transaction: inflow.entryable) RecurringTransaction.cleanup_stale_for(@family) assert_equal "active", rt.reload.status end + test "Cleaner retires a recurring transfer whose pair has stopped" do + # No matching Transfer pair → genuinely stale → should be retired. This is + # the correctness the pair-detection (vs the old blanket skip) buys us. + rt = @family.recurring_transactions.create!( + account: @account, destination_account: accounts(:credit_card), + name: "Transfer to CC", amount: 250, currency: "USD", + expected_day_of_month: 5, + last_occurrence_date: 7.months.ago.to_date, + next_expected_date: 5.days.from_now.to_date, + manual: true + ) + + RecurringTransaction.cleanup_stale_for(@family) + assert_equal "inactive", rt.reload.status + end + + test "matching_transactions finds the transfer pair regardless of occurrence name" do + rt = @family.recurring_transactions.create!( + account: @account, destination_account: accounts(:credit_card), + name: "Transfer to CC", amount: 250, currency: "USD", + expected_day_of_month: 5, last_occurrence_date: Date.current, + next_expected_date: 1.month.from_now.to_date, manual: true + ) + date = Date.current.beginning_of_month + 4.days # day-of-month 5 + outflow = @account.entries.create!( + date: date, amount: 250, currency: "USD", name: "an importer's wording", + entryable: Transaction.new(kind: "funds_movement") + ) + inflow = accounts(:credit_card).entries.create!( + date: date, amount: -250, currency: "USD", name: "an importer's wording", + entryable: Transaction.new(kind: "funds_movement") + ) + Transfer.create!(outflow_transaction: outflow.entryable, inflow_transaction: inflow.entryable) + + assert_includes rt.matching_transactions.map(&:id), outflow.id + end + test "Identifier#update_manual_recurring_transactions skips recurring transfers" do # Same reasoning as the Cleaner skip. Without the guard, the helper # would call find_matching_transaction_entries (single-account, by diff --git a/test/models/sure_import_test.rb b/test/models/sure_import_test.rb index ff99bb6fd..d0a076bf8 100644 --- a/test/models/sure_import_test.rb +++ b/test/models/sure_import_test.rb @@ -37,8 +37,23 @@ class SureImportTest < ActiveSupport::TestCase end test "max_row_count is higher than standard imports" do - assert_equal 100_000, SureImport.max_row_count - assert_equal 100_000, @import.max_row_count + with_env_overrides( + "SURE_IMPORT_MAX_ROWS" => nil, + "SURE_IMPORT_MAX_NDJSON_SIZE_MB" => nil + ) do + assert_equal 100_000, SureImport.max_row_count + assert_equal 100_000, @import.max_row_count + end + end + + test "max row count and ndjson size can be configured by environment" do + with_env_overrides( + "SURE_IMPORT_MAX_ROWS" => "150000", + "SURE_IMPORT_MAX_NDJSON_SIZE_MB" => "64" + ) do + assert_equal 150_000, SureImport.max_row_count + assert_equal 64.megabytes, SureImport.max_ndjson_size + end end test "dry_run totals can be derived from existing line type counts" do @@ -296,7 +311,7 @@ class SureImportTest < ActiveSupport::TestCase assert_equal({ "expected" => 0, "actual" => 1 }, verification.dig("mismatches", "valuations")) end - test "publish records mismatch when expected rows are skipped by readback" do + test "import records mismatch when expected rows are skipped by readback" do attach_ndjson(build_ndjson([ { type: "Transaction", data: { id: "transaction-1", @@ -308,10 +323,12 @@ class SureImportTest < ActiveSupport::TestCase } } ])) - @import.publish + initial_transaction_count = @family.entries.where(entryable_type: "Transaction").count + + @import.import! @import.reload - assert_equal "complete", @import.status + assert_equal initial_transaction_count, @family.entries.where(entryable_type: "Transaction").count assert_equal "mismatch", @import.readback_verification["status"] assert_equal({ "expected" => 1, "actual" => 0 }, @import.readback_verification.dig("mismatches", "transactions")) end @@ -410,6 +427,43 @@ class SureImportTest < ActiveSupport::TestCase assert_equal "Revertable Account", @import.accounts.first.name end + test "import tracks split parent entries for revert" do + attach_ndjson(build_ndjson([ + { type: "Account", data: { + id: "split-account", + name: "Split Revert Account", + balance: "500.00", + currency: "USD", + accountable_type: "Depository" + } }, + { type: "Transaction", data: { + id: "split-parent", + account_id: "split-account", + date: "2024-01-15", + amount: "100.00", + name: "Revertable split parent", + currency: "USD", + split_lines: [ + { id: "split-child-1", amount: "40.00", name: "Split child one" }, + { id: "split-child-2", amount: "60.00", name: "Split child two" } + ] + } } + ])) + + @import.publish + + parent_entry = @family.entries.find_by!(name: "Revertable split parent") + split_entry_ids = [ parent_entry.id, *parent_entry.child_entries.pluck(:id) ] + + assert parent_entry.split_parent? + assert_equal 3, @import.entries.where(id: split_entry_ids).count + + assert_difference -> { Entry.where(id: split_entry_ids).count }, -3 do + @import.revert + end + assert_equal "pending", @import.reload.status + end + test "publishes later enqueues job" do attach_ndjson(build_ndjson([ { type: "Account", data: { @@ -428,6 +482,242 @@ class SureImportTest < ActiveSupport::TestCase assert_equal "importing", @import.status end + test "publish_later raises custom error when preflight passes but import is not publishable" do + @import.stubs(:validate_sure_preflight!).returns(true) + @import.stubs(:publishable?).returns(false) + + assert_no_enqueued_jobs do + error = assert_raises SureImport::NotPublishableError do + @import.publish_later + end + assert_equal "Import was uploaded but has no publishable records.", error.message + end + assert_equal "pending", @import.reload.status + end + + test "publish_later restores previous status when enqueue fails" do + attach_ndjson(build_ndjson([ + { type: "Account", data: { + id: "account-1", + name: "Queued Account", + balance: "100", + currency: "USD", + accountable_type: "Depository" + } } + ])) + ImportJob.stubs(:perform_later).raises(StandardError, "queue down") + + assert_no_enqueued_jobs do + error = assert_raises StandardError do + @import.publish_later + end + assert_equal "queue down", error.message + end + + assert_equal "pending", @import.reload.status + end + + test "preflight reports blocking errors before publish_later enqueues" do + @family.categories.create!( + name: "Groceries", + color: "#407706", + lucide_icon: "shopping-basket" + ) + attach_ndjson(build_ndjson([ + { type: "Account", data: { + id: "account-1", + name: "Blocked Account", + balance: "100", + currency: "USD", + accountable_type: "Depository" + } }, + { type: "Category", data: { id: "category-1", name: "Groceries" } } + ])) + + assert_no_enqueued_jobs do + assert_raises SureImport::PreflightError do + @import.publish_later + end + end + + assert_equal "failed", @import.reload.status + assert_includes @import.error, "Category name \"Groceries\" already exists" + end + + test "publish_later reports unsupported records through preflight before publishable check" do + attach_ndjson(build_ndjson([ + { type: "MysteryType", data: { id: "mystery-1" } } + ])) + + assert_no_enqueued_jobs do + assert_raises SureImport::PreflightError do + @import.publish_later + end + end + + assert_equal "failed", @import.reload.status + assert_includes @import.error, "unsupported record type MysteryType" + end + + test "publish preflight failure does not partially import records" do + attach_ndjson(build_ndjson([ + { type: "Account", data: { + id: "account-1", + name: "Should Not Import", + balance: "100", + currency: "USD", + accountable_type: "NotReal" + } } + ])) + + assert_no_difference -> { @family.accounts.where(name: "Should Not Import").count } do + @import.publish + end + + assert_equal "failed", @import.reload.status + assert_includes @import.error, "invalid accountable_type" + end + + test "preflight catches missing fields unsupported types duplicate valuations and references" do + attach_ndjson(build_ndjson([ + { type: "RecurringTransaction", data: { id: "recurring-1" } }, + { type: "MysteryType", data: { id: "mystery-1" } }, + { type: "Account", data: { + id: "account-1", + name: "Bad Subtype", + balance: "100", + accountable_type: "Depository", + accountable: { subtype: "not-a-subtype" } + } }, + { type: "Valuation", data: { account_id: "account-1", date: "2024-01-01", amount: "100" } }, + { type: "Valuation", data: { account_id: "account-1", date: "2024-01-01", amount: "101" } }, + { type: "Transaction", data: { + id: "transaction-1", + account_id: "missing-account", + date: "2024-01-02", + amount: "-5", + tag_ids: [ "missing-tag" ] + } } + ])) + + result = @import.sure_preflight + codes = result.errors.map { |error| error[:code] } + + assert_not result.valid? + assert_includes codes, "missing_required_fields" + assert_includes codes, "unsupported_record_type" + assert_includes codes, "invalid_accountable_subtype" + assert_includes codes, "duplicate_valuation" + assert_includes codes, "missing_reference" + end + + test "preflight rejects invalid accountable types through explicit allowlist" do + attach_ndjson(build_ndjson([ + { type: "Account", data: { + id: "account-1", + name: "Bad Accountable", + balance: "100", + accountable_type: "Kernel", + accountable: { subtype: "system" } + } } + ])) + + result = @import.sure_preflight + + assert_not result.valid? + assert_nil Accountable.from_type("Kernel") + assert_equal Depository, Accountable.from_type("Depository") + assert_equal [ "invalid_accountable_type" ], result.errors.map { |error| error[:code] } + assert_includes result.error_message, 'invalid accountable_type "Kernel"' + end + + test "preflight catches duplicate taxonomy names inside ndjson" do + attach_ndjson(build_ndjson([ + { type: "Category", data: { id: "category-1", name: "Groceries" } }, + { type: "Category", data: { id: "category-2", name: "Groceries" } } + ])) + + result = @import.sure_preflight + + assert_not result.valid? + assert_includes result.errors.map { |error| error[:code] }, "duplicate_taxonomy_name" + assert_includes result.error_message, "appears more than once" + end + + test "preflight rejects split line totals that cannot import atomically" do + attach_ndjson(build_ndjson([ + { type: "Account", data: { + id: "split-account", + name: "Split Checking", + balance: "500.00", + currency: "USD", + accountable_type: "Depository" + } }, + { type: "Transaction", data: { + id: "split-parent", + account_id: "split-account", + date: "2024-01-15", + amount: "100.00", + name: "Invalid split parent", + currency: "USD", + split_lines: [ + { id: "split-child-1", amount: "40.00", name: "Split child one" }, + { id: "split-child-2", amount: "50.00", name: "Split child two" } + ] + } } + ])) + + result = @import.sure_preflight + + assert_not result.valid? + assert_includes result.errors.map { |error| error[:code] }, "split_amount_mismatch" + + assert_no_enqueued_jobs do + assert_raises SureImport::PreflightError do + @import.publish_later + end + end + assert_equal "failed", @import.reload.status + end + + test "strict preflight requires references to be present in the same ndjson" do + existing_account = @family.accounts.first + existing_parent = @family.categories.create!( + name: "Existing Parent", + color: "#407706", + lucide_icon: "shapes" + ) + + attach_ndjson(build_ndjson([ + { + type: "Valuation", + data: { + account_id: existing_account.id, + date: "2024-01-01", + amount: "100" + } + }, + { + type: "Category", + data: { + id: "category-child", + name: "Imported Child", + parent_id: existing_parent.id + } + } + ])) + + result = @import.sure_preflight + + assert_not result.valid? + assert_equal( + [ "missing_reference", "missing_reference" ], + result.errors.map { |error| error[:code] } + ) + assert_includes result.error_message, "references missing account_id" + assert_includes result.error_message, "references missing parent_id" + end + private def attach_ndjson(ndjson) @@ -492,3 +782,75 @@ class SureImportTest < ActiveSupport::TestCase ]) end end + +class Import::PreflightTest < ActiveSupport::TestCase + setup do + @family = families(:dylan_family) + end + + test "SureImport preflight reports strict taxonomy collisions" do + @family.tags.create!(name: "Reviewed", color: "#12B76A") + ndjson = build_ndjson([ + { type: "Tag", data: { id: "tag-1", name: "Reviewed" } } + ]) + + assert_no_difference("Import.count") do + response = Import::Preflight.new( + family: @family, + params: { type: "SureImport", raw_file_content: ndjson } + ).call + payload = response.payload[:data] + + assert_equal :ok, response.status + assert_equal false, payload[:valid] + assert_equal "existing_taxonomy_collision", payload[:errors].first[:code] + end + end + + test "SureImport preflight counts invalid rows instead of validation errors" do + ndjson = build_ndjson([ + [], + { type: "Transaction", data: { id: "transaction-1" } } + ]) + + response = Import::Preflight.new( + family: @family, + params: { type: "SureImport", raw_file_content: ndjson } + ).call + payload = response.payload[:data] + + assert_equal :ok, response.status + assert_equal 2, payload[:stats][:rows_count] + assert_equal 1, payload[:stats][:valid_rows_count] + assert_equal 1, payload[:stats][:invalid_rows_count] + assert_operator payload[:errors].size, :>, payload[:stats][:invalid_rows_count] + end + + test "SureImport preflight handles missing entity counts" do + result = Struct.new(:stats, :errors, :warnings, keyword_init: true) do + def valid? + true + end + end.new( + stats: { rows_count: 1, valid_rows_count: 1, invalid_rows_count: 0 }, + errors: [], + warnings: [] + ) + SureImport::Preflight.stubs(:new).returns(stub(call: result)) + + response = Import::Preflight.new( + family: @family, + params: { type: "SureImport", raw_file_content: "{}" } + ).call + payload = response.payload[:data] + + assert_equal :ok, response.status + assert_includes payload[:warnings], "No importable records were found." + end + + private + + def build_ndjson(records) + records.map(&:to_json).join("\n") + end +end diff --git a/workers/preview/deploy/redact_preview_log.sh b/workers/preview/deploy/redact_preview_log.sh new file mode 100755 index 000000000..40633ff4e --- /dev/null +++ b/workers/preview/deploy/redact_preview_log.sh @@ -0,0 +1,12 @@ +#!/usr/bin/env bash +set -euo pipefail + +perl -pe ' + s#registry\.cloudflare\.com/[^/]+/#registry.cloudflare.com//#g; + s#((?:Authorization|Proxy-Authorization):\s*Bearer\s+)[^[:space:]]+#$1#gi; + s#((?:X-Auth-Key|X-Auth-Email|X-Api-Key|Api-Key):\s*)[^[:space:]]+#$1#gi; + s#([?&](?:token|api_key|access_token|refresh_token|auth_token|key|private_key)=)[^&[:space:]]+#$1#gi; + s#("(?:token|api_key|access_token|refresh_token|auth_token|secret|client_secret|private_key)"\s*:\s*")[^"]*#$1#gi; + s#(CLOUDFLARE_ACCOUNT_ID=)[^[:space:]]+#$1#g; + s#((?:CLOUDFLARE_API_TOKEN|API_KEY|ACCESS_TOKEN|REFRESH_TOKEN|AUTH_TOKEN|PRIVATE_KEY)=)[^[:space:]]+#$1#g; +' diff --git a/workers/preview/deploy/resolve_preview_request.cjs b/workers/preview/deploy/resolve_preview_request.cjs index 7aec276c1..596a1e3c0 100644 --- a/workers/preview/deploy/resolve_preview_request.cjs +++ b/workers/preview/deploy/resolve_preview_request.cjs @@ -32,6 +32,10 @@ function uniqueNumbers(candidates) { return [...new Set(candidates.map((candidate) => candidate.parsed.prNumber))]; } +function uniquePullRequestNumbers(pullRequests) { + return [...new Set(pullRequests.map((pullRequest) => pullRequest.number))]; +} + function associatedPullRequestsForHead(associatedPullRequests, context, headSha) { const baseRepo = repoFullName(context); @@ -43,15 +47,65 @@ function associatedPullRequestsForHead(associatedPullRequests, context, headSha) } function selectPullRequestNumber({ runPullRequest, artifacts, associatedPullRequests, context, headSha }) { - if (runPullRequest?.number) { + const associatedHeadPullRequests = associatedPullRequestsForHead(associatedPullRequests, context, headSha); + const associatedPullRequestNumbers = uniquePullRequestNumbers(associatedHeadPullRequests); + const artifactPullRequestNumbers = uniqueNumbers(artifactCandidates(artifacts, headSha)); + const workflowRunPullRequestNumber = runPullRequest?.number ?? null; + + if (artifactPullRequestNumbers.length > 1) { return { - prNumber: runPullRequest.number, - source: "workflow_run", + error: `Workflow run ${headSha} published preview artifacts for multiple pull requests`, }; } - const associatedHeadPullRequests = associatedPullRequestsForHead(associatedPullRequests, context, headSha); - const artifactPullRequestNumbers = uniqueNumbers(artifactCandidates(artifacts, headSha)); + if (artifactPullRequestNumbers.length === 1) { + const artifactPullRequestNumber = artifactPullRequestNumbers[0]; + + if (workflowRunPullRequestNumber && workflowRunPullRequestNumber !== artifactPullRequestNumber) { + return { + error: `Preview artifact PR ${artifactPullRequestNumber} conflicts with workflow_run PR ${workflowRunPullRequestNumber}`, + }; + } + + if ( + associatedPullRequestNumbers.length > 0 && + !associatedPullRequestNumbers.includes(artifactPullRequestNumber) + ) { + return { + error: `Preview artifact PR ${artifactPullRequestNumber} conflicts with commit-associated PRs ${associatedPullRequestNumbers.join(", ")}`, + }; + } + + const corroboratingSources = []; + if (workflowRunPullRequestNumber === artifactPullRequestNumber) corroboratingSources.push("workflow_run"); + if (associatedPullRequestNumbers.includes(artifactPullRequestNumber)) { + corroboratingSources.push("commit_association"); + } + + return { + prNumber: artifactPullRequestNumber, + source: + corroboratingSources.length > 0 + ? `artifact_name+${corroboratingSources.join("+")}` + : "artifact_name", + }; + } + + if (workflowRunPullRequestNumber) { + if ( + associatedPullRequestNumbers.length > 0 && + !associatedPullRequestNumbers.includes(workflowRunPullRequestNumber) + ) { + return { + error: `workflow_run PR ${workflowRunPullRequestNumber} conflicts with commit-associated PRs ${associatedPullRequestNumbers.join(", ")}`, + }; + } + + return { + prNumber: workflowRunPullRequestNumber, + source: "workflow_run", + }; + } if (associatedHeadPullRequests.length === 1) { return { @@ -61,34 +115,11 @@ function selectPullRequestNumber({ runPullRequest, artifacts, associatedPullRequ } if (associatedHeadPullRequests.length > 1) { - const associatedNumbers = new Set(associatedHeadPullRequests.map((pullRequest) => pullRequest.number)); - const artifactMatches = artifactPullRequestNumbers.filter((number) => associatedNumbers.has(number)); - - if (artifactMatches.length === 1) { - return { - prNumber: artifactMatches[0], - source: "artifact_and_commit_association", - }; - } - return { error: `Workflow run head SHA ${headSha} is associated with multiple open pull requests and no single preview artifact matched`, }; } - if (artifactPullRequestNumbers.length === 1) { - return { - prNumber: artifactPullRequestNumbers[0], - source: "artifact_name", - }; - } - - if (artifactPullRequestNumbers.length > 1) { - return { - error: `Workflow run ${headSha} published preview artifacts for multiple pull requests`, - }; - } - return { prNumber: null, source: "none", @@ -186,6 +217,7 @@ async function resolvePreviewRequest({ github, context, core }) { core.setOutput("head_sha", headSha); core.setOutput("is_fork", String(isFork)); core.setOutput("pr_number", String(prNumber)); + core.setOutput("resolution_source", selected.source); core.setOutput("should_deploy", "true"); }