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

Event scheduling for tracks #1631

Merged
merged 5 commits into from Aug 25, 2017

Conversation

2 participants
@AEtherC0r3
Copy link
Contributor

AEtherC0r3 commented Aug 9, 2017

  • Don't allow tracks with overlapping dates to use the same room
  • Add accepted and self_organized scopes to Track
  • Don't allow an event to be scheduled outside of it's track's room and dates
  • Remove unnecessary abilities from AdminAbility, in order to allow CanCanCan to load @Events and @event in EventsController
  • Allow the track organizers to manage the events and the events' commercials of their tracks
  • Create associate between Schedule and Track
  • Allow the track organizers to create schedules for their tracks and schedule the events of their tracks
  • Restrict events of self-organized tracks to be scheduled only in their tracks schedules
  • Show scheduled events of self-organized tracks and the rooms those tracks occupy in the conference scheduled as semitransparent
  • Add admin/SchedulesController#new action and related form and buttons
  • Add buttons to easily access admin//Schedules#create and #show from Tracks#index and #show
  • Show a unified schedule in Schedules#show
  • Create tabs for conference and track schedules in admin/Schedules#index
  • Add selected_schedule_id to Track
  • Relax requirement for cfp_active to be enabled in Event and check it in ProposalsController
  • Validate that events of self-organized tracks are scheduled only in their track's schedules

@AEtherC0r3 AEtherC0r3 force-pushed the AEtherC0r3:track_scheduling branch 2 times, most recently from 243eba6 to 08df3cb Aug 11, 2017

@AEtherC0r3 AEtherC0r3 added the GSoC label Aug 11, 2017

@@ -136,7 +136,7 @@ def redirect_back_or_to(options = {}, response_status = {})

def concurrent_events(event)
return nil unless event.scheduled? && event.program.selected_event_schedules
event_schedule = event.program.selected_event_schedules.find_by(event: event)
event_schedule = event.program.selected_event_schedules.find { |es| es.event == event }

This comment has been minimized.

Copy link
@differentreality

differentreality Aug 14, 2017

Contributor

Why are you changing this?

This comment has been minimized.

Copy link
@AEtherC0r3

AEtherC0r3 Aug 14, 2017

Author Contributor

Because of the changes I've made to Program#selected_event_schedules, it no longer returns an ActiveRecord::Relation but an Array, and arrays don't have find_by, they just have find

This comment has been minimized.

Copy link
@differentreality

This comment has been minimized.

Copy link
@AEtherC0r3

AEtherC0r3 Aug 20, 2017

Author Contributor

Yup, sorry I missed it

end

##
# Validates that the event is scheduled within it's track's time slot

This comment has been minimized.

Copy link
@differentreality

differentreality Aug 14, 2017

Contributor

it's -> it is
its -> it belongs to it

This comment has been minimized.

Copy link
@AEtherC0r3

AEtherC0r3 Aug 14, 2017

Author Contributor

Yeah, I know. My hand just adds the apostrophe by itself, when I want to express possession and I'm not really paying attention to what I'm writing :P

@@ -52,4 +54,26 @@ def start_before_start_hour
def conference_id
schedule.program.conference_id
end

##
# Validates that the event is scheduled in the same room as it's track

This comment has been minimized.

Copy link
@differentreality
@@ -7,12 +7,13 @@
/ subtracting the padding before calculate the number of lines
- lines = (height - 7) / 23
- color = event.track.try(:color).present? ? event.track.try(:color) : 'FFFFFF'
- non_schedulable = event_schedule_id && EventSchedule.find(event_schedule_id).schedule != @schedule

This comment has been minimized.

Copy link
@differentreality

differentreality Aug 14, 2017

Contributor

I'd add a parenthesis here for better readability

@AEtherC0r3 AEtherC0r3 removed the in progress label Aug 14, 2017

@AEtherC0r3 AEtherC0r3 changed the title [WIP] Event scheduling for tracks Event scheduling for tracks Aug 14, 2017

@AEtherC0r3 AEtherC0r3 force-pushed the AEtherC0r3:track_scheduling branch from 42d350b to 9a2c707 Aug 14, 2017

@AEtherC0r3 AEtherC0r3 force-pushed the AEtherC0r3:track_scheduling branch from 9a2c707 to 607847c Aug 14, 2017

##
# Validates that the event is scheduled in the same room as its track
#
def room_of_track

This comment has been minimized.

Copy link
@differentreality

differentreality Aug 18, 2017

Contributor

I would rename this to something more self explanatory based on what it is doing.

@@ -99,35 +99,28 @@

describe '#acceptable_track' do
context 'is valid' do
it 'when the track belong to the same program, is confirmed and is included in the cfp' do
track = create(:track, state: 'confirmed', cfp_active: true, program: conference.program)
it 'when the track belong to the same program and is confirmed' do

This comment has been minimized.

Copy link
@differentreality

differentreality Aug 18, 2017

Contributor

belongs

@@ -99,35 +99,28 @@

describe '#acceptable_track' do

This comment has been minimized.

Copy link
@differentreality

differentreality Aug 18, 2017

Contributor

This makes me think 'a track that can be accepted'.
Shall we rename this to selectable_track or valid_track or ...?

end

context 'is valid' do
it 'when the tracks are in different rooms' do

This comment has been minimized.

Copy link
@differentreality

differentreality Aug 18, 2017

Contributor

Perhaps it makes sense to add in different rooms at the same time

expect(track.valid?).to eq true
end

it 'when it ends before the other tracks' do

This comment has been minimized.

Copy link
@differentreality

differentreality Aug 18, 2017

Contributor

in the same room

end

context 'is invalid' do
it 'when it starts or ends with another track in the same room' do

This comment has been minimized.

Copy link
@differentreality

differentreality Aug 18, 2017

Contributor

The test is not doing what its descriptions says.

@@ -246,7 +354,8 @@
self_organized_track.cfp_active = true
self_organized_track.save!
@a_track_organizer.add_role 'track_organizer', self_organized_track
@an_event_of_the_track = create(:event, program: self_organized_track.program, track: self_organized_track)
@an_event_of_the_track = create(:event, program: self_organized_track.program, track: self_organized_track, state: 'confirmed')

This comment has been minimized.

Copy link
@differentreality

differentreality Aug 18, 2017

Contributor

How about we change this variable name to something shorter/simpler and self-explanatory?
This is an event that has as track the self_organized_track, so some ideas:

event_with_self_organized_track
self_organized_track_event
event_of_self_organized_track
@@ -246,7 +354,8 @@
self_organized_track.cfp_active = true
self_organized_track.save!
@a_track_organizer.add_role 'track_organizer', self_organized_track
@an_event_of_the_track = create(:event, program: self_organized_track.program, track: self_organized_track)
@an_event_of_the_track = create(:event, program: self_organized_track.program, track: self_organized_track, state: 'confirmed')
@schedule_of_the_track = create(:schedule, program: self_organized_track.program, track: self_organized_track)

This comment has been minimized.

Copy link
@differentreality

differentreality Aug 18, 2017

Contributor

I would rename this as well.
schedule_of_self_organized_track
or
self_organized_track_schedule

@differentreality

This comment has been minimized.

Copy link
Contributor

differentreality commented Aug 19, 2017

If my track is accepted, and confirmed, perhaps it makes sense that I am able to check in tracks#index or tracks#show and be directed in seeing the submitted events for my tracks, and scheduling my events.

@AEtherC0r3

This comment has been minimized.

Copy link
Contributor Author

AEtherC0r3 commented Aug 20, 2017

I've added buttons that send you to admin/Tracks#show

@AEtherC0r3

This comment has been minimized.

Copy link
Contributor Author

AEtherC0r3 commented Aug 20, 2017

Buttons in Tracks#index and Tracks#show that redirect to admin/Tracks#show
2017-08-21 00-06-56
2017-08-21 00-07-20

Track organizer being able to access admin/Events#index and manage his/her tracks's events
2017-08-21 00-21-03

admin/Schedules#show not showing the events of self-organized tracks
2017-08-21 00-17-15

admin/Schedules#show showing the rooms as semitransparent in conference schedules
2017-08-21 00-33-10

Showing events from selected track schedules as semitransparent in conference schedules
2017-08-21 00-40-22

Track organizer being able to create and manage schedules of his/her tracks from admin/Schedules#index, admin/Tracks#index and #show
2017-08-21 00-23-29
2017-08-21 00-24-07
2017-08-21 00-29-12

Track organizer being able to schedule events of his/her tracks
2017-08-21 00-40-39

@@ -49,6 +49,13 @@ def create
# by default.
@event.speakers = [current_user]
@event.submitter = current_user

if Track.find_by(id: params[:track_id]).try(:cfp_active) == false

This comment has been minimized.

Copy link
@differentreality

differentreality Aug 21, 2017

Contributor

You could load @track with cancancan and do unless @track.try(:cfp_active)

This comment has been minimized.

Copy link
@AEtherC0r3

AEtherC0r3 Aug 25, 2017

Author Contributor

We want to load the track that corresponds to the track_id in the parameters hash. I don't think that cancancan can load it, because there is no track_id in the url for it to use and we don't want it to load the track that might be associated with the event

This comment has been minimized.

Copy link
@differentreality

differentreality Aug 25, 2017

Contributor

That's true our params have [:event][:track_id], but then what you are writing in this line is wrong. Did you try if this is working on your browser?

@AEtherC0r3 AEtherC0r3 force-pushed the AEtherC0r3:track_scheduling branch from 5369b44 to 58f06c8 Aug 23, 2017

# Validates that the event is scheduled in the same room as its track
#
def same_room_as_track
if event && event.track.try(:room) && event.track.room != room

This comment has been minimized.

Copy link
@differentreality

differentreality Aug 23, 2017

Contributor

You could also try this approach to make it easier to understand what your code is doing. This way when reading the line that adds the error you are focusing on the actual validation that matters (in this case the room being the same as the track room)

return false unless event && event.track && event.track.room
errors.add() unless event.track.room == room
value = @schedule == @schedule.track.selected_schedule
url = update_selected_schedule_admin_conference_program_track_path(@conference.short_title, @schedule.track) + '?[track][selected_schedule_id]='
else
value = @schedule.id == @selected_schedule.try(:id)

This comment has been minimized.

Copy link
@differentreality

differentreality Aug 23, 2017

Contributor

Why do we need the id here?

This comment has been minimized.

Copy link
@AEtherC0r3

AEtherC0r3 Aug 24, 2017

Author Contributor

I don't know, I don't think that we need it. It's there because I just copy-pasted the expression from the existing code. In the expression I wrote (#1631 (diff)) I don't use the id, but compare the two objects directly.
@Ana06 can you help us with this?

@@ -72,6 +72,8 @@
method: :patch, class: 'btn btn-mini btn-success', id: "resubmit_track_request_#{track.id}"
- if can? :edit, track
= link_to 'Edit', edit_conference_program_track_path(@conference.short_title, track), class: 'btn btn-default'
- if current_user.has_role? :track_organizer, track
= link_to 'Administer', admin_conference_program_track_path(@conference.short_title, track), class: 'btn btn-default'

This comment has been minimized.

Copy link
@differentreality

differentreality Aug 23, 2017

Contributor

Maybe 'Manage' makes more sense in this context?

@differentreality

This comment has been minimized.

Copy link
Contributor

differentreality commented Aug 23, 2017

Very good work @AEtherC0r3 🎉 glad you managed to finish scheduling so early! This looks very good 👍
Please rebase & squash!

@AEtherC0r3 AEtherC0r3 force-pushed the AEtherC0r3:track_scheduling branch from 79cb7db to a218311 Aug 24, 2017

AEtherC0r3 added some commits Aug 8, 2017

Validate that tracks don't overlap
Also, add scope for accepted tracks
Validate room and start_time for EventSchedule
Don't allow an event to be scheduled outside of it's track's room and
time slot
Remove unnecessary ability from adminAbilities
CanCanCan can load @Events and @event in EventsController by itself
Allow the track organizer to manage events
He/She can manage the events of his/her tracks and their commercials

@AEtherC0r3 AEtherC0r3 force-pushed the AEtherC0r3:track_scheduling branch from a218311 to 0abddf0 Aug 24, 2017

Implement track scheduling
Add track association to schedule
Show schedules in admin sidebar to track organizers
Allow track organizers to manage the schedules of their tracks
Don't allow self-organized track events to be dragged or unscheduled in
a conference schedule
Make scheduled events of self-organized tracks appear semitransparent in
conference schedules
Make the rooms of confirmed self_organized tracks appear semitransparent
and don't allow events to be scheduled to it in the conference schedules
during the dates of its track
Create admin/SchedulesController#new action
Add a button in admin/Schedules#index to create schedules for tracks
Add self_organized scope to Track
Modify Schedules#show to handle track schedules and show a unified
schedule
Allow track organizers to create new schedules for their tracks
Correctly identify scheduled and unscheduled events in Schedules#events
Fix Event#room and Event#time for when the event is scheduled in a track
schedule
Modify Program#selected_event_schedules to include the event_schedules
of selected track schedules
Modify Track#revoke_role_and_cleanup to destroy the track's schedules
and revert its events' state to new
Add tabs for conference and track schedules in admin/Schedules#index
Add button to Create/Show a tracks schedule in Tracks#index and #show
Fix concurrent_events in application_helper because of changes in
Program#selected_event_schedules
Do not take into account cfp_active in Event#valid_track
Modify EventsController#get_tracks accordingly
Enforce cfp_active of track to be enabled for proposals in
ProposalsController#create and #update
Add support for multiple schedules per track
Add selected_schedule_id to Track
Load EventSchedules of selected track schedules for conference schedules
in admin/SchedulesController#show
Modify SchedulesController#show to take into account only the selected
track schedules
Create Event#selected_schedule_id and use it in Event#scheduled? and
Event#time
Validate that an EventSchedule for an event of a self-organized track
belongs to one of the track's schedules
Add 'Manage' button in Tracks#index, #show that sends you to the admin
side of things
Add admin/TracksController#update_selected_schedule to update the
selected_schedule_id of tracks

@AEtherC0r3 AEtherC0r3 force-pushed the AEtherC0r3:track_scheduling branch from 0abddf0 to f371c27 Aug 25, 2017


# Show Events in the admin sidebar
can :update, Event do |event|
event.new_record? && conf_ids_for_track_organizer.include?(event.program.conference_id)

This comment has been minimized.

Copy link
@differentreality

differentreality Aug 25, 2017

Contributor

We should look into rewriting this. Can update a new record does not sound very elegant.

end
else
respond_to do |format|
format.js { render json: { errors: "The selected schedule couldn't been updated #{@track.errors.to_a.join('. ')}" }, status: 422 }

This comment has been minimized.

Copy link
@differentreality

differentreality Aug 25, 2017

Contributor

couldn't be updated

@@ -49,6 +49,13 @@ def create
# by default.
@event.speakers = [current_user]
@event.submitter = current_user

if Track.find_by(id: params[:event][:track_id]).try(:cfp_active) == false

This comment has been minimized.

Copy link
@differentreality

differentreality Aug 25, 2017

Contributor

Let's remove == false

@@ -61,6 +68,12 @@ def create
def update
@url = conference_program_proposal_path(@conference.short_title, params[:id])

if Track.find_by(id: params[:event][:track_id]).try(:cfp_active) == false

This comment has been minimized.

Copy link
@differentreality

differentreality Aug 25, 2017

Contributor

Let's remove == false

# Check that there is no other track in the same room with overlapping dates
def overlapping
return unless start_date && end_date && room && program.try(:tracks)
(program.tracks.accepted + program.tracks.confirmed - [self]).each do |other_track|

This comment has been minimized.

Copy link
@differentreality

differentreality Aug 25, 2017

Contributor

I would name the variable existing_track to make it more understandable

(program.tracks.accepted + program.tracks.confirmed - [self]).each do |other_track|
if other_track.room == room &&
other_track.start_date && other_track.end_date &&
(other_track.start_date <= start_date && other_track.end_date >= start_date ||

This comment has been minimized.

Copy link
@differentreality

differentreality Aug 25, 2017

Contributor

I'd keep the same order of variables for all checks, and preferably I'd put the new track dates in the beginning, so that we can can read like this 'the start date (of the new track) is more or equal than the existing track'

@differentreality differentreality merged commit 7eea930 into openSUSE:master Aug 25, 2017

2 of 3 checks passed

Hakiri Security warnings were found.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.2%) to 83.83%
Details
@differentreality

This comment has been minimized.

Copy link
Contributor

differentreality commented Aug 25, 2017

:tada Nicely done 👍 Let's change the comments in the refactoring PR!

@AEtherC0r3 AEtherC0r3 deleted the AEtherC0r3:track_scheduling branch Aug 25, 2017

@AEtherC0r3

This comment has been minimized.

Copy link
Contributor Author

AEtherC0r3 commented Aug 27, 2017

The changes have been made in #1639

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.