Ft create recurring events 129620817 #192
Conversation
…vents - starts story
…daily, weekly and monthly
| prev_color = '', | ||
| color = '', | ||
| map; | ||
|
|
||
| if (event_date) { countdown(convertDate(event_date)); } | ||
| if (event_date) { countdown(convertDate(event_date, start_time), end_date, end_time); } |
There was a problem hiding this comment.
NITPICK: This can be expanded out to more lines for better readability.
| } else if (frequency === 'Monthly'){ | ||
| rec_text = 'Every ' + week + ' ' + day + ' of the Month, from '; | ||
| } else { | ||
| rec_text = ''; |
There was a problem hiding this comment.
Can make use of Switch-Case here for more optimization, as well as readability.
| rec_text = ''; | ||
| } | ||
|
|
||
| if (start_date) { countdown(convertDate(start_date, start_time), end_date, end_time); } |
There was a problem hiding this comment.
NITPICK: This can be expanded out to more lines for better readability.
| @@ -26,7 +46,13 @@ $(document).ready(function () { | |||
| var title = $('#event_title').val() === '' ? 'Event title goes here' : $('#event_title').val(); | |||
There was a problem hiding this comment.
NITPICK: avoid evaluating a DOM obj repeatedly, $('#event_title') can be assigned to a variable and then used concurrently after.
| $('.our-event-time').html(rec_text + start_time + ' to ' + end_time); | ||
| } else { | ||
| $('.our-event-date').html(start_date + ', ' + start_time + ' - ' + end_date + ', ' + end_time); | ||
| $('.our-event-time').html(''); |
There was a problem hiding this comment.
NITPICK: $('.our-event-date') and $('.our-event-time') can be assigned to variables for multiple usage.
| @@ -13,6 +13,7 @@ | |||
| require "capybara/rails" | |||
| require "database_cleaner" | |||
| require "capybara/poltergeist" | |||
| require "selenium-webdriver" | |||
| require "webmock/rspec" | |||
|
|
|||
There was a problem hiding this comment.
different configurations can be extracted out into their individual /support/... files to keep concerns neatly separated.
| it { is_expected.to be_valid } | ||
| it { should respond_to :day } | ||
| it { is_expected.to respond_to :week } | ||
| it { should belong_to :event } |
There was a problem hiding this comment.
NITPICK: avoid mixing the should syntax with expect. try to use only expect here because should is deprecated.
| description = "This is a demo description for our event" | ||
| fill_in "event[description]", with: description | ||
| click_link "Next" | ||
| click_link "Preview" | ||
| sleep 2 |
There was a problem hiding this comment.
Why the need for sleep? ( sleeping on the Job? ).
| @@ -107,6 +122,7 @@ | |||
| fill_in "Enter staff email", with: email | |||
| click_button "add_staff" | |||
| click_link "Preview" | |||
| sleep 2 | |||
There was a problem hiding this comment.
Avoid manipulating test with sleep. Let the test behave exactly like the user is meant to. or... is the User meant to sleep on the page? 😀
| @@ -78,6 +88,7 @@ | |||
| fill_in "Enter staff email", with: email | |||
| click_button "add_staff" | |||
| click_link "Preview" | |||
| sleep 2 | |||
| @@ -42,7 +46,9 @@ | |||
| click_link "Next" | |||
|
|
|||
| click_link "Preview" | |||
| sleep 2 | |||
| page.execute_script("$('#event_end_date')\ | ||
| .pickadate('picker').set('select', #{date})") | ||
| fill_in "event[end_time]", with: "02:00PM" |
There was a problem hiding this comment.
Multiple instances of filling the same form. This can be extracted to it's own method which can then be easily called on a single command... something like fill_in_form or ....
| @@ -53,6 +53,7 @@ | |||
| expect(page.current_path).to eq "/tickets" | |||
|
|
|||
| visit print_path(25) | |||
| sleep 2 | |||
There was a problem hiding this comment.
Are these absolutely necessary? why? using sleep to manipulate test should be avoided.
| @@ -1,14 +1,14 @@ | |||
| <nav class='custom_nav <%= "landing" unless @current_page && @current_page == "new" %>'> | |||
| <div class="nav-wrapper"> | |||
| <%= "#{@event.event_template} darken-4 " if controller_name == "events" && action_name == "show"%> | |||
| <!-- "#{@event.event_template} darken-4" if controller_name == "events" && action_name == "show" --> | |||
There was a problem hiding this comment.
If code is obsolete, it should be removed as opposed to just getting commented out.
| </div> | ||
| <% end %> | ||
|
|
||
| <% if @event.recurring_event.frequency == "Monthly" %> |
There was a problem hiding this comment.
Why duplicating same block for two conditions?
You can look into using partials here.
| <% if @event.recurring_event.persisted? %> | ||
| <%= render "events/persisted_recurring_event_fields", f: f %> | ||
| <% else %> | ||
| <%= render "events/new_recurring_event_fields", f: f %> |
| @@ -5,6 +5,7 @@ class Event < ActiveRecord::Base | |||
| has_many :ticket_types, dependent: :destroy | |||
| has_many :event_staffs, dependent: :destroy | |||
| has_many :highlights, dependent: :destroy | |||
| has_one :recurring_event, dependent: :destroy | |||
There was a problem hiding this comment.
for readability purposes... similar associations can be grouped... all has_many together .... and etc.
| else | ||
| "Every " + recurring_event.week + " " + | ||
| recurring_event.day + " of the Month, from " | ||
| end |
|
|
||
| def end_time | ||
| return object.end_time.strftime("%I:%M%p") if object.end_time? | ||
| # Time.now.strftime("%I:%M%p") |
There was a problem hiding this comment.
avoid commenting obsolete codes... this is a candidate for removal.
|
|
||
| def start_time | ||
| return object.start_time.strftime("%I:%M%p") if object.start_time? | ||
| # Time.now.strftime("%I:%M%p") |
There was a problem hiding this comment.
avoid commenting obsolete codes... this is a candidate for removal.
| @@ -7,7 +7,12 @@ def image_url(version) | |||
|
|
|||
| def start_date | |||
| return object.start_date.strftime("%b %d %Y") if object.start_date? | |||
| Time.zone.now.strftime("%b %d %Y") | |||
| # Time.zone.now.strftime("%b %d %Y") | |||
There was a problem hiding this comment.
avoid commenting obsolete codes... this is a candidate for removal.
| @@ -28,7 +33,12 @@ def sponsors | |||
|
|
|||
| def end_date | |||
| return object.end_date.strftime("%b %d %Y") if object.end_date? | |||
| Time.zone.now.strftime("%b %d %Y") | |||
| # Time.zone.now.strftime("%b %d %Y") | |||
There was a problem hiding this comment.
avoid commenting obsolete codes... this is a candidate for removal.
| ticket_types_attributes: [:id, :_destroy, :name, :quantity, :price], | ||
| highlights_attributes: [:id, :_destroy, :day, :title, :description, | ||
| :start_time, :end_time, :image, :image_title], | ||
| recurring_event_attributes: [:id, :_destroy, :frequency, :day, :week], |
[129620817] Story description
What does this PR do?
It adds the functionality that enables Event Managers to create Recurring Events
Description of Task to be completed?
As a User:
I want to create an event with start-time and end-time,
And make it recurring,
So that such events are not created repeatedly.
Scenario 1:
Given I’m a logged-in Event-Manager,
When I go to the homepage,
And I click on "Create Event" button,
Then I should be presented with "New Event" form with the event start-time, end-time and an option to make it recurring.
Scenario 2:
Given that I have clicked the option to make it recurring,
I should be presented with a form to enter the event details.
I should be able to select the frequency (Daily , Weekly, Yearly) for the events.
How should this be manually tested?
git checkout ft-create-recurring-events-129620817bundle installto install dependenciesrake db:setupfor database setupWhat are the relevant pivotal tracker stories?
#129620817
Screenshots (if appropriate)