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

Make the cfp more flexible #1523

Merged
merged 6 commits into from Jun 27, 2017

Conversation

@AEtherC0r3
Contributor

AEtherC0r3 commented Jun 6, 2017

Up until now the cfp only accepts requests for events(proposals), this pr modifies it so it can be used for more things (like tracks and booths), while providing backwards compatibility.

  • The column cfp_type allows as to have different kinds of cfps for the same conference
  • The constant Cfp::TYPES defines the allowed values for cfp_type
  • The method Cfp#open? is the equivalent of the older Program#cfp_open? for a specific cfp
  • An index view has been added for the cfps
  • The Cfp#show view was refactored to use partials, so each type of cfp can have it's own partial

@AEtherC0r3 AEtherC0r3 requested a review from differentreality Jun 6, 2017

describe 'validations' do
it { is_expected.to validate_presence_of(:cfp_type) }
it { is_expected.to validate_inclusion_of(:cfp_type).in_array(Cfp::TYPES) }
it { is_expected.to validate_uniqueness_of(:cfp_type).scoped_to(:program_id) }

This comment has been minimized.

@sayalilunkad

sayalilunkad Jun 6, 2017

Just after a brief look try using validate_uniqueness_of(:cfp_type).scoped_to(:program_id).case_insensitive
Will review the whole PR soon.

@sayalilunkad sayalilunkad self-assigned this Jun 9, 2017

@AEtherC0r3 AEtherC0r3 added the GSoC label Jun 12, 2017

##
# ====Returns
# * +Array+ -> The types of cfps for which a cfp doesn't exist yet
def available_cfp_types

This comment has been minimized.

@differentreality

differentreality Jun 13, 2017

Contributor

I would rename this to something that is easier to associate with the result we get (the remaining types for which we can create a cfp)

@differentreality

This comment has been minimized.

Contributor

differentreality commented Jun 13, 2017

undefined method `validate_inclusion_of' for #RSpec::ExampleGroups::Cfp::Validations:0x0000000d77f1c0

error should be fixed if you update shoulda-matchers gem to version 2.8

Conference registration error seems to be an invalid failure, that should be fixed when travis runs again.

I still haven't figured out why we are getting the test failure from validate_uniqueness_of(:cfp_type).scoped_to(:program_id).case_insensitive, so for now let's fix it by checking if program exists when validating.

@differentreality

This comment has been minimized.

Contributor

differentreality commented Jun 13, 2017

@sayalilunkad have you reviewed this?
We should be able to bring it to a mergeable state, and merge it, in the next few days.
To that end, @AEtherC0r3 after rebasing & submitting the travis fix, so that we can review, work on the haml lint errors as well.

@sayalilunkad

This comment has been minimized.

sayalilunkad commented Jun 13, 2017

@differentreality yes I have tested it out and it seems to work as expected. As for the failing travis tests I think validate_uniqueness_of(:cfp_type).scoped_to(:program_id).case_insensitive is also a bug. But other than that it looks fine.

@AEtherC0r3 AEtherC0r3 force-pushed the AEtherC0r3:cfp_changes branch from fa0da2c to 12b21e4 Jun 15, 2017

= f.input :start_date, as: :string, input_html: { id: 'registration-period-start-datepicker', start_date: @conference.start_date, end_date: @conference.end_date, readonly: 'readonly' }
= f.input :end_date, as: :string, input_html: { id: 'registration-period-end-datepicker', readonly: 'readonly' }
= f.input :cfp_type, as: :select, collection: (@cfp.new_record? ? @program.remaining_cfp_types : @program.remaining_cfp_types.insert(0, @cfp.cfp_type)).map {|type| ["#{type.capitalize}", type]}, include_blank: false, label: 'Type', input_html: { class: 'select-help-toggle' }

This comment has been minimized.

@differentreality

differentreality Jun 16, 2017

Contributor

Let's try to make this more readable

.btn-group
= link_to 'Edit', edit_admin_conference_program_cfp_path(@conference.short_title, cfp.id), method: :get, class: 'btn btn-primary'
= link_to 'Delete', admin_conference_program_cfp_path(@conference.short_title, cfp.id), method: 'delete', class: 'btn btn-danger', data: { confirm: 'Are you sure you want to delete the CfP?' }
- if @program.remaining_cfp_types.length > 0

This comment has been minimized.

@differentreality

differentreality Jun 16, 2017

Contributor

Add an ability so that:

  1. This line becomes something like if can? [:new, :create] @program.cfps.new (alternatively you could still be showing the button, just make it disabled with a title saying that it is not possible to create any other type of CfP for the conference
  2. You also authorize the access to the action new/create so that with load_and_authorize in controller it will not be possible to access this page at all
@differentreality

This comment has been minimized.

Contributor

differentreality commented Jun 20, 2017

@AEtherC0r3 let's focus on finalizing this PR first.

@AEtherC0r3 AEtherC0r3 force-pushed the AEtherC0r3:cfp_changes branch from 5eecdbd to d044a42 Jun 20, 2017

@@ -240,4 +240,23 @@
end
end
describe '#cfp' do
it 'return the cfp for events' do

This comment has been minimized.

@differentreality

differentreality Jun 23, 2017

Contributor

Returns

cfp.start_date = Date.new(2014, 05, 26)
cfp.end_date = Date.new(2014, 05, 26) + 21
subject.program.cfp = cfp
cfp.save

This comment has been minimized.

@differentreality
@@ -208,7 +213,7 @@ def signed_in_with_cfp_role(user)
can :manage, Room, venue: { conference_id: conf_ids_for_cfp }
can :show, Venue, conference_id: conf_ids_for_cfp
can :show, Commercial, commercialable_type: 'Venue', commercialable_id: Venue.where(conference_id: conf_ids_for_cfp).pluck(:id)
can :manage, Cfp, program: { conference_id: conf_ids_for_cfp }
can :manage, Cfp, program: { conference_id: conf_ids_for_cfp}

This comment has been minimized.

@differentreality

differentreality Jun 23, 2017

Contributor

Keep the space inside the curly brackets

@differentreality

This comment has been minimized.

Contributor

differentreality commented Jun 23, 2017

Go ahead and squash after you fix the failures 😊

@AEtherC0r3 AEtherC0r3 force-pushed the AEtherC0r3:cfp_changes branch 4 times, most recently from ad13691 to d706516 Jun 23, 2017

AEtherC0r3 added some commits May 30, 2017

Modify CFP to accept proposals for other things
Add field cfp_type and relevant validations
Add Program#cfp to preserve backwards compatibility
Add 'for_events' scope to the cfp, in order for it to be used like
program.cfps.events
Add useful methods
Add rspec test for the new code
The supported cfp types can be viewed via Cfp::TYPES
Add index view for the cfp and modify existing ones
Add cfp_type to the form partial
Refactor Cfps#show to use partials for the different cfp types
Modify the rest of the view, where the cfp was used
Fix rspec tests because of the changes to the cfp
Remove redundant association
Note: a conference created with :full_conference already has a cfp
@AEtherC0r3

This comment has been minimized.

Contributor

AEtherC0r3 commented Jun 27, 2017

Cfp#new
2017-06-27 19-30-57

Cfp#index
2017-06-27 19-30-24

Cfp#show for events
2017-06-27 19-28-35

@differentreality

This comment has been minimized.

Contributor

differentreality commented Jun 27, 2017

Looks nice 👍 Let's get it in 🎉

@differentreality differentreality merged commit f37d309 into openSUSE:master Jun 27, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 86.203%
Details

@AEtherC0r3 AEtherC0r3 deleted the AEtherC0r3:cfp_changes branch Jun 29, 2017

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