ZA | 25-SDC-JULY | Luke Manyamazi | Sprint 1 | New Feature Rebloom - #257
ZA | 25-SDC-JULY | Luke Manyamazi | Sprint 1 | New Feature Rebloom#257Luke-Manyamazi wants to merge 1 commit into
Conversation
Luke-Manyamazi
added
📅 Sprint 1
illicitonion
left a comment
illicitonion
left a comment
There was a problem hiding this comment.
This generally looks pretty good, but when I run it locally and open up http://localhost:8000 the page is mostly empty and there's an error in the console. Can you take a look at that?
| original_bloom_id: Optional[int] = None | ||
| original_sender: Optional[str] = None | ||
| original_sent_timestamp: Optional[datetime.datetime] = None |
There was a problem hiding this comment.
Rather than having three Optional properties here which are always either set or not, I'd recommend extracting a RebloomDetails class and having a single Optional[RebloomDetails] property.
That way you don't need to worry in code about the (hopefully impossible!) possibility that original_bloom_id is set where original_sender isn't - we make clear through the types the properties are grouped and are all present or all absent.
| ), | ||
| EXISTS( | ||
| SELECT 1 FROM blooms rb2 | ||
| WHERE rb2.sender_id = %(current_user_id)s |
There was a problem hiding this comment.
With this clause, will users who aren't logged in still be able to see blooms?
| # Force JSON error bodies everywhere - Flask's default HTML error pages | ||
| # break frontend code that expects response.json() to succeed. | ||
| @app.errorhandler(HTTPException) | ||
| def handle_http_exception(error): | ||
| return jsonify({"success": False, "message": error.description}), error.code | ||
|
|
||
| @app.errorhandler(Exception) | ||
| def handle_unexpected_exception(error): | ||
| app.logger.exception(error) | ||
| return jsonify({"success": False, "message": "Internal server error"}), 500 | ||
|
|
There was a problem hiding this comment.
This is a nice improvement!
In real life I'd expect this as a separate PR, but I can believe this helped you to debug things a lot on the way so I'm not going to complain about having it in this one :)
| content TEXT NOT NULL, | ||
| send_timestamp TIMESTAMP NOT NULL | ||
| send_timestamp TIMESTAMP NOT NULL, | ||
| original_bloom_id BIGINT REFERENCES blooms(id) |
There was a problem hiding this comment.
This is good data modelling, well done!
illicitonion
added
Reviewed
2 participants
Learners, PR Template
Self checklist
Changelist
Added code for the new feature rebloom
Briefly explain your PR.
This PR is for the new feature rebloom exercise. I hope all this time I have met all the requirements to make this PR pass.
Questions
I do not have any questions.
Ask any questions you have for your reviewer. You must remove this section if you have no questions.