weixin_39643338
weixin_39643338
2021-01-02 05:36

Schedule versioning

Organizers can create multiple proposals for schedules in the backend, only 1 schedule can be selected and that's the conference schedule.

I have created the new associations and models needed for the schedule versioning, illustrated in the following image:

nuevodocumento 5_1

The Schedule and EventSchedule models are new. EventSchedule is where the time and room is saved. So, for every Schedule and Event we can have one EventScheduleallowing that an Event is scheduled in different rooms and times for every schedule.

Also, the selected_schedule in program indicates what of the schedules that belongs to that program is the public one.

该提问来源于开源项目:openSUSE/osem

  • 点赞
  • 写回答
  • 关注问题
  • 收藏
  • 复制链接分享
  • 邀请回答

22条回答

  • weixin_39643338 weixin_39643338 4月前

    I've pluralized event schedule controller name and moved the max_attendees_no_more_than_room_size validation. I've also open an issue to pluralize conference and proposal controllers https://github.com/openSUSE/osem/issues/1120

    Is there anything else to change?

    点赞 评论 复制链接分享
  • weixin_39655049 weixin_39655049 4月前

    Because of the

    Selected schedule = check_box_tag .short_title, .id, (.id == .try(:id)), method: :put, url: (admin_conference_schedule_path(.short_title, ) + '?selected_schedule='), class: 'switch-checkbox', data: { size: 'small', off_color: 'warning', on_text: 'Yes', off_text: 'No' }

    I get a message from my browser every time I reload the page...

    点赞 评论 复制链接分享
  • weixin_39643338 weixin_39643338 4月前

    :

    Why not two different functions for #create and #update ?

    Because they will be identical except by the type and the url in the params, but I will still need the if(event_schedule_id != null) condition to know if I have to call create or update. I think it will complicate the code. Every time we want to change something in the way we schedule events (for example to add a save button) it will be necessary to change both functions.

    点赞 评论 复制链接分享
  • weixin_39643338 weixin_39643338 4月前

    Because of the

    Selected schedule = check_box_tag .short_title, .id, (.id == .try(:id)), method: :put, url: (admin_conference_schedule_path(.short_title, ) + '?selected_schedule='), class: 'switch-checkbox', data: { size: 'small', off_color: 'warning', on_text: 'Yes', off_text: 'No' }

    I get a message from my browser every time I reload the page...

    What message? I think I don't get any message :confused:

    点赞 评论 复制链接分享
  • weixin_39559523 weixin_39559523 4月前

    could you please add a comment with the overall workflow you are implementing? Like a few bullet points of what you are doing, what is no longer there, what is new.

    点赞 评论 复制链接分享
  • weixin_39643338 weixin_39643338 4月前

    Organizers can create multiple proposals for schedules in the backend, only 1 schedule can be selected and that's the conference schedule - Integrate the schedule in the backend - Allow having several schedules in the backend: - Add the relations and models illustrated in the image of the first comment - Add an index with all schedules were more schedules can be created - Add a checkbox inside show to be able to publish a schedule - Adapt the public schedule to work with the new relations and models - Simplify the javascript needed for the drag and drop (events are not loaded with javascript anymore) and the javascript code has been moved - To schedule an event it is used event schedules controller instead of schedule controller - I changed the routes - Change the ability file and the tests to work with the previous changes

    And I think that's all. :smiley:

    点赞 评论 复制链接分享
  • weixin_39643338 weixin_39643338 4月前

    I made the last corrections you added.

    I also moved the max_attendees_no_more_than_room_size to event again, as suggested. I am not sure that we don't need to validate it in event_schedule too, but as says:

    the workflow of max_attendees is a subject for the proposal registration process to solve, not this PR.

    点赞 评论 复制链接分享
  • weixin_39643338 weixin_39643338 4月前

    I made the last correction you pointed out and created event_schedules_controller_spec.rb and schedules_controller_spec.rb :smiley:

    点赞 评论 复制链接分享
  • weixin_39643338 weixin_39643338 4月前

    Why do you create a new one and don't use the factories you already defined?

    attributes_for(:event_schedule) is {} as I don't give any default value to event_schedule attributes in factory because most of them are relations so I think it is not possible to do it.

    点赞 评论 复制链接分享
  • weixin_39643338 weixin_39643338 4月前

    Move the delete to a before filter! Because you don't have to call it twice!

    In most controller it is called twice, for example: https://github.com/openSUSE/osem/blob/master/spec/controllers/admin/conferences_controller_spec.rb#L81

    Do I create an issue to change that?

    点赞 评论 复制链接分享
  • weixin_39655049 weixin_39655049 4月前

    Except some nitpicky test namings which you should improve, it LGTM! Really good work :clap: :clap: :clap:

    do you also want to have a look at it?

    点赞 评论 复制链接分享
  • weixin_39643338 weixin_39643338 4月前

    Is there anything else to change?

    点赞 评论 复制链接分享
  • weixin_39655049 weixin_39655049 4月前

    From my side it looks good and I assume you already fixed suggestions. So let's :ship: it!

    Great work :clap: :clap: :clap:

    点赞 评论 复制链接分享
  • weixin_39643338 weixin_39643338 4月前

    I've made the corrections you sent me by email.

    I am not pretty sure how we want the schedules index to look like:

    image

    Also, the authorization for the schedule used to be like that and I am not sure why:

    authorize! :update, .events.new

    I have changed it to only allow to manage the schedule and event_schedules to users with organizer role. Is that okey?

    点赞 评论 复制链接分享
  • weixin_39643338 weixin_39643338 4月前

    :

    Don't do that! Use instead the Rails url helper eg in a data attribute and get the whole url. This way you also don't need to initialize the schedule and conference ids.

    I would need the schedule_id even if I had the url as the url is using event_schedule_id, which is needed to create new event_schedules.

    Also all unscheduled events use the same url (to create a new schedule), so I think it is not worthwhile adding it in a data attribute in every event. The schedule events use the same url + the event_schedule_id for both editing and deleting. I use the schedule_id attribute to know if an event is already scheduled or not to know if I should edit or create the event_schedule, if both has an url attribute it would be more difficult to do that. This is here: https://github.com/openSUSE/osem/pull/1094/files#diff-96573ccecfe6b86eaba8cd21511262bbR35

    I have initialize the Schedule with the url instead of the conference id, so I am using the helper but it is not exactly what you suggested. Is it okey like that?

    点赞 评论 复制链接分享
  • weixin_39643338 weixin_39643338 4月前

    :

    Why did you rename app/controllers/admin/schedules_controller.rb to app/controllers/admin/schedule_controller.rb http://alexander-clark.com/blog/rails-conventions-singular-or-plural/

    If controller should be plural, why conference controller is singular? :confused:

    I suppose I should also pluralize event schedule controller.

    点赞 评论 复制链接分享
  • weixin_39643338 weixin_39643338 4月前

    I've just realized that max_attendees_no_more_than_room_size should be moved to EventSchedule, shouldn't it?

    Otherwise we are only checking that for the event_schedules in the selected schedule.

    I think I should render the room size in the admin schedule (it used to be rendered but I eliminated it when integrating the view). Also, it should be allowed to drop events in a room whose size is not enough for the number of max attendees. Do I add that in this PR or in a different one?

    点赞 评论 复制链接分享
  • weixin_39655049 weixin_39655049 4月前

    If controller should be plural, why conference controller is singular? :confused:

    It should also be plural! Even for singular routes you should name the controller in plural.

    Because you might want to use the same controller for a singular route (/account) and a plural route (/accounts/45), singular resources map to plural controllers. So that, for example, resource :photo and resources :photos creates both singular and plural routes that map to the same controller (PhotosController).

    from http://edgeguides.rubyonrails.org/routing.html#singular-resources

    I suppose I should also pluralize event schedule controller.

    Yes, it should be event_schedules_controller!

    点赞 评论 复制链接分享
  • weixin_39655049 weixin_39655049 4月前

    I've just realized that max_attendees_no_more_than_room_size should be moved to EventSchedule, shouldn't it?

    Otherwise we are only checking that for the event_schedules in the selected schedule.

    Makes sense

    I think I should render the room size in the admin schedule (it used to be rendered but I eliminated it when integrating the view). Also, it should be allowed to drop events in a room whose size is not enough for the number of max attendees. Do I add that in this PR or in a different one?

    Let's finish this one first, it's already quite big and hard to review!

    点赞 评论 复制链接分享
  • weixin_39643338 weixin_39643338 4月前

    Why the commits in the PR are not in the same order than in my computer? :confounded:

    image

    点赞 评论 复制链接分享
  • weixin_39643338 weixin_39643338 4月前

    I think that everything is working now and that the test are fixed. :wink:

    点赞 评论 复制链接分享
  • weixin_39643338 weixin_39643338 4月前

    I've added the integration of the schedule together with the necessary changes to make use of the introduced associations and models (in a separate commit to make reviewing it easier):

    image

    image

    I will continue the rest of the schedule integration (adding a save button, having several schedules, etc) in another branch (maybe one branch for each part) using this branch as a starting point . But we need that part here, otherwise the admin schedule wouldn't work until I finished the whole integration. That way we can merge that before.

    I still have to adapt the public schedule to make use of the new associations and models and to fix some test after that too. I was waiting to merge #1032 but I think I am going to do it now to avoid having to wait, and I'll solve merge conflicts afterwards.

    点赞 评论 复制链接分享

相关推荐