Skip to content

Chore 1738 remove unused functions - #1818

Open
eskebab wants to merge 6 commits into
mainfrom
chore-1738-remove-unused-functions
Open

Chore 1738 remove unused functions#1818
eskebab wants to merge 6 commits into
mainfrom
chore-1738-remove-unused-functions

Conversation

@eskebab

@eskebab eskebab commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Description

Related Issue(s)

Verification

  • Your code builds clean without any errors or warnings
  • Manual testing done (required)
  • Relevant automated test added (if you find this hard, leave it and we'll help out)
  • All tests run green

Documentation

  • User documentation is updated with a separate linked PR in altinn-studio-docs. (if applicable)

Summary by CodeRabbit

  • New Features

    • Notification order and retrieval results now include resource actions and notification types.
    • SMS status updates support delivery report data, including null values when applicable.
    • Cleanup operations support configurable batch sizes and report the number of deleted records.
  • Improvements

    • Updated cancellation, tracking, and notification processing workflows to use the latest behavior.
    • Removed obsolete notification operations and legacy interfaces.
    • Improved handling of large order identifiers during notification processing.

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 2cc7fee3-43db-4387-b9d6-aa99786fa79a

📥 Commits

Reviewing files that changed from the base of the PR and between f599f1b and 5c575e1.

📒 Files selected for processing (1)
  • components/api/src/Altinn.Notifications.Persistence/Migration/v0.95/01-drop-unused-functions.sql
💤 Files with no reviewable changes (1)
  • components/api/src/Altinn.Notifications.Persistence/Migration/v0.95/01-drop-unused-functions.sql

📝 Walkthrough

Walkthrough

The migration versions notification functions, adds resourceaction and notificationtype fields, persists SMS delivery reports, removes legacy functions, and adds conditional cleanup for obsolete database routines.

Changes

Notification SQL changes

Layer / File(s) Summary
Versioned notification contracts
components/api/src/Altinn.Notifications.Persistence/Migration/FunctionsAndProcedures/cancelorder.sql, getorderincludestatus.sql, getnotificationsbynin.sql
Cancellation and order-status functions use new versions. Their return data includes resourceaction. NIN notification results include notificationtype.
SMS delivery report update
components/api/src/Altinn.Notifications.Persistence/Migration/FunctionsAndProcedures/updatesmsnotification.sql
updatesmsnotification_v3 accepts and stores _deliveryreport. Expiry still controls status-field updates.
Obsolete function cleanup
components/api/src/Altinn.Notifications.Persistence/Migration/FunctionsAndProcedures/deleteoldstatusfeedrecords.sql, getemailsstatusnewupdatestatus.sql, getmetrics.sql, getshipmentforstatusfeed.sql, getshipmenttracking.sql, getsmsstatusnewupdatestatus.sql, updateemailstatus.sql, components/api/src/Altinn.Notifications.Persistence/Migration/v0.95/01-drop-unused-functions.sql
Legacy function definitions were removed or replaced. The migration conditionally drops obsolete notification functions and the updateemailstatus procedure.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant cancelorder_v2
  participant getorder_includestatus_v5
  participant NotificationOrder
  cancelorder_v2->>getorder_includestatus_v5: request order details
  getorder_includestatus_v5->>NotificationOrder: read ResourceAction
  NotificationOrder-->>getorder_includestatus_v5: return order status and resourceaction
  getorder_includestatus_v5-->>cancelorder_v2: return order details
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: removing unused notification functions and related legacy database routines.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chore-1738-remove-unused-functions

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@eskebab
eskebab marked this pull request as ready for review August 12, 2026 12:15

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
components/api/src/Altinn.Notifications.Persistence/Migration/v0.95/01-drop-unused-functions.sql (1)

35-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove the duplicate updatesmsnotification_v2 drop.

Line 25 already drops notifications.updatesmsnotification_v2(text, text, uuid). The second DROP FUNCTION IF EXISTS is harmless, but it makes the migration harder to audit and may hide a missing cleanup entry.

Proposed fix
- DROP FUNCTION IF EXISTS notifications.updatesmsnotification_v2(text, text, uuid); -- v2
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@components/api/src/Altinn.Notifications.Persistence/Migration/v0.95/01-drop-unused-functions.sql`
at line 35, Remove the duplicate DROP FUNCTION statement for
notifications.updatesmsnotification_v2(text, text, uuid) from the migration,
keeping the existing earlier drop entry unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In
`@components/api/src/Altinn.Notifications.Persistence/Migration/v0.95/01-drop-unused-functions.sql`:
- Line 35: Remove the duplicate DROP FUNCTION statement for
notifications.updatesmsnotification_v2(text, text, uuid) from the migration,
keeping the existing earlier drop entry unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 66addb48-a1ff-4537-9669-9e1fdb7972fa

📥 Commits

Reviewing files that changed from the base of the PR and between 49e411b and f599f1b.

📒 Files selected for processing (1)
  • components/api/src/Altinn.Notifications.Persistence/Migration/v0.95/01-drop-unused-functions.sql

@sonarqubecloud

Copy link
Copy Markdown

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant