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

Standup comments 1 #446

Open
wants to merge 41 commits into
base: main
Choose a base branch
from
Open

Standup comments 1 #446

wants to merge 41 commits into from

Conversation

GALTdea
Copy link
Collaborator

@GALTdea GALTdea commented Nov 29, 2023

What's the change?

  • Adds comment feature to Standup Meetings that allows users to leave comments in other user's standup_metings
  • Adds StandupMeetingComments model to keep track of comments made to StandupMeetings
  • Adds sections controller and index to dynamically render standup_meeting_comments belonging to any given standup_comment section i.e: "yesterday_work_description," "today_work_Description," "blocker_description," etc.

What key workflows are impacted?

Allows users to interact with other users by leaving comments on their standup meeting updates

Highlights / Surprises / Risks / Cleanup

To be fully usable, I believe I should use Turbo for comments, especially to enable users to edit their comments. but this should be done in a different PR. I'll open one with those changes as soon as this one gets accepted.

Demo / Screenshots

Screenshot 2023-12-02 at 3 43 17 PM
Screen.Recording.2023-12-02.at.3.57.23.PM.mov

Issue ticket number and link

#418

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 4, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (06c9b49) 99.26% compared to head (12c8a34) 99.28%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #446      +/-   ##
==========================================
+ Coverage   99.26%   99.28%   +0.02%     
==========================================
  Files         219      231      +12     
  Lines        3393     3505     +112     
==========================================
+ Hits         3368     3480     +112     
  Misses         25       25              

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

@GALTdea GALTdea marked this pull request as ready for review December 5, 2023 18:43
@GALTdea GALTdea requested review from toyhammered, afogel and JoshDevHub and removed request for toyhammered and afogel December 5, 2023 18:44
app/views/standup_meetings/sections/index.html.erb Outdated Show resolved Hide resolved
@@ -0,0 +1,7 @@
class StandupMeetings::SectionsController < ApplicationController
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This controller (along with its view) creates a dynamic view corresponding to the appropriate question/section in the standup meeting groups. e.g:
when params:
http://localhost:3000/standup_meetings/597/sections?section=yesterday_work_description

Screenshot 2024-01-09 at 10 59 42 AM

app/models/standup_meeting.rb Outdated Show resolved Hide resolved
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.

This looks and feels great to use. Huge props @GALTdea 🙌

Just have a few comments / requested changes. Let me know if you have any questions.

Also be sure to merge main and fix the conflicts.


def allowed_section?(secion_name)
ALLOWED_SECTIONS.include?(secion_name)
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome 🙌

@JoshDevHub
Copy link
Collaborator

Let me know when you want me to review again @GALTdea

@GALTdea
Copy link
Collaborator Author

GALTdea commented Jan 19, 2024

Let me know when you want me to review again @GALTdea

Thank you, @JoshDevHub! I need to fix a couple of failing tests from my latest commits, I'll let you know when I do!

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.

2 participants