Skip to content

feat: add update credential status endpoint #110

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

Merged
merged 7 commits into from
Feb 19, 2021

Conversation

tplooker
Copy link
Contributor

@tplooker tplooker commented Feb 11, 2021

This PR adds a credential issuer API for managing an update to the status of an issued credential.

Outstanding Questions

  • How should the status property relate to the type property in the request? Are we assuming that the type sets the valid enumeration of values for status? Yes
  • Is the credentialId the id of the issued credential? Yes

@OR13
Copy link
Contributor

OR13 commented Feb 11, 2021

  • Is the credentialId the id of the issued credential?

Lets do whatever will be most confusing for developers :)

@OR13
Copy link
Contributor

OR13 commented Feb 11, 2021

  • ....Are we assuming that the type sets the valid enumeration of values for status?

IMO, Yes....

Support for a type is sufficient to convey support for the enumeration of values.... thats the whole point of creating unqiue type names :)

@OR13
Copy link
Contributor

OR13 commented Feb 11, 2021

@tplooker if you can provide some sample JSON / curls that would be awesome.

@tplooker
Copy link
Contributor Author

@OR13
Copy link
Contributor

OR13 commented Feb 11, 2021

@tplooker I guess I was hoping for test suite JSON examples... but maybe better in a separate PR...

@tplooker
Copy link
Contributor Author

Ah gotcha, yes fixtures and tests will be added after this PR.

@tplooker
Copy link
Contributor Author

Would love some feedback from @msporny and @peacekeeper on this one, not trying to rush the process, just aware that this is time critical

@mprorock
Copy link
Contributor

we are blocked by this, both functionally, and from a purpose of being able to run a cleanup and consistency pass on the YAML definition file, and on beginning to finalize documentation on a more complete version of the API that matches our bare minimum use case needs.

@tplooker
Copy link
Contributor Author

Another outstanding question for this endpoint is should it be credentials/status or credentials/updateStatus

@OR13
Copy link
Contributor

OR13 commented Feb 16, 2021

credentials/status

Copy link
Contributor

@OR13 OR13 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 this PR!

@tplooker tplooker requested review from msporny and OR13 February 17, 2021 17:46
@tplooker
Copy link
Contributor Author

Ready for re-review @msporny @OR13 @peacekeeper

@tplooker
Copy link
Contributor Author

Also I have local test cases for revocation almost ready so if we can get this reviewed soon then I will file the next PR

@tplooker
Copy link
Contributor Author

@peacekeeper @msporny looking for re-reviews here PR been open 8 days, made changes yesterday as per @OR13 review and now approval, anything else from you needed before approval and merging?

@tplooker tplooker force-pushed the tl/add-update-status-endpoint branch from 5870b49 to 7105bdb Compare February 19, 2021 02:00
@tplooker tplooker merged commit 3c09fd3 into master Feb 19, 2021
@tplooker tplooker deleted the tl/add-update-status-endpoint branch February 19, 2021 02:01
@tplooker
Copy link
Contributor Author

Multiple reviews and approvals 8 days open, merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants