-
-
Notifications
You must be signed in to change notification settings - Fork 21
London | 25-SDC-July | Mikiyas Gebremichael | Sprint 1 | New Feature - Rebloom #62
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
base: main
Are you sure you want to change the base?
Conversation
illicitonion
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This generally looks good. A couple of UI considerations:
- Right now if I go to someone's profile page and press "Share", it reblooms, but the existing bloom doesn't update. What do you think of this UX?
- What sort order do you think reblooms should have? Should they sort at the original bloom time? Or the rebloom time? Or something different?
| return None | ||
| content = original_bloom.content | ||
| update_rebloom_counter(id) | ||
| add_bloom(sender=sender, content=content, original_bloom_id=id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense setting the content for a rebloom?
| content TEXT NOT NULL, | ||
| send_timestamp TIMESTAMP NOT NULL | ||
| send_timestamp TIMESTAMP NOT NULL, | ||
| reblooms INT NOT NULL DEFAULT 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| //I used inner html to render the arrow ↪ sign | ||
| rebloomInfoEl.innerHTML = `↪ Rebloom of the ${originalBloom.sender}'s post, posted ${timeStamp} ago`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an XSS vulnerability - can you work out how to avoid that?
Learners, PR Template
Self checklist
Changelist
This PR is made for the rebloom implementation
Questions
No question