Skip to content

Added /deregister and /list capabilities#5

Merged
MFAshby merged 6 commits intomainfrom
add_deregister
Feb 10, 2026
Merged

Added /deregister and /list capabilities#5
MFAshby merged 6 commits intomainfrom
add_deregister

Conversation

@MFAshby
Copy link
Copy Markdown
Contributor

@MFAshby MFAshby commented Jan 26, 2026 •
edited
Loading

These do the obvious thing.

Related: https://github.com/patientsknowbest/phr/pull/10886

@MFAshby
Added /deregister and /list capabilities
f3196da 
These do the obvious thing.
russellpkb
russellpkb previously approved these changes Jan 27, 2026
View reviewed changes
alyssaruth
alyssaruth previously approved these changes Feb 4, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@alyssaruth alyssaruth left a comment

Choose a reason for hiding this comment

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

LGTM, couple of minor suggestions

Comment thread main_test.go Outdated
Comment thread main.go Outdated
homps
homps previously approved these changes Feb 5, 2026
View reviewed changes
Copy link
Copy Markdown

@homps homps left a comment

Choose a reason for hiding this comment

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

I very much support the idea of ditching the callbackUrl to keep the API sane for deregister, now is the perfect time to do it.

@MFAshby MFAshby dismissed stale reviews from homps, alyssaruth, and russellpkb via 488dcf6 February 5, 2026 22:14
@MFAshby MFAshby requested review from alyssaruth and homps February 6, 2026 08:28
alyssaruth
alyssaruth previously approved these changes Feb 6, 2026
View reviewed changes
leonyork
leonyork requested changes Feb 6, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@leonyork leonyork left a comment

Choose a reason for hiding this comment

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

I just saw this at the end of the day, so sorry the review is a little short! I think it's worth fixing the concurrency issue (and including the tests I added) as it'll be annoying if this breaks in e2es. (Happy to chat more/look through it a little more on Monday)

Comment thread main.go
@leonyork leonyork mentioned this pull request Feb 6, 2026
Merged
alyssaruth
alyssaruth previously approved these changes Feb 6, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@alyssaruth alyssaruth left a comment

Choose a reason for hiding this comment

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

Bah annoying to have missed the concurrency thing given that I fixed it the first time around: #2 😅

LGTM

leonyork
leonyork previously approved these changes Feb 9, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@leonyork leonyork left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the concurrency issue. Added a few non-blockers, but nothing that should stop this getting merged.

Comment thread main.go
Comment thread main.go
Comment thread main_test.go
@MFAshby MFAshby dismissed stale reviews from leonyork and alyssaruth via 9208960 February 9, 2026 14:53
@MFAshby MFAshby merged commit 8f59395 into main Feb 10, 2026
1 check passed
@MFAshby MFAshby deleted the add_deregister branch February 10, 2026 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@homps homps homps approved these changes

@leonyork leonyork leonyork approved these changes

@alyssaruth alyssaruth alyssaruth approved these changes

@russellpkb russellpkb Awaiting requested review from russellpkb

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@MFAshby @russellpkb @homps @leonyork @alyssaruth