-
Notifications
You must be signed in to change notification settings - Fork 35
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: delete credential by said #297
feat: delete credential by said #297
Conversation
*/ | ||
async delete(said: string): Promise<void> { | ||
const path = `/notifications/` + said; | ||
const method = 'DELETE'; | ||
await this.client.fetch(path, method, null); | ||
await this.client.fetch(path, method, undefined); |
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.
JSON.stringify(undefined)
is undefined
whereas JSON.stringify(null)
is "null"
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #297 +/- ##
==========================================
+ Coverage 83.63% 83.65% +0.01%
==========================================
Files 48 48
Lines 4235 4239 +4
Branches 1055 1055
==========================================
+ Hits 3542 3546 +4
- Misses 663 689 +26
+ Partials 30 4 -26 ☔ View full report in Codecov by Sentry. |
Not sure what happened on the integration test pulling the images. I can't re-run. edit - nvm, cmd needs |
|
||
await step('Holder deletes LE credential', async () => { | ||
await holderClient.credentials().delete(leCredentialId); | ||
assert.rejects( |
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.
Never used assert.rejects
but to me it looks like this would give false positives since this is not awaited.
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.
It actually was always checking for me until I put the right string because I guess the next await
below it takes longer to fetch that this on average. But yes, awaited now! Thanks for spotting.
I also added awaits elsewhere for rejects.
@lenkan good to merge? |
Removes credential from DB. KERIA does not yet handle what happens when deleting a chained credential, so maybe beware. I only realised this because I was trying to delete the QVI credential in the integration test and was getting a 404.
Once WebOfTrust/keria#339 addressed, if the result is deleting the chained credentials below that point, the tests here could be updated.