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 requests and track organizer #1556

Merged
merged 3 commits into from Jul 14, 2017

Conversation

@AEtherC0r3
Contributor

AEtherC0r3 commented Jun 23, 2017

Work in progress implementation of track requests and the track organizer role

  • Add the fields submitter_id, state, and cfp_active to tracks
  • Add validations and the self_organized? method to the Track model
  • Create a new TracksController outide of the admin namespace
  • Create the relevant views for index, show, new and edit
  • Modify the admin views for tracks to include extra info for self-organized tracks
  • Add track organizer role and auto-create it when a self-organized track is created
  • Define abilities for track organizers
  • Modify the roles views and controller to handle the new role

@AEtherC0r3 AEtherC0r3 force-pushed the AEtherC0r3:track_requests branch 2 times, most recently from e88c26b to 81fb69e Jun 27, 2017

@@ -66,7 +66,7 @@ Metrics/AbcSize:
# Offense count: 202
# Configuration parameters: CountComments, ExcludedMethods.
Metrics/BlockLength:
Max: 487
Max: 658

This comment has been minimized.

@differentreality

differentreality Jun 29, 2017

Contributor

Why don't you exclude the relevant file?

This comment has been minimized.

@AEtherC0r3

AEtherC0r3 Jun 29, 2017

Contributor

I tried to exclude it but for some reason it didn't work, so I ended up increasing the maximum length

@differentreality

This comment has been minimized.

Contributor

differentreality commented Jun 29, 2017

@AEtherC0r3 how is the DB table roles going to look like after your changes? Please add a table/image in comments.

authorize! :index, @role
end
def show
@url = if @track
toggle_user_track_admin_conference_role_path(@conference.short_title, @role.name, @track.name.tr(' ', '_'))

This comment has been minimized.

@differentreality

differentreality Jun 29, 2017

Contributor

Instead of changing the track.name, it would make sense to add a short_name like conference has, and change how we navigate to tracks all together, and base it on track.short_name instead of track.id

This comment has been minimized.

@AEtherC0r3

AEtherC0r3 Jun 30, 2017

Contributor

#1565 should take care of this

This comment has been minimized.

@lagartoflojo

lagartoflojo Jul 6, 2017

Contributor

Actually, Rails already has a way of turning a model into the appropriate URL parameter, via to_param.

By implementing #to_param like:

def to_param
  short_title # or `name` in Role
end

you can then just do:

toggle_user_track_admin_conference_role_path(@conference, @role, @track)

This comment has been minimized.

@AEtherC0r3

AEtherC0r3 Jul 6, 2017

Contributor

@lagartoflojo thanks for the tip

@AEtherC0r3

This comment has been minimized.

Contributor

AEtherC0r3 commented Jun 29, 2017

I haven't made any changes to the roles table.
In any case, here's an image that shows track organizer roles
2017-06-29 18-35-32

@AEtherC0r3 AEtherC0r3 force-pushed the AEtherC0r3:track_requests branch 5 times, most recently from 49c0461 to 083f2d6 Jun 30, 2017

@AEtherC0r3 AEtherC0r3 removed the in progress label Jul 3, 2017

@AEtherC0r3 AEtherC0r3 changed the title from [WIP] Track requests and track organizer to Track requests and track organizer Jul 3, 2017

@AEtherC0r3 AEtherC0r3 moved this from In progress to In review in [GSoC17] Submission process & scheduling for tracks Jul 3, 2017

@differentreality

This comment has been minimized.

Contributor

differentreality commented Jul 4, 2017

This is dependent on #1565

@AEtherC0r3 AEtherC0r3 force-pushed the AEtherC0r3:track_requests branch from 083f2d6 to c167d7f Jul 4, 2017

@AEtherC0r3

This comment has been minimized.

Contributor

AEtherC0r3 commented Jul 4, 2017

