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

API: Leave Group issue solving #27

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Signior-X
Copy link
Member

@Signior-X Signior-X commented Mar 29, 2020

Fixes #18
I have make the changes in app.py which adding the API for leaving the group. Also checked it using the postman and it is working. After deleting the data from ClubMembers table, it will return if it is success or not in JSON form.
Also added a file, to see the tables list.

@Signior-X Signior-X changed the title # I have make the changes in app.py which adding the API for leaving the group. Also checked it using the postman and it is working. After deleting the data from ClubMembers table, it will return if it is succesfull or not in json form. Also added a file, to see the tables list.Added Leave Group API API: Leave Group issue solving Mar 29, 2020
Make_Tables/mysqlconnect.py Outdated Show resolved Hide resolved
app.py Outdated
@@ -20,5 +21,28 @@ def Login():
flash('Login Unsuccessful. Invalid Email/Password')
return render_template('login.html',title='Login | SAC Portal, IIT Mandi',form=form)

#Example https://host/leaveclub?userID='B19188'&clubID='C10001'
@app.route('/leaveclub', methods = ['GET','POST'])
Copy link
Member

Choose a reason for hiding this comment

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

  • Make this route, POST only

Copy link
Member Author

Choose a reason for hiding this comment

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

It is showing method not allowed in POST, I thing it is GET, so I have make this route only GET

Copy link
Member

Choose a reason for hiding this comment

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

it will show method not allowed if you access it through browsers address bar because it only sends get request. that is meant to happen. that is why you should test api with postman
Screenshot from 2020-03-31 00-43-52

Copy link
Member

Choose a reason for hiding this comment

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

Also here i have used query params but use request body to transfer data

Copy link
Collaborator

Choose a reason for hiding this comment

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

You have changed the route method, but you are still using query params for data tranfer

Make_Tables/showtables.py Outdated Show resolved Hide resolved
@Milind712000
Copy link
Member

Milind712000 commented Mar 29, 2020

@Signior-X
Copy link
Member Author

Fixes #18 I made the changes in app.py as you suggested.
Sorry, I added the showtables.py by mistake, I use it on my local machine to instantly get the schema of the tables. I added it in this commit by mistake and removed it.
I will always add a new line, learnt a new thing.
Also, that method is useless, so I deleted it.
Making the new pull request.

@Signior-X
Copy link
Member Author

Also, please don't merge it now, I think merge it after the api of add groups and add members is created, so that they don't get disturbed to update their PR.

app.py Outdated Show resolved Hide resolved
app.py Outdated
@@ -20,5 +21,28 @@ def Login():
flash('Login Unsuccessful. Invalid Email/Password')
return render_template('login.html',title='Login | SAC Portal, IIT Mandi',form=form)

#Example https://host/leaveclub?userID='B19188'&clubID='C10001'
@app.route('/leaveclub', methods = ['GET','POST'])
Copy link
Member

Choose a reason for hiding this comment

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

it will show method not allowed if you access it through browsers address bar because it only sends get request. that is meant to happen. that is why you should test api with postman
Screenshot from 2020-03-31 00-43-52

@Milind712000
Copy link
Member

Another thing, You don't need to commit amend or rebase everytime you make a change,
Its fine if your PR has multiple commits infact it makes it easier to see the changes that you make after each review, I'll tell you to rebase and squash once the PR is ready for merge and no more changes are required

@Signior-X
Copy link
Member Author

Signior-X commented Mar 31, 2020

Okay, I will use only postman.
Yes, I understand, what you are saying.
And one thing, thank you I am learning a lot of new things because of all this.

app.py Outdated
def leave_club():
data = {
"tablename" : "ClubMembers",
"userID" : request.args.get('userID'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

use request body to transfer data, instead of query parameters,
this'll allow for direct HTML form calls

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, userstood for sure, will take care from next time in PRs
However, changing it and doing it as vipul suggested, to not take any parameter, isnted use the session value.

@@ -20,5 +21,28 @@ def Login():
flash('Login Unsuccessful. Invalid Email/Password')
return render_template('login.html',title='Login | SAC Portal, IIT Mandi',form=form)

#Example https://host/leaveclub?userID='B19188'&clubID='C10001'
@app.route('/leaveclub', methods = ['POST'])

Choose a reason for hiding this comment

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

Not sure about this comment. @vsvipul @Milind712000 what do you think about this?
Guard this function to check whether the user is logged in. If the user is logged in we don't need to store the userid.
If you are thinking about admin functionalities, we can have a different function for that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, done, understood.
I will do this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will only take the clubID in the body, and take the userID from the session.

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.

API: Leave Club
4 participants