ZA | 25-SDC-JULY | Luke Manyamazi | Sprint 1 | New Feature Rebloom #58
Conversation
Luke-Manyamazi
added
the
Needs Review
| # Returning 0 or raising an error are typical ways to indicate failure | ||
| return 0 |
There was a problem hiding this comment.
I don't think any other code in this codebases uses magic return values to indicate errors, and the code that calls this doesn't pass this error back to the client. You need to handle errors differently here.
| print(f"Error adding rebloom: User with ID {user_id} not found.") | ||
| return None |
There was a problem hiding this comment.
This doesn't bubble the error back up to the caller - the caller will need to know this failed.
This applies throughout your change.
| }) | ||
|
|
||
| @jwt_required() | ||
| def check_rebloom_status(bloom_id_str): |
There was a problem hiding this comment.
What are the trade-offs of having this be a stand-alone function that's called per bloom vs including this information up-front when getting a list of blooms?
| try: | ||
| # First create the new bloom | ||
| rebloom_bloom_id = add_bloom( | ||
| sender=sender_user, # <<< Use the fully hydrated User object |
There was a problem hiding this comment.
AFAICT add_bloom only actually uses the sender ID, an no other data from the sender - might it be simpler to make add_bloom take just the id, and change the other call-sites from sender to sender.id, rather than needing to add a whole extra database roundtrip to turn the sender ID into a sender, just to take only the ID back out of it?
| This is necessary for operations like reblooming where only the ID is known. | ||
| """ | ||
| with db_cursor() as cur: | ||
| # NOTE: Adjust column names if they are different in your 'users' table |
There was a problem hiding this comment.
What does this comment mean?
| rebloom_id = blooms.add_rebloom( | ||
| user_id=current_user.id, | ||
| original_bloom_id=bloom_id, | ||
| content=original_bloom.content # FIX: .content not ['content'] |
There was a problem hiding this comment.
What is this comment about?
| app.json = CustomJsonProvider(app) | ||
|
|
||
| # Configure CORS to handle preflight requests | ||
| # --- CORS FIX APPLIED HERE --- |
There was a problem hiding this comment.
Things seemed to be working ok without this change - why did you need to change it?
| ); | ||
|
|
||
| -- Add rebloom_count to blooms table (to track total reblooms) | ||
| ALTER TABLE blooms ADD COLUMN rebloom_count INTEGER DEFAULT 0; |
There was a problem hiding this comment.
Ideally you wouldn't need to track rebloom counts in the database - there is a race condition here where values can get out of date (e.g. if you create a rebloom but haven't updated this count yet).
Instead, use SQL relationships to compute this - in a query, you should be able to compute the count of blooms which have this ID as an original_bloom_id
Your Python and JS code for querying a Bloom shouldn't have to change, but it removes the need for you to update the original bloom from the python side.
| ALTER TABLE blooms ADD COLUMN original_bloom_id BIGINT REFERENCES blooms(id); | ||
|
|
||
| -- Create reblooms table to track the rebloom relationship | ||
| CREATE TABLE reblooms ( |
There was a problem hiding this comment.
This table shouldn't need to exist - all of the information should already be present in the blooms table for you to do some joins to infer this information.
|
|
||
| const rebloomIndicator = document.createElement('div'); | ||
| rebloomIndicator.className = 'rebloom-indicator'; | ||
| rebloomIndicator.innerHTML = ` |
There was a problem hiding this comment.
There's an XSS vulnerability here by using innerHTML
illicitonion
added
Reviewed
2 participants
Learners, PR Template
Self checklist
Changelist
Questions
Ask any questions you have for your reviewer.