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

[F] version 3.0.0 - major changes in api #2

Merged
merged 11 commits into from
Jun 14, 2018
Merged

Conversation

mikafinja
Copy link
Contributor

spec for api version 3.0.0

price:
type: integer
default: 150
barcode:
type: string
image:
Copy link
Contributor

@YtvwlD YtvwlD Sep 27, 2017

Choose a reason for hiding this comment

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

How is this supposed to work?
Currently (mete/v1) we are POSTing (or PATCHing) a file called logo along with the other parameters.
(Which isn't documented, yet.)

Edit: Nevermind.

name: start
required: true
type: string
description: start date in ISO8601 format
Copy link
Contributor

Choose a reason for hiding this comment

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

I really like this. 😃
The date format is currently very ugly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it's ugly and these parameter arrays are only supported by php and ruby. Afaik no other language is capable of handling these properly.

Copy link
Contributor

@YtvwlD YtvwlD Sep 29, 2017

Choose a reason for hiding this comment

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

We could use type: date for this. (see Swagger Spec and RFC 3339)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I'm still learning swagger and when I raised the pull request, I didn't know about it :(

name: id
type: integer
required: true
description: Numeric ID of the user to get.
Copy link
Contributor

Choose a reason for hiding this comment

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

User?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well I think an old relic of an idea I had before... Will be removed. Soon™

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I think it was a c'n'p error :(

global_credit_limit:
type: integer
example: 2000
description: globale credit limit in cent; to disable set to false
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yapp!

price:
type: integer
default: 150
barcode:
Copy link
Contributor

@YtvwlD YtvwlD Sep 27, 2017

Choose a reason for hiding this comment

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

A product can have multiple barcodes, currently. It would be nice to preserve this.
We used to have endpoints on /barcodes/ for this stuff.
(But this isn't documented in v1, yet.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, I'll rewrite this. It's true. A product may have multiple barcodes during it's lifetime.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you can wait a few days, the current endpoints under /barcodes in Mete will be in v1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sure!

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -529,6 +665,8 @@ definitions:
balance:
description: in cents
type: integer
barcode:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do users have barcodes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is to implement a kind of 'payment card'. With these you can have smaller devices which only have a small lcd. The user first scans his card, than his credit is shown and afterwards the product is scanned and the transaction is completed.

@YtvwlD
Copy link
Contributor

YtvwlD commented Sep 27, 2017

All in all, this looks great. I really like the date format and to option to upload images for users.
Do we want to keep the email field and / or the Gravatar integration?

I don't really know about the barcode stuff. The spec for v1 doesn't include it, but um, that's not your fault.

Copy link
Contributor Author

@mikafinja mikafinja left a comment

Choose a reason for hiding this comment

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

Hmm, not sure. I would prefere to drop the gravatar service to get rid of any external dependencies. An I think we should keep any data local and not rely on external services. An it's more "datensparsam" :)

name: id
type: integer
required: true
description: Numeric ID of the user to get.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well I think an old relic of an idea I had before... Will be removed. Soon™

global_credit_limit:
type: integer
example: 2000
description: globale credit limit in cent; to disable set to false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yapp!

price:
type: integer
default: 150
barcode:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, I'll rewrite this. It's true. A product may have multiple barcodes during it's lifetime.

@@ -529,6 +665,8 @@ definitions:
balance:
description: in cents
type: integer
barcode:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is to implement a kind of 'payment card'. With these you can have smaller devices which only have a small lcd. The user first scans his card, than his credit is shown and afterwards the product is scanned and the transaction is completed.

@YtvwlD
Copy link
Contributor

YtvwlD commented Sep 29, 2017

Transferring amounts between users would probably be nice (see mete#46).

@YtvwlD
Copy link
Contributor

YtvwlD commented Sep 30, 2017

I'd like to announce the possible amounts to deposit via the API, too (see mete#5).

@mikafinja
Copy link
Contributor Author

Barcodes updated to be compatible with mete, amount of calories and sugar for products added, transfers between users, amounts of deposits are read- and setable via api (denominations)

Copy link
Contributor

@YtvwlD YtvwlD left a comment

Choose a reason for hiding this comment

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

Thank you!

patch:
tags:
- barcodes
summary: update an exsisting barcode
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo. :)

- created_at
- updated_at
properties:
id:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do barcodes need to have an id? The barcode content should be unique, anyway.
(If they don't have one, we won't be able to update a barcode later on. But this might not be needed.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The barcodes have to be unique, otherwise you won't be able to tell them apart. I tried to maintain an unique way to form the api endpoints. Therefore I decided to assign an id to them.

updated_at:
type: string
format: date-time

Copy link
Contributor

Choose a reason for hiding this comment

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

A final newline would be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ummm, there is a final newline…

@YtvwlD
Copy link
Contributor

YtvwlD commented Oct 31, 2017

I'm not really decided about whether barcodes should have an id different to their content.
But otherwise, this looks really nice.

@nomaster @marudor Any thoughts on this?

@marudor
Copy link
Contributor

marudor commented Nov 1, 2017

Indifferent about Barcods.

@mikafinja
Copy link
Contributor Author

just added a new path to get user's details via an associated barcode. It is required to build small checkout devices with a 'payment card' as discussed above.

@YtvwlD
Copy link
Contributor

YtvwlD commented Feb 7, 2018

I would like to keep the attribute bottle_size (or something like this).
And I would like to have defaults for creating new entities.
And if the typo could be corrected, I'm for merging this.

@marudor
Copy link
Contributor

marudor commented Apr 19, 2018

I'm all for merging now.

@YtvwlD
Copy link
Contributor

YtvwlD commented Apr 19, 2018

Could we make the support for uploading user avatars optional (eg. make the server just return an error message or something like that)?

This would help in implementing this API version in mete, because uploading images could be done later™.

(Clients wishing to support older API versions will have to make this optional, either way.)

@marudor
Copy link
Contributor

marudor commented Apr 19, 2018

How about we treat /image as optional (No Idea how to show that in swagge actually) and add a capabilities to /info.json.
If we return something like capabilities: { image: true } server supports /image.
Otherwise not.

@mikafinja
Copy link
Contributor Author

/images is optional. You can query the server if images are supported or not ny sending a get request to /images and it'll return 204 if the server is capable and 501 if the feature isn't implemented (yet).

@@ -104,9 +90,11 @@ paths:
type: integer
product:
type: integer
401:
description: Audits deactived by user
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@marudor marudor merged commit 99b7584 into Space-Market:master Jun 14, 2018
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