Skip to content

fix: resolve r4.1 release review findings (#285, #286, #287, #289) - #290

Merged
bigludo7 merged 11 commits into
camaraproject:mainfrom
albertoramosmonagas:fix/r4.1-release-review-findings
Sep 16, 2026
Merged

bigludo7 merged 11 commits into
camaraproject:mainfrom
albertoramosmonagas:fix/r4.1-release-review-findings

Conversation

@albertoramosmonagas

@albertoramosmonagas albertoramosmonagas commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • correction
  • tests

What this PR does / why we need it:

Fixs addressing findings from the Release r4.1 review (#284).

Special notes for reviewers:

Changelog input

release-note
SimSwap r4.1 release review corrections:
- Clarified mandatory vs optional operation support (#285)
- Updated API documentation for v0.4.0 with pagination, credentials, and event structure details (#286)
- Fixed documentation regressions: RFC 3339 link, missing example, incorrect description (#287)
- Added comprehensive test definitions for retrieve-age-band operation (#289)

@albertoramosmonagas
albertoramosmonagas force-pushed the fix/r4.1-release-review-findings branch from 96056e5 to a0c948f Compare September 7, 2026 09:16
@albertoramosmonagas
albertoramosmonagas marked this pull request as ready for review September 8, 2026 13:37
bigludo7
bigludo7 previously approved these changes Sep 8, 2026

@bigludo7 bigludo7 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@hdamker hdamker left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

See suggestions below

Comment thread code/API_definitions/sim-swap.yaml Outdated
Comment thread code/API_definitions/sim-swap-subscriptions.yaml Outdated
Comment thread code/API_definitions/sim-swap-subscriptions.yaml Outdated
Comment thread code/Test_definitions/sim-swap-retrieveSimSwapAgeBand.feature Outdated
Comment thread code/Test_definitions/sim-swap-retrieveSimSwapAgeBand.feature Outdated
Comment thread code/Test_definitions/sim-swap-retrieveSimSwapAgeBand.feature Outdated
Comment thread code/API_definitions/sim-swap-subscriptions.yaml Outdated
Comment thread code/API_definitions/sim-swap-subscriptions.yaml Outdated
- camaraproject#285: clarify mandatory /check and /retrieve-date operations
- camaraproject#286: update sim-swap-subscriptions description for v0.4.0
- camaraproject#287: fix RFC 3339 link, restore example, fix EventSwapped description
- camaraproject#289: add test definitions for /retrieve-age-band
@albertoramosmonagas
albertoramosmonagas force-pushed the fix/r4.1-release-review-findings branch from dc8084c to a46dd21 Compare September 8, 2026 15:32
@hdamker

hdamker commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

"albertoramosmonagas force-pushed the fix/r4.1-release-review-findings branch from 2655d4c to dc8084c"

Force push should be a last resort on public PRs.

@hdamker

hdamker commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

"Fixes #284, #285, #286, #287, #289":

That would close only #284, but it shouldn't close it, as not all Sub Issues are addressed with this PR.

@albertoramosmonagas
albertoramosmonagas marked this pull request as draft September 8, 2026 15:52
@albertoramosmonagas

Copy link
Copy Markdown
Contributor Author

Move to draft to work on the suggested changes

@albertoramosmonagas albertoramosmonagas changed the title fix: resolve r4.1 release review findings (#284) fix: resolve r4.1 release review findings (#285, #286, #287, #289) Sep 9, 2026
@albertoramosmonagas
albertoramosmonagas marked this pull request as ready for review September 9, 2026 09:39
@albertoramosmonagas

Copy link
Copy Markdown
Contributor Author

All changes applied.

bigludo7
bigludo7 previously approved these changes Sep 14, 2026

@bigludo7 bigludo7 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM - Thanks @albertoramosmonagas

@bigludo7

Copy link
Copy Markdown
Collaborator

@hdamker please give me your thumb up if OK for you ;)

@hdamker hdamker left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Compared to the version I reviewed (commit a0c948f), five scenarios in sim-swap-retrieveSimSwapAgeBand.feature have no surviving replacement — not consolidated, just gone. Both sibling files (sim-swap-checkSimSwap.feature, sim-swap-retrieveSimSwapDate.feature) still cover all of these:

  • 401.2_expired_access_token
  • 401.3_invalid_access_token
  • 404 IDENTIFIER_NOT_FOUND (both the standalone and C02.02 copies are gone)
  • C02.03_unnecessary_phone_number

See my suggestions below. Additionally a suggestion for the band 10 scenario.

Comment thread code/Test_definitions/sim-swap-retrieveSimSwapAgeBand.feature Outdated
Comment thread code/Test_definitions/sim-swap-retrieveSimSwapAgeBand.feature
Comment thread code/Test_definitions/sim-swap-retrieveSimSwapAgeBand.feature
Comment thread code/Test_definitions/sim-swap-retrieveSimSwapAgeBand.feature
Comment thread code/API_definitions/sim-swap.yaml Outdated
Co-authored-by: Herbert Damker <herbert.damker@telekom.de>
@albertoramosmonagas

albertoramosmonagas commented Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

Compared to the version I reviewed (commit a0c948f), five scenarios in sim-swap-retrieveSimSwapAgeBand.feature have no surviving replacement — not consolidated, just gone. Both sibling files (sim-swap-checkSimSwap.feature, sim-swap-retrieveSimSwapDate.feature) still cover all of these:

  • 401.2_expired_access_token
  • 401.3_invalid_access_token
  • 404 IDENTIFIER_NOT_FOUND (both the standalone and C02.02 copies are gone)
  • C02.03_unnecessary_phone_number

See my suggestions below. Additionally a suggestion for the band 10 scenario.

Thanks @hdamker for the suggestions, they were very helpful. I've try to cover everything but please let me know if I missed anything.

CC: @bigludo7

@hdamker

hdamker commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

@albertoramosmonagas Two issues from the latest push (ef5a48f + d672361):

sim-swap.yaml — the retrieveSimSwapAgeBand description got corrupted applying the suggestion: the "Transient backend or data-source failures..." sentence is now cut off mid-word ("...Transient"), followed by the whole paragraph repeated a second time. The original redundant "This operation is OPTIONAL... 501 NOT_IMPLEMENTED" sentence is also still there.

Feature file — my suggested scenarios and the separate manual commit both landed, so several tags are now duplicated: 401.2_expired_access_token and 401.3_invalid_access_token each appear twice, and the 404-not-found / unnecessary-identifier cases each appear twice under two different tag names (C02.02/404.1, C02.03/422.3).

Could you dedupe the feature file (one copy of each, in my suggestions I kept the 4xx.y ) and fix the sim-swap.yaml paragraph back to one clean copy?

@albertoramosmonagas

Copy link
Copy Markdown
Contributor Author

@albertoramosmonagas Two issues from the latest push (ef5a48f + d672361):

sim-swap.yaml — the retrieveSimSwapAgeBand description got corrupted applying the suggestion: the "Transient backend or data-source failures..." sentence is now cut off mid-word ("...Transient"), followed by the whole paragraph repeated a second time. The original redundant "This operation is OPTIONAL... 501 NOT_IMPLEMENTED" sentence is also still there.

Feature file — my suggested scenarios and the separate manual commit both landed, so several tags are now duplicated: 401.2_expired_access_token and 401.3_invalid_access_token each appear twice, and the 404-not-found / unnecessary-identifier cases each appear twice under two different tag names (C02.02/404.1, C02.03/422.3).

Could you dedupe the feature file (one copy of each, in my suggestions I kept the 4xx.y ) and fix the sim-swap.yaml paragraph back to one clean copy?

Thanks for catching those issues. I've just fixed both in the last 2 commits. Apologies for the extended discussion. Please let me know if anything else needs attention.

@bigludo7 bigludo7 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@hdamker hdamker left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@albertoramosmonagas thanks for fixing the regressions. Look good to me now!

@bigludo7
bigludo7 merged commit 05d46bf into camaraproject:main Sep 16, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sim-swap description overclaims optionality for /check and /retrieve-date

3 participants