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

Feature/add rich text reaction model #451

Merged
merged 38 commits into from
Jan 10, 2024

Conversation

wwrk22
Copy link
Contributor

@wwrk22 wwrk22 commented Dec 12, 2023

What's the change?

Added a model that stores emojis for reacting to standup meeting updates. The backend implementation was done on this branch.

What key workflows are impacted?

Users can create RichTextReaction records via the console to emulate reacting to standup meeting updates.

Highlights / Surprises / Risks / Cleanup

The frontend implementation that includes the UI to allow users to react is still required.

Demo / Screenshots

Create reactons via the Rails console.

# Assume current_user is the signed-in User, and rich_text_id is the id of an ActionText::RichText record.
rich_text_id = StandupMeeting.where(user: current_user).yesterday_work_description.id

# Refer to RichTextReaction.emojis for emojis that can be used in creating reactions.
current_user.rich_text_reactions.create(rich_text_id:, emoji_caption: ‘thumbs_up')

Issue ticket number and link

#212
This PR does not completely resolve the issue.

Checklist before requesting a review

Please delete items that are not relevant.

  • Did you add appropriate automated tests?
  • Did you consider risks around security, performance, etc.?
  • Have you thought of misfiring code? e.g. too many loops, n+1, or how to handle nils?
  • Any validations to data needed?
  • Related to readability and consistency: Did you add comments and/or documentation? (README, Notion, etc.)
  • Is everything elevated to the highest level? (e.g., can logic be moved out of view into controllers, or out of controllers into models?)
  • Can (and should) any models be extracted?
  • Is there any linting that needs to be executed?
  • Did you leave helpful inline PR comments for the reviewer(s)?
  • Did you include instructions for how to test in the ticket?
  • Did you add any relevant tags and decide if this PR is a Draft vs. Ready for Review?
  • Do you have a plan for how this change should be rolled back? (e.g., how do you deal with a migration rollback? Is your feature released behind a feature flag?)

Copy link

codecov bot commented Dec 12, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2fc41b0) 98.94% compared to head (008127f) 98.97%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #451      +/-   ##
==========================================
+ Coverage   98.94%   98.97%   +0.02%     
==========================================
  Files         209      216       +7     
  Lines        3231     3321      +90     
==========================================
+ Hits         3197     3287      +90     
  Misses         34       34              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wwrk22 wwrk22 force-pushed the feature/add-rich-text-reaction-model branch from 64e142d to 8b84d66 Compare December 13, 2023 20:31
@jp524
Copy link
Contributor

jp524 commented Dec 20, 2023

In the Demo / Screenshots section of the PR, last line should read current_user.rich_text_reactions.create(rich_text_id:, emoji_caption: :thumbs_up) since emoji was changed to emoji_caption in the schema.

app/controllers/rich_text_reactions_controller.rb Outdated Show resolved Hide resolved
app/models/rich_text_reaction.rb Outdated Show resolved Hide resolved
spec/requests/rich_text_reactions_spec.rb Outdated Show resolved Hide resolved
spec/requests/rich_text_reactions_spec.rb Outdated Show resolved Hide resolved
@wwrk22
Copy link
Contributor Author

wwrk22 commented Dec 23, 2023

In the Demo / Screenshots section of the PR, last line should read current_user.rich_text_reactions.create(rich_text_id:, emoji_caption: :thumbs_up) since emoji was changed to emoji_caption in the schema.

Fixed

@wwrk22 wwrk22 force-pushed the feature/add-rich-text-reaction-model branch 2 times, most recently from 0378337 to f7e443a Compare January 4, 2024 20:23
@wwrk22 wwrk22 force-pushed the feature/add-rich-text-reaction-model branch from aa26943 to 1388b6d Compare January 8, 2024 15:19
Copy link
Collaborator

@JoshDevHub JoshDevHub left a comment

Choose a reason for hiding this comment

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

Just one reminder of something to fix in here. The other stuff looks really good though.

spec/factories/rich_text_reactions.rb Outdated Show resolved Hide resolved
spec/lib/emoji_spec.rb Outdated Show resolved Hide resolved
# be any persisted ActiveRecord model record. By default, a User record is
# used.
transient do
rich_text_owner { user }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JoshDevHub
Some ActiveRecord model object has to be given for creating ActionText::RichText.
I decided the User model is probably the safest to use since it most likely won’t be going away or changed to a different name.

@@ -2,6 +2,8 @@

RSpec.describe 'FactoryBot' do
FactoryBot.factories.each do |factory|
next if factory.name == :action_text_rich_text
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JoshDevHub
I think unless we can somehow have a fake ActiveRecord model with a fake database table, it’s best to skip linting the ActionText::RichText factory.

Copy link
Collaborator

@JoshDevHub JoshDevHub left a comment

Choose a reason for hiding this comment

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

Alright cool. Looks like you have to fix some merge conflicts and it'll be good to go

@wwrk22
Copy link
Contributor Author

wwrk22 commented Jan 9, 2024

@JoshDevHub
Per the comment above:
Merge conflict has been resolved.

@JoshDevHub JoshDevHub merged commit 1544afe into main Jan 10, 2024
3 checks passed
@JoshDevHub JoshDevHub deleted the feature/add-rich-text-reaction-model branch January 10, 2024 04:52
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.

3 participants