Skip to content
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

WIP: Ability to add an event #431

Closed
wants to merge 29 commits into from

Conversation

Projects
None yet
5 participants
@StlMaris123
Copy link
Contributor

commented Mar 16, 2017

PT title: Ability to add an event
PT link: https://www.pivotaltracker.com/story/show/67698568

@tansaku

This comment has been minimized.

Copy link
Member

commented Mar 17, 2017

great start @StlMaris123 - need any help with anything?

@StlMaris123

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2017

Yes @tansaku , how do I reduce the number of lines on the create action?It raises an issue on code climate. I also wanted to know whether we are to work on the index and show pages. Thank you.

1 similar comment
@StlMaris123

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2017

Yes @tansaku , how do I reduce the number of lines on the create action?It raises an issue on code climate. I also wanted to know whether we are to work on the index and show pages. Thank you.

@sigu

This comment has been minimized.

Copy link
Member

commented Mar 20, 2017

@StlMaris123 , I guess you now have a clearer way of doing this after the kickoff

.gitignore Outdated
@@ -48,6 +48,8 @@ config/database.yml
.rbenv-gemsets
.floo
.flooignore
Gemfile

This comment has been minimized.

Copy link
@sigu

sigu Mar 20, 2017

Member

Gemfile and Gemfile.lock should not be ignored. They are used for consistency in gems and gem versions used in the project. Incase you made changes to the gemfile that you do not want to be checked out, when adding files to staging on git do not include the gemfile ( dont know if i am making sense )

end

def create
unless current_user.try(:superadmin?)

This comment has been minimized.

Copy link
@sigu

sigu Mar 20, 2017

Member

This can be written in a private method that checks if user is super admin. it will help you in solving the issue you had in #431 (comment)

  1. We can then have a before action to check if user is superuser
class EventsController < ApplicationController
  before_action :is_superadmin?, except[:show, :index]
# rest of the class........
  1. and the method is reduce to this
def create
@event = Event.new(event_params)
  if @event.save
    redirect_to @event, notice: 'Event was successfully created'
    else
      render :new
   end
end
  1. the private method is then defined as
private
# previous code is here
def is_superadmin?
  unless current_user.try(:superadmin?)
  flash[:notice] = PERMISSION_DENIED
  redirect_to events_path and return false
end

This comment has been minimized.

Copy link
@StlMaris123

StlMaris123 Mar 20, 2017

Author Contributor

But the create action still has 6 lines

This comment has been minimized.

Copy link
@StlMaris123

StlMaris123 Mar 21, 2017

Author Contributor

But the create action still has 6 lines
On the line:
redirect_to events_path and return false
if I use && in place of and as code climate had suggested, it raises an error.

This comment has been minimized.

Copy link
@tansaku

tansaku Mar 21, 2017

Member

yeah, it's annoying - that &&/and thing is one where we have to ignore codeclimate - I will open a chore to adjust our codeclimate.yml to not flag that - apologies for the inconvenience: https://www.pivotaltracker.com/story/show/142133741

@@ -0,0 +1,6 @@
class Event < ActiveRecord::Base

This comment has been minimized.

Copy link
@sigu

sigu Mar 20, 2017

Member

we can reduce line count by doing

class Event < ActiveRecord::Base
  validates_presence_of :title, :description #yada yada
end

This comment has been minimized.

Copy link
@StlMaris123

StlMaris123 Mar 21, 2017

Author Contributor

This shortens the code but its a violation against rubocop, do we go by fewer lines that violate rubocop or many lines that that are okay i.e validate each param separately?

@@ -1,5 +1,7 @@
LocalSupport::Application.routes.draw do

get 'events/new'

This comment has been minimized.

Copy link
@sigu

sigu Mar 20, 2017

Member

We do not need this since it is covered in line 30

resources :events, :only => [:new, :create, :show, :index, :update, :destroy] 
@@ -25,6 +27,7 @@
resources :proposed_organisation_edits, :only => [:new, :show, :create, :update]
end
resources :users
resources :events, :only => [:new, :create, :show, :index, :update, :destroy]

This comment has been minimized.

Copy link
@sigu

sigu Mar 20, 2017

Member

We can as well omit the only section since you are allowing every action of the controller

@@ -0,0 +1,12 @@
class CreateEvents < ActiveRecord::Migration
def change
create_table :events do |t|

This comment has been minimized.

Copy link
@sigu

sigu Mar 20, 2017

Member

Does an event not belong to any other model? ( is it not associated with another model? ) 🤔

This comment has been minimized.

Copy link
@StlMaris123

StlMaris123 Mar 21, 2017

Author Contributor

So far i don't think there is any relationship

This comment has been minimized.

Copy link
@sigu

sigu Mar 21, 2017

Member

So how can we know who created the event, or which organization has organised the event ? 😊

This comment has been minimized.

Copy link
@tansaku

tansaku Mar 21, 2017

Member

we will want things like that eventuallyu @sigu but it hasn't been specified at the moment, so those relationships can be introduced in other tickets I think :-)

RSpec.describe EventsController, type: :controller do

describe "GET #new" do
it "returns http success" do

This comment has been minimized.

Copy link
@sigu

sigu Mar 20, 2017

Member

302 is actually a redirect 😄

@StlMaris123

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2017

Niice . Thank you so much for the review. Am on it

StlMaris123 added some commits Mar 21, 2017

@StlMaris123

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2017

@tansaku @sigu Am done with the major task which included:

  • Creating an acceptance test
  • Create form
  • Create routes
  • Create controller
  • Model tests
  • Create an event model
    However, I still have a few issues on code climate. Am also open to anything else that needs improvement. Thank you
@tansaku

