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

Ft event find insights 129622087 #191

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

megwali
Copy link
Contributor

@megwali megwali commented Sep 28, 2016

What does this PR do?

Creates a feature that captures and presents insights on how an event was found.

Description of Task to be completed?

As an event manager I can get actionable insights regarding the channels through which users found my event.

How should this be manually tested?

Clone repository and start the server.
Create an event manager profile and at least one event
Create a user profile and attend your event, an option requesting how event was found is now available.
Go to your event manager dashboard and check for the bookings, a field now shows how the event was found.

Any background context you want to provide?

Not really

What are the relevant pivotal tracker stories?

#129622087

Screenshots (if appropriate)

Questions:

left: 2px;
position: relative;
opacity: 1;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can try to leverage more on SCSS syntax for better performance in .scss files

@@ -104,6 +105,14 @@ def ticket_params
params.require(:tickets_quantity)
end

def source_params
params.permit(:event_source, :other)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the need for params.permit... here, given that the returned is later manipulated to give the required params[:event_source] ?

<div class="col l12 m12">
<h6>How did you find this event?</h6>
<hr>
<div class="col l2 m4">
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NITPICK: Indentation here can be better.

<%= radio_button_tag("event_source", "Other", false, class: "modal-radio") %>
<%= text_field_tag("other", "", class: "modal-text") %>
</div>
</div>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NITPICK: Indentation on this div block can be better.

@@ -1,5 +1,8 @@
FactoryGirl.define do
factory :user_ticket do
ticket_number "MyString"
event_source "Facebook"
booking_id 1
ticket_type_id 1
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look into using Faker for getting random attributes here. I can see that it's already included into the project.

@@ -24,7 +24,7 @@

scenario "User tries to attend past Event" do
visit events_path
find_link("Old Event").trigger("click")
click_link"Old Event"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space between click_link and "Old Event"

}

.modal-radio[type="radio"]:not(:checked),
.modal-radio[type="radio"]:checked {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checked and not(:checked) are mutually exclusive. No need to specify the two... this could easily be .modal-radio[type="radio"]

@@ -25,7 +25,8 @@ def create
tickets = []
ticket_params.each do |ticket_type_id, quantity|
quantity.to_i.times do
user = UserTicket.new(ticket_type_id: ticket_type_id, booking: @booking)
user = UserTicket.new(ticket_type_id: ticket_type_id, booking: @booking,
event_source: source_params)
tickets << user
end
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is way too much going on in this controller action which can be moved to the model... ( lean controllers fat models )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant