-
Notifications
You must be signed in to change notification settings - Fork 1
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
[MA-96] describe getActionAgendaById API #2
base: master
Are you sure you want to change the base?
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'm not really knowledgeable on OpenAPI documentation so my comments are not really specific to OpenAPI. But overall I think it looks good for a guideline on how to program this.
required: true | ||
schema: | ||
type: string | ||
responses: |
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 would add a response of 401 here, saying that the user is unauthorized to do view the Action Agenda
tags: | ||
- Action Agenda | ||
summary: Display a specified Action Agenda | ||
description: For valid response try integer IDs with value >= 1 and <= 10. Other |
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 don't really understand this. If the ID is between 1 and 10 it will give an Action Agenda and otherwise it will generate an exception? Won't it generate 404 errors for every non-existing Action Agenda?
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.
That was a leftover of the base we used. This is only in the description here and not an actual limitation of the values but indeed this needs to change. Thanks for noticing it!
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.
Some nitpicky suggestions to make the contract a bit stricter.
With the current API, we always need to play a guessing game as to which action agenda ID exists.
Two things:
- Shouldn't we also already include an API to return all tuples [action agenda ID, action agenda name]? otherwise we must guess, thus the
GET /actionagenda/{aaId}
we offer is not usable - For all the objects we return, I think we must include their PK identifier. (so, aaId, faId, aID, iId, or AAid, FAid, Aid, Iid) (granted, we can add this to the contract at the next update, but since we're allowed BC now before launching the product, it's fine to include them now). If we want to change an indicator, we'll need to know its PK in the DB, because that's the only way we can manipulate them.
example: 1 | ||
maximum: | ||
type: integer | ||
example: 5 |
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.
example: 5 | |
example: 5 | |
empty line at the end of file
description: For valid response try integer IDs with value >= 1 and <= 10. Other | ||
values will generated exceptions |
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.
description: For valid response try integer IDs with value >= 1 and <= 10. Other | |
values will generated exceptions | |
description: > | |
Get one Action Agenda by ID |
?
404: | ||
description: Action Agenda not found | ||
content: {} | ||
components: |
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.
components: | |
components: |
I like line breaks
content: {} | ||
404: | ||
description: Action Agenda not found | ||
content: {} |
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.
content: {} | |
content: {} | |
default: | |
description: Error |
This fixes the 401 request by Rik. I don't think we need to specify here the meaning of 401, because the whole API is only accessible with authN
$ref: "#/components/schemas/Indicator" | ||
Indicator: | ||
type: object | ||
properties: |
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.
properties: | |
required: | |
- name | |
- minimum | |
- maximum | |
properties: |
$ref: '#/components/schemas/FocusArea' | ||
FocusArea: | ||
type: object | ||
properties: |
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.
properties: | |
required: | |
- name | |
- actions | |
properties: |
type: array | ||
items: | ||
$ref: '#/components/schemas/FocusArea' | ||
FocusArea: |
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.
FocusArea: | |
FocusArea: |
type: array | ||
items: | ||
$ref: '#/components/schemas/Action' | ||
Action: |
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.
Action: | |
Action: |
type: array | ||
items: | ||
$ref: "#/components/schemas/Indicator" | ||
Indicator: |
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.
Indicator: | |
Indicator: |
components: | ||
schemas: | ||
ActionAgenda: | ||
type: array |
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.
When we return an ActionAgenda, we must also return its ID, because that's the only way we can manipulate this object.
Solves: [https://myaegee.atlassian.net/browse/MA-96]
First description of the API to return an Action Agenda by Id with all necessary sub-objects.
We'd like to get feedback and recommendations, as well as input on how the security should look like (we removed it for now).