This comment has been minimized.

Copy link
Member

commented Mar 23, 2017

thanks @StlMaris123 - great work - will try to review ASAP

@@ -0,0 +1,5 @@
require 'rails_helper'

This comment has been minimized.

Copy link
@tansaku

tansaku Mar 27, 2017

Member

can delete this file I think - I don't believe that view specs are giving us any value

@@ -0,0 +1,12 @@
require 'rails_helper'

This comment has been minimized.

Copy link
@tansaku

tansaku Mar 27, 2017

Member

can delete this file I think - I don't believe that controller specs are giving us any value

@tansaku

This comment has been minimized.

Copy link
Member

commented Mar 27, 2017

great work @StlMaris123

one thing we might want to sort out is the layout for the events new page:

it's actually a general problem for a few of our forms since the bootstrap 3 update - we need to adjust all the forms to use the latest bootstrap styles so they are aligned properly like so:

please don't worry if you're not sure how to do this - we can look together in more detail at the best bootstrap 3 style to use ...

@StlMaris123

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2017

Hi @tansaku Let me try it first. If I get stuck ill get back to you.

StlMaris123 added some commits Apr 5, 2017

@tansaku

This comment has been minimized.

Copy link
Member

commented Apr 21, 2017

thanks @StlMaris123 - I'll try to give a detailed UX review shortly - in the meantime does the codeclimate comment about the guard clause make sense? Is that something you could fix by yourself, or do you need my input?

create_table :events do |t|
t.string :title
t.text :description
t.string :start_date

This comment has been minimized.

Copy link
@sigu

sigu Apr 24, 2017

Member

The date should be of datetime type

@sigu
Copy link
Member

left a comment

change of mind 😄 , you need to remove the vcr files from this PR

@tansaku

This comment has been minimized.

Copy link
Member

commented Apr 24, 2017

@StlMaris123 I tried to pull this out locally to review, but config/database.yml is missing ...

also, ideally this new feature would be on a new feature branch, not on the events branch that I think you used already ...

@StlMaris123 StlMaris123 force-pushed the StlMaris123:events branch from 2382b77 to c883e15 Apr 25, 2017

@sigu

This comment has been minimized.

Copy link
Member

commented Apr 25, 2017

@tansaku I guess she made it through on the same branch, you can merge it in once @cesswairimu does a review. Lets give her a chance to learn how to review PRs. I will help her through

@sigu

This comment has been minimized.

Copy link
Member

commented Apr 27, 2017

screenshot from 2017-04-27 21-35-55

Some CSS styling for better UX 😛

@sigu
Copy link
Member

left a comment

Consider most importantly the guard clause section.

@@ -0,0 +1,15 @@
require 'rails_helper'

# Specs in this file have access to a helper object that includes

This comment has been minimized.

Copy link
@sigu

sigu Apr 27, 2017

Member

Remove the comments and the pending spec which might cause failtures

def superadmin?
flash[:notice] = PERMISSION_DENIED
redirect_to events_path and return false
unless current_user.try(:superadmin?)

This comment has been minimized.

Copy link
@sigu

sigu Apr 27, 2017

Member

This will not work as intended, reconsider positining the unless statement

@@ -0,0 +1,3 @@
# Place all the behaviors and hooks related to the matching controller here.

This comment has been minimized.

Copy link
@sigu

sigu Apr 27, 2017

Member

You can optionally remove this file

@tansaku

This comment has been minimized.

Copy link
Member

commented May 4, 2017

@StlMaris123 @sigu any progress here?

@StlMaris123

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2017

@Maroo-b

This comment has been minimized.

Copy link
Contributor

commented May 5, 2017

@StlMaris123 I agree that datetime select helpers are tricky at first, I suggest that you implement the method described from this blog

@@ -25,6 +25,7 @@
resources :proposed_organisation_edits, :only => [:new, :show, :create, :update]
end
resources :users
resources :events

This comment has been minimized.

Copy link
@Maroo-b

Maroo-b May 5, 2017

Contributor

Do you intend to implement all the RESTful actions ? becaause from events_controllerrb there is only new and create implemented
For best practices tend to add the routes that you only need to help developers understand what events_controller is doing by looking at the available routes, for this example:
resources :events, only: [:new, :create]

@StlMaris123

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2017

StlMaris123 and others added some commits May 9, 2017

@tansaku

This comment has been minimized.

Copy link
Member

commented May 18, 2017

@tansaku

This comment has been minimized.

Copy link
Member

commented May 18, 2017

@tansaku

This comment has been minimized.

Copy link
Member

commented May 18, 2017

@StlMaris123 one thing I just noticed here - we need to have a feature flag so that we can turn this on or off while we're still working on all the other components.

martin fowler calls them feature toggles:

https://martinfowler.com/bliki/FeatureToggle.html

we had a feature flag system in place - see:

https://github.com/AgileVentures/LocalSupport/blob/develop/CONTRIBUTING.md#feature-flags

@tansaku

This comment has been minimized.

Copy link
Member

commented May 22, 2017

@StlMaris123 @rehan-work wonderful that you've started work on the listing events feature, but we really need the feature toggle in first so that we can deploy

We want to get that feature toggle in first and merge the PR, then move on to the next feature. If either of you can join any Kent Beck or Martin Fowler scrum this week I can explain in more detail :-)

StlMaris123 added some commits May 23, 2017

@StlMaris123

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2017

event

@tansaku

This comment has been minimized.

Copy link
Member

commented May 24, 2017

that looks nice @StlMaris123 but did you understand my point about putting a feature toggle in place, and separating features out onto different branches?

@StlMaris123

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2017

@tansaku tansaku closed this Jun 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.