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

Fixes #832 : Adds JSON-API standards #847

Merged
merged 1 commit into from May 27, 2018

Conversation

@gabru-md
Member

gabru-md commented May 25, 2018

Fixes #835

Checklist

  • I have read the Contribution & Best practices Guide and my PR follows them.
  • My branch is up-to-date with the Upstream development branch.
  • I have added necessary documentation (if appropriate)

Preview Link

Changes proposed in this pull request:

  • Adds jsonAPI standards to the API
@gabru-md

This comment has been minimized.

Show comment
Hide comment
@gabru-md

gabru-md May 25, 2018

Member

CC : @djmgit @harshit98 @yashLadha @ParthS007
can you please review it?
I can iterate over it in my further PRs if this can be merged. I will post screenshots showing the changes to backend. It took me a lot of time and I would like to iterate over it in my upcoming PRs.
I shall be thankful if this can be done

Member

gabru-md commented May 25, 2018

CC : @djmgit @harshit98 @yashLadha @ParthS007
can you please review it?
I can iterate over it in my further PRs if this can be merged. I will post screenshots showing the changes to backend. It took me a lot of time and I would like to iterate over it in my upcoming PRs.
I shall be thankful if this can be done

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot May 25, 2018

Codecov Report

Merging #847 into development will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           development   #847   +/-   ##
==========================================
  Coverage          100%   100%           
==========================================
  Files                1      1           
  Lines               43     43           
==========================================
  Hits                43     43

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ae4d76...baca4fe. Read the comment docs.

codecov bot commented May 25, 2018

Codecov Report

Merging #847 into development will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           development   #847   +/-   ##
==========================================
  Coverage          100%   100%           
==========================================
  Files                1      1           
  Lines               43     43           
==========================================
  Hits                43     43

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ae4d76...baca4fe. Read the comment docs.

@gabru-md

This comment has been minimized.

Show comment
Hide comment
@gabru-md

gabru-md May 25, 2018

Member

screenshot_2018-05-25_14-48-41
screenshot_2018-05-25_14-48-15
screenshot_2018-05-25_14-48-08

The above Screenshots show the change in API standards.
@yashLadha is it fine? Please let me know and can this be merged so that I can iterate over this PR?

The routes in the image are different, I changed them before pushing them to my remote

Member

gabru-md commented May 25, 2018

screenshot_2018-05-25_14-48-41
screenshot_2018-05-25_14-48-15
screenshot_2018-05-25_14-48-08

The above Screenshots show the change in API standards.
@yashLadha is it fine? Please let me know and can this be merged so that I can iterate over this PR?

The routes in the image are different, I changed them before pushing them to my remote

@yashLadha

Looking fine to me and available for the consumption in frontend. Nice work @gabru-md , just need some clarification on these routes inline.

@gabru-md

This comment has been minimized.

Show comment
Hide comment
@gabru-md

gabru-md May 25, 2018

Member

These schemas are just layers of abstraction to be offered over the contemporary data structure provided in and as the model for the respective database table.

The self_url specifies the URL that the particular layer of abstraction or the schema is associated with.
The schemas are being used inside the specific routes to offer data abstraction.

Am I clear @yashLadha ?

Member

gabru-md commented May 25, 2018

These schemas are just layers of abstraction to be offered over the contemporary data structure provided in and as the model for the respective database table.

The self_url specifies the URL that the particular layer of abstraction or the schema is associated with.
The schemas are being used inside the specific routes to offer data abstraction.

Am I clear @yashLadha ?

@yashLadha

This comment has been minimized.

Show comment
Hide comment
@yashLadha

yashLadha May 25, 2018

Member

I understood the logic for the layers of abstraction provided in the schema, just saw the screenshots for the url. Understood as they are for just testing purpose, so its ok then 👍

Member

yashLadha commented May 25, 2018

I understood the logic for the layers of abstraction provided in the schema, just saw the screenshots for the url. Understood as they are for just testing purpose, so its ok then 👍

@vaibhavsingh97

Good work @gabru-md 👍

