Skip to content

[MA-96] describe getActionAgendaById API - #2

Open
ghost wants to merge 1 commit into
masterfrom
feature/ma-96
Open

[MA-96] describe getActionAgendaById API#2
ghost wants to merge 1 commit into
masterfrom
feature/ma-96

Conversation

@ghost

@ghost ghost commented Nov 28, 2020

Copy link
Copy Markdown

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 WikiRik and linuxbandit November 28, 2020 18:33

@WikiRik WikiRik left a comment

Copy link
Copy Markdown
Member

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.

Comment thread strategy-contract.yaml
required: true
schema:
type: string
responses:

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

Comment thread strategy-contract.yaml
tags:
- Action Agenda
summary: Display a specified Action Agenda
description: For valid response try integer IDs with value >= 1 and <= 10. Other

Copy link
Copy Markdown
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?

@freitzio freitzio Nov 30, 2020

Copy link
Copy Markdown

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!

Comment thread strategy-contract.yaml
Comment thread strategy-contract.yaml
Comment thread strategy-contract.yaml
Comment thread strategy-contract.yaml

@HassanCehef HassanCehef left a comment

Copy link
Copy Markdown

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.

Comment thread strategy-contract.yaml
example: 1
maximum:
type: integer
example: 5 No newline at end of file

Copy link
Copy Markdown

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 thread strategy-contract.yaml
Comment on lines +15 to +16
description: For valid response try integer IDs with value >= 1 and <= 10. Other
values will generated exceptions

Copy link
Copy Markdown

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

?

Comment thread strategy-contract.yaml
404:
description: Action Agenda not found
content: {}
components:

Copy link
Copy Markdown

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

Comment thread strategy-contract.yaml
content: {}
404:
description: Action Agenda not found
content: {}

Copy link
Copy Markdown

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

Comment thread strategy-contract.yaml
$ref: "#/components/schemas/Indicator"
Indicator:
type: object
properties:

Copy link
Copy Markdown

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:

Comment thread strategy-contract.yaml
$ref: '#/components/schemas/FocusArea'
FocusArea:
type: object
properties:

Copy link
Copy Markdown

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:

Comment thread strategy-contract.yaml
type: array
items:
$ref: '#/components/schemas/FocusArea'
FocusArea:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
FocusArea:
FocusArea:

Comment thread strategy-contract.yaml
type: array
items:
$ref: '#/components/schemas/Action'
Action:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
Action:
Action:

Comment thread strategy-contract.yaml
type: array
items:
$ref: "#/components/schemas/Indicator"
Indicator:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
Indicator:
Indicator:

Comment thread strategy-contract.yaml
components:
schemas:
ActionAgenda:
type: array

Copy link
Copy Markdown

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