@differentreality @sayalilunkad Sorry for the gigantic commit, properly separating the changes in different commits and fixing the conflicts was really difficult :(

@AEtherC0r3 AEtherC0r3 force-pushed the AEtherC0r3:track_requests branch 2 times, most recently from 81b967c to 4f5843d Jul 5, 2017

@AEtherC0r3

This comment has been minimized.

Contributor

AEtherC0r3 commented Jul 5, 2017

Tracks#new
2017-06-09 00-08-15

admin/Tracks#new
2017-06-09 00-04-20

Tracks#index
2017-06-09 00-10-14

admin/Tracks#index
2017-06-09 09-42-15

Tracks#edit
2017-06-09 00-10-27

admin/Tracks#edit for self organized tracks
2017-06-09 00-11-28

admin/Tracks#edit for regular tracks
2017-06-09 00-04-44

Tracks#show
2017-06-09 10-00-19

@AEtherC0r3 AEtherC0r3 force-pushed the AEtherC0r3:track_requests branch from 4f5843d to 2095641 Jul 5, 2017

@AEtherC0r3 AEtherC0r3 force-pushed the AEtherC0r3:track_requests branch from 357dbe0 to 288d26f Jul 11, 2017

@AEtherC0r3

This comment has been minimized.

Contributor

AEtherC0r3 commented Jul 11, 2017

The remaining rubocop offense will be fixed by #1569

@sayalilunkad

This comment has been minimized.

sayalilunkad commented Jul 12, 2017

needs rebase

@AEtherC0r3 AEtherC0r3 force-pushed the AEtherC0r3:track_requests branch 3 times, most recently from 3de0a03 to 85fa1cb Jul 12, 2017

def create
@track = @program.tracks.new(track_params)
@track.submitter = current_user
@track.state = 'pending'

This comment has been minimized.

@differentreality

differentreality Jul 13, 2017

Contributor

Why don't you keep 'new' as we have in other places as well?

This comment has been minimized.

@AEtherC0r3

AEtherC0r3 Jul 13, 2017

Contributor

It makes more sense to me for a request to be pending than to be new. If you wish I can change it back to new.

method: :get, class: 'btn btn-primary'
= link_to 'Delete', admin_conference_program_track_path(@conference.short_title, track.short_name),
= link_to 'Delete', admin_conference_program_track_path(@conference.short_title, track),

This comment has been minimized.

@differentreality

differentreality Jul 13, 2017

Contributor

For self organized tracks, destroy button should be disabled, so that the record doe snot get deleted, only moved to withdrawn/cancelled state.

@differentreality

This comment has been minimized.

Contributor

differentreality commented Jul 13, 2017

@AEtherC0r3 please fix the errors and prepare your PR for merging! 🎉 Nice work 👍

@AEtherC0r3 AEtherC0r3 force-pushed the AEtherC0r3:track_requests branch from 36d6abd to 111bf9c Jul 13, 2017

AEtherC0r3 added some commits Jun 8, 2017

Implement track requests and add the track organizer role
About track requests:
Create migration that adds the fields submitter_id, state, and
cfp_active to Tracks
Add validations and the self_organized? method to the Track model
Create a new TracksController outide of the admin namespace
Create the relevant views for index, show, new and edit
Modify the admin views for tracks to include extra info for
self-organized tracks

About track organizers:
Create the role when a self-organized track is created
Define track organizer abilities
Modify the roles views and controller to handle the new role

The route for Roles#edit needs to have higher priority than the nested
routes for track roles, otherwise, the word edit in the url is matched
as a track with short_name edit
Add to_param to the Track model
And update the urls

@AEtherC0r3 AEtherC0r3 force-pushed the AEtherC0r3:track_requests branch from 111bf9c to a843bb8 Jul 13, 2017

before_validation :capitalize_color
after_create :create_organizer_role, if: :self_organized?

This comment has been minimized.

@differentreality

differentreality Jul 14, 2017

Contributor

Doesn't this need to happen after we accept the track?

This comment has been minimized.

@AEtherC0r3

AEtherC0r3 Jul 17, 2017

Contributor

This is fixed in #1597

= f.input :name
= f.input :short_name, hint: "A short and unique handle for the track, using only letters, numbers, underscores, and dashes. This will be used to identify the track in URLs etc. Example: 'my_awesome_track'", input_html: { required: 'required', pattern: '[a-zA-Z0-9_-]+', title: 'Only letters, numbers, underscores, and dashes.' }
= f.input :color, input_html: {size: 6, type: 'color'}, required: true
= f.input :description, input_html: {rows: 2, data: { provide: 'markdown-editable' } }, hint: markdown_hint
- if @track.self_organized?
= f.input :cfp_active, label: 'Allow event submitters to select this track for their proposal'

This comment has been minimized.

@differentreality

differentreality Jul 14, 2017

Contributor

Since we have that option anyway, why not make it available to all tracks, to avoid confusion? We can mark it as true when the track is created in the admin view.

This comment has been minimized.

@AEtherC0r3

AEtherC0r3 Jul 20, 2017

Contributor

This is also fixed in #1597

%span.label{style: "background-color: #{track.color}; color: #{ contrast_color(track.color) }"}
= track.color
%td
- if track.self_organized?

This comment has been minimized.

@differentreality

differentreality Jul 14, 2017

Contributor

We could always be showing the state, and mark as accepted/confirmed the tracks that are created on admin side

This comment has been minimized.

@AEtherC0r3

AEtherC0r3 Jul 17, 2017

Contributor

This is fixed in #1597

member do
post :toggle_user
get ':track_name' => 'roles#show', as: 'track'

This comment has been minimized.

@differentreality

differentreality Jul 14, 2017

Contributor

This still needs to be called track_id by the convention of the routes format

This comment has been minimized.

@AEtherC0r3

AEtherC0r3 Aug 17, 2017

Contributor

Fixed in #1639

member do
post :toggle_user
get ':track_name' => 'roles#show', as: 'track'

This comment has been minimized.

@differentreality

differentreality Jul 14, 2017

Contributor

What about removing all those extra lines and doing it in a more rails way? Perhaps nest the roles resources under the admin/tracks? Please think about how we could simply this route.

This comment has been minimized.

@AEtherC0r3

AEtherC0r3 Aug 17, 2017

Contributor

This is fixed in #1639

@differentreality

This comment has been minimized.

Contributor

differentreality commented Jul 14, 2017

I have left some comments which can be added in your next PRs as the feature progresses. Travis failed test is not related to your PR, so I think this is good to be merged 👍

@differentreality differentreality merged commit e3b51df into openSUSE:master Jul 14, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Hakiri No security warnings were found.
Details
coverage/coveralls Coverage increased (+0.3%) to 86.393%
Details

@AEtherC0r3 AEtherC0r3 deleted the AEtherC0r3:track_requests branch Jul 14, 2017

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