@@ -6,8 +6,8 @@
class User(db.Model):
__tablename__ = 'User'
id = db.Column(db.String(100), primary_key=True)
username = db.Column(db.String(80))
id = db.Column(db.String(100))

This comment has been minimized.

@vaibhavsingh97

vaibhavsingh97 May 25, 2018

Member

why removed primary_key=True?

@vaibhavsingh97

vaibhavsingh97 May 25, 2018

Member

why removed primary_key=True?

This comment has been minimized.

@gabru-md

gabru-md May 25, 2018

Member

user must be recognised by the username and not the id.
Rest is self explanatory.
😄

@gabru-md

gabru-md May 25, 2018

Member

user must be recognised by the username and not the id.
Rest is self explanatory.
😄

This comment has been minimized.

@vaibhavsingh97

vaibhavsingh97 May 25, 2018

Member

@gabru-md Agreed, but why id is not an primary key, so every id genrated against user must be unique. I guess, I am confused, so can you present some more information supporting the same

@vaibhavsingh97

vaibhavsingh97 May 25, 2018

Member

@gabru-md Agreed, but why id is not an primary key, so every id genrated against user must be unique. I guess, I am confused, so can you present some more information supporting the same

This comment has been minimized.

@gabru-md

gabru-md May 25, 2018

Member

I guess you are not reading the implementation of the code before creating comments.
I already stated that the answer is self explanatory, but it seems that you cannot understand so I will write here.
The id will be unique no matter what. The id being passed onto the user object is the uid generated by firebase, and which in itself is unique.
I guess you know about removing resundancy, sincr we already have a unique id taken care by firebase, then there is no need to mention it here.

I expect this much explanation is sufficient. 😄

@gabru-md

gabru-md May 25, 2018

Member

I guess you are not reading the implementation of the code before creating comments.
I already stated that the answer is self explanatory, but it seems that you cannot understand so I will write here.
The id will be unique no matter what. The id being passed onto the user object is the uid generated by firebase, and which in itself is unique.
I guess you know about removing resundancy, sincr we already have a unique id taken care by firebase, then there is no need to mention it here.

I expect this much explanation is sufficient. 😄

id = db.Column(db.String(100), primary_key=True)
username = db.Column(db.String(80))
id = db.Column(db.String(100))
username = db.Column(db.String(80), primary_key=True)

This comment has been minimized.

@vaibhavsingh97

vaibhavsingh97 May 25, 2018

Member

one can add unique=True

@vaibhavsingh97

vaibhavsingh97 May 25, 2018

Member

one can add unique=True

This comment has been minimized.

@gabru-md

gabru-md May 25, 2018

Member

both are different.
primary_key will be used here.
cheers!

@gabru-md

gabru-md May 25, 2018

Member

both are different.
primary_key will be used here.
cheers!

Show outdated Hide outdated backend/blueprint/api/controllers/loginUser.py Outdated
Show outdated Hide outdated backend/blueprint/api/controllers/fileUploader.py Outdated
@djmgit

Looks good, please look inline for my question, a little confusion.

Adds jsonAPI standards
fix

fix

fix4

fix

fix
@gabru-md

This comment has been minimized.

Show comment
Hide comment
@gabru-md

gabru-md May 25, 2018

Member

@djmgit done, please review again. Thanks

Member

gabru-md commented May 25, 2018

@djmgit done, please review again. Thanks

@djmgit

djmgit approved these changes May 25, 2018

Looks good 👍

@open-event-bot open-event-bot bot removed the needs-review label May 25, 2018

@djmgit djmgit requested review from vaibhavsingh97 and ParthS007 May 25, 2018

@djmgit

This comment has been minimized.

Show comment
Hide comment
@djmgit

djmgit May 25, 2018

Member

lets wait for others to review.

Member

djmgit commented May 25, 2018

lets wait for others to review.

@vaibhavsingh97

vaibhavsingh97 suggested changes May 25, 2018 edited

Please look inline comments and also when I tried to register, it not able to do that task
Screenshot for your reference:
image

