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

added flask app #132

Merged
merged 13 commits into from
Jul 23, 2024
Merged

added flask app #132

merged 13 commits into from
Jul 23, 2024

Conversation

tanush-128
Copy link
Contributor

@tanush-128 tanush-128 commented Jun 8, 2024

fixes #105

Copy link
Member

@proffapt proffapt left a comment

Choose a reason for hiding this comment

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

Looks good from above. Would recommend to have more intuitive names for the endpoints

@proffapt proffapt mentioned this pull request Jun 8, 2024
8 tasks
Copy link
Contributor

@rohan-b-84 rohan-b-84 left a comment

Choose a reason for hiding this comment

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

Good start @tanush-128
I've added some comments, try to add those changes and its good to go

server.py Outdated Show resolved Hide resolved
server.py Outdated Show resolved Hide resolved
server.py Outdated Show resolved Hide resolved
server.py Outdated Show resolved Hide resolved
server.py Outdated Show resolved Hide resolved
server.py Outdated Show resolved Hide resolved
server.py Outdated Show resolved Hide resolved
Copy link
Member

@proffapt proffapt left a comment

Choose a reason for hiding this comment

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

Now it looks better. But a few changes:

  • Separate out endpoints for verify otp and download (these two are different things and will be better to be completed in their specific functions)
  • Now some name changes
    • verify otp -> login
    • send otp -> request otp
    • create_erp_session -> establish_erp_session
    • end_session -> close erp session

session_manager.py Outdated Show resolved Hide resolved
server.py Outdated Show resolved Hide resolved
server.py Outdated Show resolved Hide resolved
Copy link
Contributor

@rohan-b-84 rohan-b-84 left a comment

Choose a reason for hiding this comment

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

looks good. hope you've tested these during development. make these changes and resolve the conversations which are done.

session_manager.py Outdated Show resolved Hide resolved
session_manager.py Outdated Show resolved Hide resolved
@rohan-b-84
Copy link
Contributor

@tanush-128 link this PR to all the issues it is solving

@tanush-128
Copy link
Contributor Author

@tanush-128 link this PR to all the issues it is solving

I can't , I don't have access

@tanush-128
Copy link
Contributor Author

I would directly take the session manager and add it in package, if approved

@tanush-128 tanush-128 changed the title added flask app added flask app fixes #105 Jun 10, 2024
@tanush-128 tanush-128 changed the title added flask app fixes #105 added flask app Jun 10, 2024
server.py Outdated Show resolved Hide resolved
server.py Outdated Show resolved Hide resolved
@rohan-b-84
Copy link
Contributor

I would directly take the session manager and add it in package, if approved

Yeah, that is what I'm expecting. If session manager is added in iitkgp-erp-login, we can delete the session_manager.py file and import the session manager directly from iitkgp-erp-login

@tanush-128
Copy link
Contributor Author

I would directly take the session manager and add it in package, if approved

Yeah, that is what I'm expecting. If session manager is added in iitkgp-erp-login, we can delete the session_manager.py file and import the session manager directly from iitkgp-erp-login

yes

@proffapt
Copy link
Member

proffapt commented Jun 10, 2024

You can create the PR parallely ig? If it's possible then go ahead. Parallely with the logging PR

server.py Outdated Show resolved Hide resolved
server.py Outdated Show resolved Hide resolved
server.py Outdated Show resolved Hide resolved
server.py Outdated Show resolved Hide resolved
@proffapt proffapt requested a review from rohan-b-84 June 27, 2024 11:21
Copy link
Member

@proffapt proffapt left a comment

Choose a reason for hiding this comment

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

Refer to the following for implementing the backend logic:

server.py Outdated Show resolved Hide resolved
server.py Outdated Show resolved Hide resolved
server.py Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
server.py Outdated Show resolved Hide resolved
utils/__init__.py Outdated Show resolved Hide resolved
utils/network.py Outdated Show resolved Hide resolved
gyft.py Outdated Show resolved Hide resolved
gyft.py Outdated Show resolved Hide resolved
Copy link
Member

@proffapt proffapt left a comment

Choose a reason for hiding this comment

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

  • Rename server.py to app.py
  • Use a linter and format the codebase

gyft.py Outdated Show resolved Hide resolved
server.py Outdated Show resolved Hide resolved
server.py Outdated Show resolved Hide resolved
server.py Outdated Show resolved Hide resolved
proffapt
proffapt previously approved these changes Jul 23, 2024
Copy link
Member

@proffapt proffapt left a comment

Choose a reason for hiding this comment

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

@rohan-b-84 have a look at it once. Green single from my side.

@rohan-b-84
Copy link
Contributor

Looks good, @tanush-128 resolve the merge conflicts and we'll merge it.

@tanush-128
Copy link
Contributor Author

@rohan-b-84 I have resolved the conflicts

@proffapt proffapt merged commit fd459fc into metakgp:master Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

POA: WebApp and beyond
3 participants