user.addDoc sets error when uploading SSN and KBA is needed

[behrangnm]

I think it's better for user.addDoc to not count status 202 as an error and set the err in the callback function. Status 202, which now indicates KBA questions, is not an error and is merely part of the normal flow of adding SSN to a user profile.

[tnhipps]

Hi @behrangnm

We've been debating this internally. We could make the res a json object sometimes, i.e. the 202, but then you'd have to check the type of the response object every time. What would you suggest?

[behrangnm]

Hi @tnhipps

I think HTTP response 200 should always be sent when the Synapse server understands my request and accepts all the parameters. So two points: 1- I think Having another HTTP response type is a bit confusing - so response type 202 is not intuitive 2- Also, it makes more sense to check for a specific flag or parameter to see what the next step is rather than forking out of the error control code. After all, it's a custom library and if communicated to the API user, it's ok to assign a parameter to indicate the next step and change the returned objects based on that.

[sankaet]

@behrangnm regarding point 1:

HTTP verbs is a common practice in most RESTful APIs. It actually helps developers while formulating workflows for different errors. Here is a reference to http codes . They make conditional programming very intuitive.

Would you be able to elaborate on your second point? That might be interesting to explore.

[behrangnm]

@sankaet I understand your point on HTTP verbs, but I imagine abstracting the verbs out of user interface would simplify things. After all, the API user has decided to use this library to walk away from making low level http requests (and dealing with all the verbs).

Now back to point 2. Here's what I had in mind:

LibraryFunctionCall(params/payload, function(err, returned_object) {
  if (err) {
    // Something has gone wrong. Pass the error object to the error handling function
  } else {
    // Let's examine the returned_object to figure out what's next
    switch (returned_object.action) {
       case "done":
         // use returned_object.thanks_for_using_this_api
         break;
       case "need_more_data":
         // use returned_object.need_more_data_for_KBA
         break;
       default:
         // developer needs to be alerted
    }
  }
}

Basically, I'd say the above flow is easier to use/understand than putting the KBA indicator inside the err object.

Note: I'm a new github user, so don't mind my snippet posting skills :)

[achintverma]

I agree to above user flow suggested by @behrangnm for KBA. KBA is not an error and rather than comparing status codes, it would be better to return success response with additional info regarding questions or uploading ID.

Whatever you guys decide, please let me know. I implemented whole system using your Node wrapper which was released last year and in less than 5 months, you deprecated that and created new one.

[stevula]

It seems to me there were two separate issues here:

1. Whether 202 should be treated as error or success. This is a client library issue.
2. Whether KBA should be returned as 200 or 202. This is an API issue.

I will speak to 1 -- think it would be better to treat any 2xx code as a success and leave it to the user to decide what to do with the response data. 2xx are by definition successful responses. Unfortunately this will break people's existing implementations if we change it though. Thoughts?