Conversation
hugh-emerson
left a comment
hugh-emerson
left a comment
There was a problem hiding this comment.
Looks good, go ahead and merge when you're ready! 👍
See my comments for some suggestions. And if you like, you can still ask us for feedback on any further changes.
| @@ -1,2 +1,19 @@ | |||
| Flask==1.1.1 | |||
| python-dotenv==0.12.0 | |||
There was a problem hiding this comment.
Did you deliberately remove python-dotenv?
It's required for flask-run to automatically read the .env file.
| To get this app to work for you, you will need to have a Trello account and store the following variables in your .env file: | ||
| API_KEY - Your unique key for using the Trello API | ||
| API_TOKEN - Generate your own API token and store value here | ||
| BOARD_ID - The ID of the board you want to use for the project |
There was a problem hiding this comment.
Good job updating the README. You could also add these vars with empty values to the .env.template file, which generates .env the first time you run setup.
Also it's worth mentioning that your app requires the board to have lists with the correct names.
trello_api.py
Outdated
| return trelloCards | ||
|
|
||
|
|
||
| def trelloPost(title, description): |
There was a problem hiding this comment.
Try to stick to the snake_case naming convention rather than camelCase. It's just python thing to use snake_case, but you should always be consistent and currently you're mixing cases.
Also, this function name isn't the clearest. It's not a generic function for making a POST request to Trello, it's specifically creating a new card.
| # API_PARAMS was getting appended with additional parameters, so had to set variable in each API function | ||
| API_PARAMS = {'key': API_KEY, 'token': API_TOKEN} | ||
| board = os.getenv('BOARD_ID') | ||
| headers = {"Accept": "application/json"} |
There was a problem hiding this comment.
The Accept header means what format you want the response to be in.
Since you're only using this for a PUT request, did you mean to set "Content-Type" instead? That specifies the format of your request body.
But I think Trello accepts and returns JSON by default.
trello_api.py
Outdated
| # API_PARAMS was getting appended with additional parameters, so had to set variable in each API function | ||
| API_PARAMS = {'key': API_KEY, 'token': API_TOKEN} |
There was a problem hiding this comment.
When you write postParams = API_PARAMS, you have two variables that are actually just different names for the same dictionary. They are pointing to the exact same object.
What you wanted was params = API_PARAMS.copy(), then you can delete all the duplicated API_PARAMS = ... lines
app.py
Outdated
| return redirect('/') | ||
|
|
||
|
|
||
| # old code for using session_items instead of Trello API |
There was a problem hiding this comment.
You should delete old code. It will live forever in Git.
If you really want, you can keep a note of the commit hash or a tag/branch name to look it up easily.
session_items.py
Outdated
| @@ -1,12 +1,11 @@ | |||
| from flask import session | |||
trello_api.py
Outdated
| ) | ||
| trelloLists.append(list_data) | ||
| if list_data.status == "Not Started": | ||
| session['newItemId'] = list_data.trelloId |
There was a problem hiding this comment.
This works, but it's a bit odd to store these values on the session. Cookies are for user specific data but this is constant for the instance of the app, as it's tied to the board that's in use.
It would make a bit more sense to either set some constants when the app starts up or you could even get the list ID each time you need it.
app.py
Outdated
| new_status = request.form.get(id) | ||
| if new_status == 'Delete': | ||
| session.delete_item(id) | ||
| for trelloId in request.form: |
There was a problem hiding this comment.
Obviously this will slow down if there are many cards on the board as you're making a separate HTTP request for every single card whether or not its status needs to change.
You don't need to change this now, but just making sure you're aware.
2 participants
Please review.
If I get time this week I will try and add due dates, which is the only outstanding stretch goal (I think).
Also need to style the new description input form, as not great on UI, but it works.