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.

Sign up for GitHub

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

Jump to bottom

66/json api #73

Merged
merged 13 commits into from
Dec 15, 2018
Merged

66/json api #73

merged 13 commits into from
Dec 15, 2018

Conversation

JanKoppe
Copy link
Contributor

@JanKoppe JanKoppe commented Mar 7, 2017

don't merge yet, work in progress.

@coveralls
Copy link

coveralls commented Mar 7, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.3%) to 89.009% when pulling ca709b7 on 66/json_api into f61b006 on master.

@lkiesow
Copy link
Member

lkiesow commented Apr 15, 2017

Hi @JanKoppe,
I rebased this branch to the current state of master and continued your work on this. It is mostly finished but not quite yet. What is left is to add tests and maybe make error messages a bit more pretty.

@coveralls
Copy link

coveralls commented Apr 15, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-5.6%) to 91.395% when pulling b697035 on 66/json_api into d19c77e on master.

@lkiesow
Copy link
Member

lkiesow commented Apr 15, 2017

Btw, the old commits are still in 66/json_api-backup

@coveralls
Copy link

coveralls commented Apr 17, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.2%) to 97.248% when pulling 0ed5a7f on 66/json_api into d19c77e on master.

@coveralls
Copy link

coveralls commented Apr 17, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.2%) to 97.248% when pulling bf0c704 on 66/json_api into d19c77e on master.

@lkiesow
Copy link
Member

lkiesow commented Apr 17, 2017

This now returns valid json strings. Even for errors.
Also all REST endpoints are tested.

I think we should move some of the modules around since the names do not fit that well anymore but I would do that in another pull request:

  • ui → web
  • jsonapi → rest or api
  • ui/_init_ → web/ui
  • ui/_init_.app + ui/utils → web/app

That way, we should hopefully also be able to get rid of the ugly circular dependencies

This was referenced Apr 17, 2017
Closed
Open
Closed
@coveralls
Copy link

coveralls commented May 4, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.1%) to 96.577% when pulling c89b168 on 66/json_api into 64bffdd on master.

@JanKoppe JanKoppe changed the title WIP 66/json api 66/json api May 28, 2017
@JanKoppe
Copy link
Contributor Author

Hey @lkiesow, I've rebased this to current master and fixed a small issue with serializing of event data. This tests fine on Python 3.7 as well. Can you take a quick look at this? I'm not sure where we exactly left this PR, and I'd love to get this out in the very near future.

@JanKoppe JanKoppe requested a review from lkiesow August 10, 2018 21:22
@JanKoppe JanKoppe mentioned this pull request Aug 11, 2018
Closed
JanKoppe and others added 2 commits December 14, 2018 00:16
@JanKoppe @lkiesow
fix serializing of event data
a36f422
@lkiesow
Fix Content-Type Access
2561e03 
This patch fixes a minor issue where pyCA would end up with an internal
server error in no content type was sent in an API request.
@lkiesow
Copy link
Member

lkiesow commented Dec 13, 2018

Generally looks good. I fixed a minor issue where you would get an internal server error when you don't send a content-type header but that's all I've found. I'll give it another look tomorrow but then likely merge it.

Long term, we should maybe add pagination to listing requests like /api/events but that's something we can still add later.

lkiesow
lkiesow approved these changes Dec 15, 2018
View reviewed changes
Copy link
Member

@lkiesow lkiesow left a comment

Choose a reason for hiding this comment

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

Finished the review.
Worked as expected after a few very minor modifications.
I've also added some documentation for the API.

@lkiesow
No Content-Type requirement for GET
d70610d 
This patch drops the hard requirement for content type specification for
get requests since they do not contain a data body anyway.
@lkiesow
Fix Event Status Modification via API
e4ccaf1 
This patch fixes the event status modification via API. Additionally,
the output format is allowed to be used for input in contrast to only
accepting the database internal representation.
@lkiesow
API Documentation
c89b168
@lkiesow lkiesow merged commit c89b168 into master Dec 15, 2018
@lkiesow lkiesow deleted the 66/json_api branch October 13, 2020 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lkiesow lkiesow lkiesow approved these changes

Assignees
No one assigned
Labels
ready
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JanKoppe @coveralls @lkiesow