Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

ghost
Copy link

@ghost ghost commented Nov 28, 2020

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).

@ghost ghost requested review from linuxbandit and WikiRik November 28, 2020 18:33
Copy link
Member

@WikiRik WikiRik left a 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:
Copy link
Member

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
Copy link
Member

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?

Copy link

@freitzio freitzio Nov 30, 2020

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!

Copy link

@HassanCehef HassanCehef left a 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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
example: 5
example: 5

empty line at the end of file

Comment on lines +15 to +16
description: For valid response try integer IDs with value >= 1 and <= 10. Other
values will generated exceptions

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
components:
components:

I like line breaks

content: {}
404:
description: Action Agenda not found
content: {}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
properties:
required:
- name
- minimum
- maximum
properties:

$ref: '#/components/schemas/FocusArea'
FocusArea:
type: object
properties:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
properties:
required:
- name
- actions
properties:

type: array
items:
$ref: '#/components/schemas/FocusArea'
FocusArea:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
FocusArea:
FocusArea:

type: array
items:
$ref: '#/components/schemas/Action'
Action:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Action:
Action:

type: array
items:
$ref: "#/components/schemas/Indicator"
Indicator:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Indicator:
Indicator:

components:
schemas:
ActionAgenda:
type: array

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.

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.

3 participants