WIP - feature/basic api for app #99

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

Jump to bottom

Open

Ombental wants to merge 1 commit into PythonFreeCourse:develop

from Ombental:feature/basic-api-for-app

Open

WIP - feature/basic api for app #99

Ombental wants to merge 1 commit into PythonFreeCourse:develop from Ombental:feature/basic-api-for-app

Conversation

Ombental

Copy link

@Ombental Ombental commented Jan 20, 2021

basic implementation, no docs on front, no tests, no documentation yet

Hey, want some CRs for code


 WIP feat(api): basic implementation, no docs on front, no tests, no d...

451a53f

...ocumentation yet

@Ombental Ombental changed the base branch from main to develop

January 20, 2021 17:53

@yammesicka yammesicka closed this

Jan 21, 2021

@yammesicka yammesicka deleted the branch PythonFreeCourse:develop

January 21, 2021 02:36

@yammesicka yammesicka reopened this

Jan 21, 2021

IdanPelled

IdanPelled reviewed

Jan 21, 2021

View reviewed changes

app/routers/api.py

api_routes = [{'name': '/new_event',}, {'name': '/{date}'}]

api_key = session.query(Token).filter_by(owner_id=user.id).first()

session.close()

Copy link

Contributor

@IdanPelled IdanPelled Jan 21, 2021

Choose a reason for hiding this comment

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

Is it necessary? The session is being closed at the end of the get_db

IdanPelled

IdanPelled reviewed

Jan 21, 2021

View reviewed changes

app/templates/api_docs.html

type="text/javascript"

src="{{ url_for('static', path='/apiKeyGenerator.js') }}"

></script>

{% endblock %}

Copy link

Contributor

@IdanPelled IdanPelled Jan 21, 2021 •

edited

Loading

Choose a reason for hiding this comment

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

great work!
remember, tests and documentation are essential

ivarshav

ivarshav reviewed

Jan 21, 2021

View reviewed changes

app/database/database.py

@@ -8,11 +8,13 @@

SQLALCHEMY_DATABASE_URL = os.getenv(

"DATABASE_CONNECTION_STRING", config.DEVELOPMENT_DATABASE_STRING)

"DATABASE_CONNECTION_STRING2", config.DEVELOPMENT_DATABASE_STRING)

Copy link

Contributor

@ivarshav ivarshav Jan 21, 2021

Choose a reason for hiding this comment

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

mistake?

ivarshav

ivarshav reviewed

Jan 21, 2021

View reviewed changes

app/database/database.py

#pool_pre_ping=True for POSTGRES

Copy link

Contributor

@ivarshav ivarshav Jan 21, 2021

Choose a reason for hiding this comment

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

remove

ivarshav

ivarshav reviewed

Jan 21, 2021

View reviewed changes

app/database/models.py

@@ -32,3 +33,11 @@ class Event(Base):

owner_id = Column(Integer, ForeignKey("users.id"))

owner = relationship("User", back_populates="events")

class Token(Base):

Copy link

Contributor

@ivarshav ivarshav Jan 21, 2021

Choose a reason for hiding this comment

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

I think it could be nice to add a short documentation for this table.

ivarshav

ivarshav reviewed

Jan 21, 2021

View reviewed changes

app/static/apiKeyGenerator.js

