-
Notifications
You must be signed in to change notification settings - Fork 3
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
1750: Endpoint specification for verein360 #1773
Conversation
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.
I am not really sure of the context of this doc, but perhaps it makes sense to add a little more information on the used types? E.g. that dates need to be ISO8601 date strings.
Depending on the context my comments might be unnecessary.
docs/verein360-enpoint.md
Outdated
"organization": { | ||
"address": { | ||
"country": { | ||
"shortText": "Germany" |
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 are these shortText
attributes needed?
Not sure about this one:
"shortText": "Germany" | |
"shortText": "Deutschland" |
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.
if you send an application you can check the actual payload. But good question why this was implemented like this, maybe @michael-markl can give some input
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.
For me this looks good. Just checked the payload.
Two minor things:
- We should clarify how we get the "source" of application. But as i commented this can also be applied via the api token. I think thats better than provide here an optional string field.
- I would just add some comments to the dates (format : yyyy-mm-dd f.e.)
docs/verein360-enpoint.md
Outdated
"organization": { | ||
"address": { | ||
"country": { | ||
"shortText": "Germany" |
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.
if you send an application you can check the actual payload. But good question why this was implemented like this, maybe @michael-markl can give some input
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.
LGTM
@@ -7,9 +7,9 @@ All details about the endpoint can be found in the [API Documentation](../specs/ | |||
|
|||
## Usage | |||
|
|||
### Set Authourization Header | |||
### Authourization Header |
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.
🙃 Typo
### Authourization Header | |
### Authorization Header |
Short description
Added documentation we can sent to verein360. As soon as this PR is approved I will send this documentation to Verein360. The deadline for this is end of november.
Testing
you can call the endpoint as specified and check if this works as expected.
Resolved issues
Fixes: #1750