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 changes to flask-server to configure admin panel #498

Closed
wants to merge 13 commits into from

Conversation

carpecodeum
Copy link
Contributor

@carpecodeum carpecodeum commented Jan 15, 2020

Please provide enough information so that others can review your pull request:

added changes to flask-server to configure the admin panel on the client side

Explain the details for making this change. What existing problem does the pull request solve?
changes in the helpers.py and stories.controller.py allow the endpoint api/stories/all to be served only when the user is admin
also updated the read me to allow .env file to incorporate the credentials for admin.
Test plan (required)
tested using postman that the data of all the stories is served only when the user is an admin

Closing issues #470

@carpecodeum
Copy link
Contributor Author

@Anmolbansal1 the build is failing because the generate_password_hash method takes a not null value and i have imported an env variable for admin_password and thus stays null during the build. Please guide me as to what i should do now?

Copy link
Collaborator

@Anmolbansal1 Anmolbansal1 left a comment

Choose a reason for hiding this comment

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

  1. check for codacy errors
  2. update commit message
  3. fix travis build..
  4. also there are merge conficts (rebase your branch with master)

@carpecodeum
Copy link
Contributor Author

@Anmolbansal1 the build is failing because the generate_password_hash method takes a not null value and i have imported an env variable for admin_password and thus stays null during the build.
do you have any suggestions on what should i do?

@carpecodeum
Copy link
Contributor Author

@Anmolbansal1 what are your views on this?

@carpecodeum
Copy link
Contributor Author

@Anmolbansal1 thinking of making a script for creating admins. What say?

@carpecodeum carpecodeum force-pushed the admin-server-added branch 3 times, most recently from 67a3dd7 to 727daca Compare February 14, 2020 08:19
@carpecodeum carpecodeum requested review from Anmolbansal1 and removed request for Anmolbansal1 February 14, 2020 08:26
Copy link
Collaborator

@Anmolbansal1 Anmolbansal1 left a comment

Choose a reason for hiding this comment

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

Implementation is great 👍, just clarify these doubts and fix minor issues and it's good to merge.
Also, try implement a function to check whether a user is admin in users model itself and use it wherever necessary instead of writing it again for every route.

@@ -43,7 +43,7 @@ SQLAlchemy==1.2.18
tornado==6.0
tweepy==3.8.0
typed-ast==1.2.0
urllib3==1.24.1
urllib3==1.23.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this package version changed??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was changed as a result of a warning coming during the setup.

@@ -0,0 +1,66 @@
"""empty message
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that two migrations files are unnecessary as only a new column is added to user table, so try to generate only corresponding to that change

except Exception as e:
print(str(e))
response = {"message": "Something went wrong!!"}
return make_response(jsonify(response)), 500
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's unclear why this method return a response like a route, it's just a click command. Also please attach some screenshot demonstrating this change.

Comment on lines +31 to +32


Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove these unnecessary black lines

Comment on lines +20 to +25
user_id = get_jwt_identity()
user = model.User.find_by_user_id(user_id)

es_index = current_app.config["ES_INDEX"]
es = current_app.elasticsearch
if user.role == "admin":
es_index = current_app.config["ES_INDEX"]
es = current_app.elasticsearch
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file changes looks good, could you attach screenshots on request this url with admin user, you may use postman 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure!

return app
except Exception as err:
print("Error occured:", err)
return app
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why have you removed this error handling of flask app??

@carpecodeum carpecodeum requested a review from Anmolbansal1 March 8, 2020 09:59
@carpecodeum carpecodeum force-pushed the admin-server-added branch 2 times, most recently from f88d871 to 2c3b8a0 Compare March 9, 2020 12:08
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.

6 participants