callAPIKeyRoute("/api/generate_key", { user: user_id, refresh: state }).then(

(data) => {

let keyText = document.getElementById("apiKeyHolder");

keyText.textContent = "API Key: " + data.key;

Copy link

Contributor

@ivarshav ivarshav Jan 21, 2021

Choose a reason for hiding this comment

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

can't you use f"string {param}" ?

ivarshav

ivarshav reviewed

Jan 21, 2021

View reviewed changes

app/static/popover.js

@@ -1,9 +1,9 @@

// Enable bootstrap popovers

// // Enable bootstrap popovers

Copy link

Contributor

@ivarshav ivarshav Jan 21, 2021

Choose a reason for hiding this comment

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

Please revisit this file and remove unneeded code.

yammesicka

yammesicka requested changes

Jan 22, 2021

View reviewed changes

app/database/database.py

@@ -8,11 +8,13 @@

SQLALCHEMY_DATABASE_URL = os.getenv(

"DATABASE_CONNECTION_STRING", config.DEVELOPMENT_DATABASE_STRING)

"DATABASE_CONNECTION_STRING2", config.DEVELOPMENT_DATABASE_STRING)

Copy link

Member

@yammesicka yammesicka Jan 20, 2021

Choose a reason for hiding this comment

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

Please make sure you don't commit this change :)

app/database/models.py

id = Column(String, primary_key=True, index=True)

owner_id = Column(Integer, ForeignKey("users.id"), nullable=False, unique=True)

owner = relationship("User", back_populates="token")

Copy link

Member

@yammesicka yammesicka Jan 20, 2021

Choose a reason for hiding this comment

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

As a good practice, we can add expiry_date. If you really into it, read about security in APIs: https://owasp.org/www-project-api-security/

app/routers/api.py

return JSONResponse(jsonable_encoder({'key': token}))

@key_gen_router.post('/delete_key')

Copy link

Member

@yammesicka yammesicka Jan 20, 2021

Choose a reason for hiding this comment

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

Maybe we should use @key_gen_route.delete?

app/routers/api.py

api_routes = [{'name': '/new_event',}, {'name': '/{date}'}]

api_key = session.query(Token).filter_by(owner_id=user.id).first()

session.close()

no_api_key = True

Copy link

Member

@yammesicka yammesicka Jan 20, 2021

Choose a reason for hiding this comment

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

Maybe we can change these lines to key = getattr(api_key, 'id', None) and remove the no_api_key variable?

app/routers/api.py

from typing import Optional

def check_api_key(key: str, session=Depends(get_db)):

Copy link

Member

@yammesicka yammesicka Jan 20, 2021

Choose a reason for hiding this comment

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

We should implement rate limit to this check

app/routers/api.py

data = await request.json()

if data.get('refresh', False):

session.query(Token).filter_by(owner_id=data['user']).delete()

token = secrets.token_urlsafe(32)

Copy link

Member

@yammesicka yammesicka Jan 20, 2021

Choose a reason for hiding this comment

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

Great job for using secrets!

app/static/api_style.css

@@ -0,0 +1,8 @@

#apiKeyDelete {

font-size: 0.6rem;

margin-left: 20px;

Copy link

Member

@yammesicka yammesicka Jan 22, 2021

Choose a reason for hiding this comment

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

Prefer working with em/%

ivarshav

ivarshav reviewed

Jan 22, 2021

View reviewed changes

app/routers/api.py

session.commit()

session.close()

return JSONResponse(jsonable_encoder({'success': True}))

Copy link

Contributor

@ivarshav ivarshav Jan 22, 2021

Choose a reason for hiding this comment

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

Too many blank lines :)

ivarshav

ivarshav reviewed

Jan 22, 2021

View reviewed changes

app/routers/api.py

session.add(event)

session.commit()

d['id'] = event.id

session.close()

Copy link

Contributor

@ivarshav ivarshav Jan 22, 2021

Choose a reason for hiding this comment

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

same as @IdanPelled comment

Labels

None yet

4 participants

@Ombental @yammesicka @ivarshav @IdanPelled

WIP - feature/basic api for app #99

Are you sure you want to change the base?

WIP - feature/basic api for app #99

Uh oh!

Conversation

@Ombental Ombental commented Jan 20, 2021

Uh oh!

@IdanPelled IdanPelled Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

@IdanPelled IdanPelled Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@ivarshav ivarshav Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

@ivarshav ivarshav Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

@ivarshav ivarshav Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

@ivarshav ivarshav Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

@ivarshav ivarshav Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

@yammesicka yammesicka Jan 20, 2021

Choose a reason for hiding this comment

Uh oh!

@yammesicka yammesicka Jan 20, 2021

Choose a reason for hiding this comment

Uh oh!

@yammesicka yammesicka Jan 20, 2021

Choose a reason for hiding this comment

Uh oh!

@yammesicka yammesicka Jan 20, 2021

Choose a reason for hiding this comment

Uh oh!

@yammesicka yammesicka Jan 20, 2021

Choose a reason for hiding this comment

Uh oh!

@yammesicka yammesicka Jan 20, 2021

Choose a reason for hiding this comment

Uh oh!

@yammesicka yammesicka Jan 22, 2021

Choose a reason for hiding this comment

Uh oh!

@ivarshav ivarshav Jan 22, 2021

Choose a reason for hiding this comment

Uh oh!

@ivarshav ivarshav Jan 22, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

@IdanPelled IdanPelled Jan 21, 2021 •

edited

Loading