client url verification and unsubscribe #7
Conversation
verifying client get has parameters not matching the documentation
There was a problem hiding this comment.
Thanks for the PR!
It would be great if you could provide some more information in the PR on why you think it's needed: what you're changes are on a high-level, whey they're needed etc.
Moreover, when you've done changes, please rebase on the current master; that makes the history easier to read and we don't get a huge merge-bubble.
Thanks!
| @@ -179,14 +194,14 @@ public IActionResult Post(string subscriptionId, [FromBody] Notification notific | |||
| return View("FHIRcastClient", internalModel); | |||
| } | |||
|
|
|||
| [Route("unsubscribe/{subscriptionId}")] | |||
| [Route("Unsubscribe/{subscriptionId}")] | |||
There was a problem hiding this comment.
Is this suggestion correct? Routes are generally lower-case and I think we should stick to that convention (even if it doesn't matter, technically).
| @@ -62,8 +62,8 @@ public class SubscriptionVerification : SubscriptionWithLease { | |||
| } | |||
|
|
|||
| public enum SubscriptionMode { | |||
| Subscribe, | |||
There was a problem hiding this comment.
I don't think we should change the Enum casing just to get an easy fix for ToString() being the correct case. Better to stick with the language convention here.
| @@ -87,21 +87,36 @@ public IActionResult Post([FromForm] ClientModel model) | |||
| /// <param name="verification">Hub's verification response to our subscription attempt</param> | |||
| /// <returns></returns> | |||
| [HttpGet("{subscriptionId}")] | |||
| public IActionResult Get(string subscriptionId, [FromQuery] SubscriptionVerification verification) | |||
| public IActionResult Get(string subscriptionId) | |||
There was a problem hiding this comment.
Did you see how validation is done when posting subscriptions? The validation is basically set on the model using attributes specifying what is required etc. The framework then generates error messages that can be used in the response (which should probably be Bad request and not Not found).
| callbackParameters.Mode = subscription.Mode; | ||
| callbackParameters.Topic = subscription.Topic; | ||
| logger.LogDebug($"Calling callback url: {subscription.Callback}"); | ||
| string verifyUrl = subscription.Callback + "?" + |
There was a problem hiding this comment.
You moved the logic for constructing the URL from the SubscriptionCallback class and in here. Keep it in that class instead if you need to re-write it. A better commit message on why you're doing this re-write would be great.
match specification query parameters for the client url verification call