add index page for standup meetings #145
Conversation
| standup_meeting_group_id: params[:standup_meeting_group_id] | ||
| ) | ||
| @date_options = StandupMeeting.meeting_dates(params[:standup_meeting_group_id]) | ||
| if @standup_meetings.empty? |
There was a problem hiding this comment.
gracefully handle the event in which a particular date requested by a user does not have any StandupMeetings that fall on that date.
| @@ -1,7 +1,23 @@ | |||
| class StandupMeetingsController < ApplicationController | |||
| def index | |||
| @meeting_date = params[:date].nil? ? Date.current : Date.parse(params[:date]) | |||
| @standup_meetings = StandupMeeting.includes(:user, :standup_meeting_group) | |||
There was a problem hiding this comment.
I think this should be changed to something like this:
@standup_meeting_group = StandupMeetingGroup.find(standup_meeting_group_id)
@standup_meetings = @standup_meeting_group.standup_meetings.includes(:user).where(meeting_date: @meeting_date)
The main reason is we should still go to that day even if there are no standup meetings for it. It should be treated the exact same and on the view side it will just be empty in that bottom section.
There was a problem hiding this comment.
This makes a ton more sense to me -- kind of incredible that such a small change simplifies things so dramatically.
| @@ -1,7 +1,23 @@ | |||
| class StandupMeetingsController < ApplicationController | |||
| def index | |||
| @meeting_date = params[:date].nil? ? Date.current : Date.parse(params[:date]) | |||
There was a problem hiding this comment.
I am debating on whether this should be a required param and the routes pass it in (in like 95% of the cases they will).
There was a problem hiding this comment.
I think it probably makes sense to keep, just in case?
| meeting_date: @meeting_date, | ||
| standup_meeting_group_id: params[:standup_meeting_group_id] | ||
| ) | ||
| @date_options = StandupMeeting.meeting_dates(params[:standup_meeting_group_id]) |
There was a problem hiding this comment.
Related to the above, I don't think we even need this if this was only used in the redirect due to there being no meetings on that day.
There was a problem hiding this comment.
This is used later for moving between dates in #146 as well. I introduced it here because it is also useful for the redirect guard.
There was a problem hiding this comment.
I don't think we need it there either. The logic should just be +1 or -1 to the current date. Let weekend pages show with no standup meetings.
| def index? | ||
| user.admin? || matching_user? | ||
| group_member? || user.admin? |
There was a problem hiding this comment.
fine with this for now but this is a strange one cause of the mixing/matching of what the "record" actually is. Not sure of a good solution to make this clearer.
| user.standup_meetings | ||
| end | ||
| def group_member? | ||
| record.users.where(id: user.id).present? |
There was a problem hiding this comment.
should just do find_by because we are looking for a single record. Technically doesn't make much of a difference just more clear.
There was a problem hiding this comment.
Since it's a boolean check for one record, I really like to reach for #exists? Rails guide for it
But then you can express this as record.users.exists?(user.id)
Creates general #index page and shell for other features to be added to the StandupMeeting#index page.