-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add create issue credential #6
Conversation
create the issuer profile and get the prefix of the ID from the profile issue-47This comment was generated by todo based on a
|
credential.Types = data.Type | ||
credential.Issuer = data.Issuer | ||
credential.Issued = &issueDate | ||
credential.Schemas = []verifiable.TypedID{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gonna remove this , as the bug was raised to dima and he fixed
time.Now().Hour(), time.Now().Minute(), time.Now().Second(), 0, time.UTC) | ||
|
||
// todo Issue raised, only base context doesnt work | ||
credential.Context = []string{credentialContext, data.Context} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gonna remove this , as the bug was raised to dima and he fixed
Codecov Report
@@ Coverage Diff @@
## master #6 +/- ##
=========================================
Coverage ? 94.59%
=========================================
Files ? 4
Lines ? 74
Branches ? 0
=========================================
Hits ? 70
Misses ? 2
Partials ? 2
Continue to review full report at Codecov.
|
a29e83c
to
b462217
Compare
create the profile and get the prefix of the ID from the profile issue-47This comment was generated by todo based on a
|
err := json.NewDecoder(req.Body).Decode(&data) | ||
|
||
if err != nil { | ||
rw.WriteHeader(http.StatusBadRequest) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is no return in this error block
cred, err := createCredential(&data) | ||
|
||
if err != nil { | ||
log.Errorf("Failed to create new credential: %s", err.Error()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need error status and return here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should InternalServerError be used instead of StatusNotAcceptable? @sandrask what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed this to StatusBadRequest
|
||
resp, err := json.Marshal(cred) | ||
if err != nil { | ||
log.Errorf("Failed to marshal credential: %s", err.Error()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above: set error status and return here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
_, err = rw.Write(resp) | ||
if err != nil { | ||
log.Errorf("Failed to write response for VCInputData vault creation success: %s", err.Error()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above: set error status and return here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch - I missed this earlier when reviewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
_, err = rw.Write(resp) | ||
if err != nil { | ||
log.Errorf("Failed to write response for VCInputData vault creation success: %s", err.Error()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch - I missed this earlier when reviewing
|
||
createCredentialHandler.Handle().ServeHTTP(rr, req) | ||
|
||
require.Equal(t, http.StatusBadRequest, rr.Code) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something else to check out...it looks like your handler will always return StatusCreated, so I'm curious why this test actually passes. (The subsequent call to WriteHeader is ignored?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
closes trustbloc#2 Signed-off-by: talwinder.kaur <talwinder.kaur@securekey.com>
validCredential, err := createCredential(&data) | ||
|
||
if err != nil { | ||
rw.WriteHeader(http.StatusNotAcceptable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why status not acceptable: it should be either bad request or internal server error
closes #2
Signed-off-by: talwinder.kaur talwinder.kaur@securekey.com