input_data = request.get_json()
data, err = schema.load(input_data)
if err:
return err

This comment has been minimized.

@vaibhavsingh97

vaibhavsingh97 May 25, 2018

Member

@gabru-md This will return an error to the console, but what will the user see? Please make use Response class and return proper error to the user endpoint

@vaibhavsingh97

vaibhavsingh97 May 25, 2018

Member

@gabru-md This will return an error to the console, but what will the user see? Please make use Response class and return proper error to the user endpoint

This comment has been minimized.

@gabru-md

gabru-md May 25, 2018

Member

I will iterate over this PR.
So incase you are worried about the error stuff, this is just a basic implementation.
Since it works and @yashLadha said it is important, therefore I have given a basic setup so that the frontend can be linked.

If you have any other questions I would suggest waiting for a day or two, so that I can work with @ParthS007 and @yashLadha.

@gabru-md

gabru-md May 25, 2018

Member

I will iterate over this PR.
So incase you are worried about the error stuff, this is just a basic implementation.
Since it works and @yashLadha said it is important, therefore I have given a basic setup so that the frontend can be linked.

If you have any other questions I would suggest waiting for a day or two, so that I can work with @ParthS007 and @yashLadha.

This comment has been minimized.

@yashLadha

yashLadha May 26, 2018

Member

@vaibhavsingh97 #848 can you refer to the comments there, cheers 👍

@yashLadha

yashLadha May 26, 2018

Member

@vaibhavsingh97 #848 can you refer to the comments there, cheers 👍

@@ -29,14 +27,6 @@ def registerUser():
email=user.email,

This comment has been minimized.

@vaibhavsingh97

vaibhavsingh97 May 25, 2018

Member

why email is mentioned twice in auth.create_user?

@vaibhavsingh97

vaibhavsingh97 May 25, 2018

Member

why email is mentioned twice in auth.create_user?

This comment has been minimized.

@gabru-md

gabru-md May 25, 2018

Member

Sorry, but what ?

@gabru-md

gabru-md May 25, 2018

Member

Sorry, but what ?

This comment has been minimized.

@gabru-md

gabru-md May 25, 2018

Member

There is a user for backend and a user for firebase.
I guess you should look more closely 😕

@gabru-md

gabru-md May 25, 2018

Member

There is a user for backend and a user for firebase.
I guess you should look more closely 😕

This comment has been minimized.

@vaibhavsingh97

vaibhavsingh97 May 25, 2018

Member

@gabru-md ignore this comment, I have seen something wrong 😅

@vaibhavsingh97

vaibhavsingh97 May 25, 2018

Member

@gabru-md ignore this comment, I have seen something wrong 😅

This comment has been minimized.

@gabru-md

gabru-md May 25, 2018

Member

One of many

@gabru-md

gabru-md May 25, 2018

Member

One of many

@@ -6,8 +6,8 @@
class User(db.Model):
__tablename__ = 'User'
id = db.Column(db.String(100), primary_key=True)
username = db.Column(db.String(80))
id = db.Column(db.String(100))

This comment has been minimized.

@vaibhavsingh97

vaibhavsingh97 May 25, 2018

Member

@gabru-md Agreed, but why id is not an primary key, so every id genrated against user must be unique. I guess, I am confused, so can you present some more information supporting the same

@vaibhavsingh97

vaibhavsingh97 May 25, 2018

Member

@gabru-md Agreed, but why id is not an primary key, so every id genrated against user must be unique. I guess, I am confused, so can you present some more information supporting the same

@yashLadha

LGTM

@gabru-md

This comment has been minimized.

Show comment
Hide comment
@gabru-md

gabru-md May 26, 2018

Member

@djmgit please merge

Member

gabru-md commented May 26, 2018

@djmgit please merge

@djmgit

djmgit approved these changes May 27, 2018

@djmgit djmgit merged commit 2f593ef into fossasia:development May 27, 2018

4 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
codecov/patch Coverage not affected when comparing 5ae4d76...baca4fe
Details
codecov/project 100% remains the same compared to 5ae4d76
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment