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

Track related refactoring #1639

Merged
merged 1 commit into from Aug 27, 2017

Conversation

3 participants
@AEtherC0r3
Contributor

AEtherC0r3 commented Aug 17, 2017

  • Add roles as nested routes to track (for the track organizer role)
  • Allow transition from to_accept to to_reject and backwards
  • Split Track#valid_dates validation to many independent ones
  • Show all the confirmed tracks in the conference's splashpage
  • Add comment in admin/Tracks#toggle_cfp_inclusion
  • Rewrite admin/TracksController#accept spec
  • Add feature spec for track requests
  • Change 'In' to 'Room' in Tracks#index
  • Rewrite Track#overlapping
  • Refactor code in ProposalsController
  • Fix typos
@differentreality

Very nice! I've left some refactor comments to look through, before merging this. Let me know your thoughts.

@@ -23,7 +24,7 @@
within('table#tracks') do
expect(page.has_content?('Distribution')).to be true
expect(page.has_content?('Events about our Linux')).to be true
expect(page.assert_selector('tr', count: 2)).to be true
expect(Track.count).to eq 1

This comment has been minimized.

@differentreality

differentreality Aug 19, 2017

Contributor

It's preferable to use expect {}.to change_by(1), in case later on a track already exists in our database, so this count won't be 1, but 2.
Our test is not testing if the DB has only 1 track - neither should it- it is testing if a track was added (so if Track.count was increased by 1), is that right?

The same goes for a few of the following tests.

self_organized_track.end_date = Date.today
conference.venue = create(:venue)
self_organized_track.room = create(:room, venue: conference.venue)
self_organized_track.start_date = Date.current

This comment has been minimized.

@differentreality

differentreality Aug 19, 2017

Contributor

For this case you can simply define a variable, using let, named full_self_organized_track or something similar, that indicates that this is a self_organized track with all of its attributes.

it_behaves_like 'fails to accept', true, true, false
before :each do
self_organized_track.start_date = Date.current
self_organized_track.end_date = Date.current

This comment has been minimized.

@differentreality

differentreality Aug 19, 2017

Contributor

Using the predefined track with all attributes, you can be simply removing the ones you do not want, which is going to reduce the lines of our tests (that's good because we have less to read and understand).

it_behaves_like 'fails to accept', false, false, false
context 'has none of start_date, end_date, room' do
before :each do
self_organized_track.start_date = nil

This comment has been minimized.

@differentreality

differentreality Aug 19, 2017

Contributor

We can have another variable for a track that has none of those extra attributes (dates, room) set. The same variable can be used in other cases, where you only want to set one of the attribute to non nil.

end
end
shared_examples 'tracks' do

This comment has been minimized.

@differentreality

differentreality Aug 19, 2017

Contributor

These shared examples are all about managing / working with tracks, right?
Perhaps the name could be a little bit more specific?

@@ -29,8 +29,12 @@ class Track < ActiveRecord::Base
validates :room, presence: true, if: :self_organized_and_accepted_or_confirmed?
validates :relevance, presence: true, if: :self_organized?
validates :description, presence: true, if: :self_organized?
validate :valid_dates
validate :valid_room, if: :self_organized_and_accepted_or_confirmed?
validate :start_date_after_conference_start_date

This comment has been minimized.

@differentreality

differentreality Aug 19, 2017

Contributor

I'd combine the test related to the conference date into 1, dates_within_conference_dates or something similar, and I would test if start_date is within the range of conf.start_date..conf.end_date

As long as you have the validation start_date_before_end_date, that should be sufficient. What do you think?

context 'is valid' do
it 'when the track\'s start date is before its end date' do

This comment has been minimized.

@differentreality

differentreality Aug 19, 2017

Contributor

This can probably be combined with https://github.com/openSUSE/osem/pull/1639/files#diff-c2212054e3536a54e738fecea403adffR117

so that we are testing track dates within conference dates (I would even have it start on conf.start_date and end on conf.end_date which is our edge case).

And that would be enough for our tests, since we will test the start_date before end_date in the other validation.

@differentreality

This comment has been minimized.

Contributor

differentreality commented Aug 19, 2017

I'd also transform tracks#index to show table headers for 'From', 'To', 'In'

@AEtherC0r3

This comment has been minimized.

Contributor

AEtherC0r3 commented Aug 20, 2017

@differentreality I'm having trouble refactoring the refactored admin/TracksController spec. The variable I define (@this_track) isn't available in the it_behaves_like directive, to be passed as a variable. Also, I can't pass a track defined in a let to it. I'm including the commit for reference (8c560af).
In my opinion, we should ditch the second track and, this way, get rid of the need to have a variable. This is a hybrid solution between my initially refactored code and what you proposed in the comments.
What do you think?

@AEtherC0r3 AEtherC0r3 force-pushed the AEtherC0r3:track_refactoring branch from 8c560af to 7da5f37 Aug 23, 2017

%th
%th From
%th To
%th In

This comment has been minimized.

@differentreality

differentreality Aug 23, 2017

Contributor

Perhaps that's more meaningful if it says 'Room' or 'In room', what do you think?

This comment has been minimized.

@AEtherC0r3

AEtherC0r3 Aug 24, 2017

Contributor

I think that 'Room' is enough

@sayalilunkad

Please squash the commits before merging them.

@AEtherC0r3 AEtherC0r3 dismissed stale reviews from sayalilunkad and differentreality via 7f21a4c Aug 24, 2017

@AEtherC0r3 AEtherC0r3 force-pushed the AEtherC0r3:track_refactoring branch 3 times, most recently from 986ed48 to 3614791 Aug 24, 2017

Track related refactoring
Add roles as nested routes to track (for the track organizer role)
Allow transition from to_accept to to_reject and backwards
Split Track#valid_dates validation to many independent ones
Show all the confirmed tracks in the conference's splashpage
Add comment in admin/Tracks#toggle_cfp_inclusion
Rewrite admin/TracksController#accept spec
Add feature spec for track requests
Change 'In' to 'Room' in Tracks#index
Rewrite Track#overlapping
Refactor code in ProposalsController
Fix typos

@AEtherC0r3 AEtherC0r3 force-pushed the AEtherC0r3:track_refactoring branch from 3614791 to 65552d0 Aug 27, 2017

@differentreality differentreality merged commit 27fa79a into openSUSE:master Aug 27, 2017

1 of 3 checks passed

Hakiri Security warnings were found.
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
coverage/coveralls First build on master at 83.856%
Details
@differentreality

This comment has been minimized.

Contributor

differentreality commented Aug 27, 2017

Nice work @AEtherC0r3 👍

@AEtherC0r3 AEtherC0r3 deleted the AEtherC0r3:track_refactoring branch Aug 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment