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: Event & Org support #17

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

WIP: Event & Org support #17

wants to merge 21 commits into from

Conversation

jims
Copy link
Collaborator

@jims jims commented Mar 28, 2019

@gdpelican Hey James, this is the work-in-progress that has been done during this week. It's mostly about getting everything not crashing with the new routes and database structure. I reckon some feedback could be nice :)

@jims jims self-assigned this Mar 29, 2019
Copy link

@gdpelican gdpelican left a comment

Choose a reason for hiding this comment

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

Looks like real good stuff! I've left a few nitpicks / suggestions, but this is looking really good so far!

@@ -15,6 +16,7 @@ def index

def new
@camp = Camp.new

Choose a reason for hiding this comment

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

@camp = @event.camps.build

end

private

def load_event!
@event = Event.find(params[:event_id])

Choose a reason for hiding this comment

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

not_found unless @event = Event.find(params[:event_id])

return if @camp = Camp.find_by(params.permit(:id))
flash[:alert] = t(:dream_not_found)
redirect_to camps_path
@camp = Camp.includes(:event).find(params[:id])

Choose a reason for hiding this comment

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

I think this wants to be

def load_camp!
  return unless @camp = @event.camps.find_by(params[:id])
  flash[:alert] = t(:dream_not_found)
  redirect_to event_camps_path(@event)
end

end

private

def load_camp!
@camp = Camp.find(params[:camp_id])
@event = Event.find(params[:event_id])
not_found if @event.nil? if @event.nil?

Choose a reason for hiding this comment

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

You might consider pulling out load_camp! and load_event! into a module so you don't have to define them in multiple places; this loading code is important, reusable, and shouldn't be defined in more than one place.

At the very least you should drop the double if @event.nil? here :)

session["devise.saml_data"] = request.env["omniauth.auth"]
redirect_to new_user_registration_url
end
c = Rails.application.config.x.firestarter_settings

Choose a reason for hiding this comment

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

We have a helper for this now; should be app_settings('saml_human_name').

set_flash_message :notice, :success, kind: app_settings('saml_human_name')

You can include the AppSettings concern in this controller if that doesn't work straight out of the box.

@@ -19,15 +20,15 @@
<span class='heart' id='heart5' data-animated-heart='<%= asset_path('hearts/heart_05.png')%>'></span>
</div>
<b> <%=t :donate_text %>
<%= number_field_tag 'grants', nil, in: 0..current_user.grants, step: 1, value: 1, class:'donate-value form-control' %>
<%= number_field_tag 'grants', nil, in: 0..grants_left, step: 1, value: 1, class:'donate-value form-control' %>

Choose a reason for hiding this comment

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

Assigning local variables in the view is generally frowned upon; often you'll see a helper method for this purpose (or, if not, simply setting the variable in the controller action like

@grants_left = current_user.grants_left_for(@event)

or

# app/helpers/application_helper.rb or app/helpers/events_helper.rb
def grants_left
  @grants_left ||= current_user.grants_left_for(@event)
end

@@ -65,7 +65,7 @@

<!-- If user grants are > 0, allow user to asign them -->
<% if @camp.grantingtoggle %>
<% if current_user && current_user.grants > 0 && [email protected] %>
<% if current_user && current_user.grants_left_for(@event) > 0 && [email protected] %>

Choose a reason for hiding this comment

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

I'd slightly prefer

current_user.grants_left_for(@event).to_i > 0

here

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

RSpec.describe GrantWallet, type: :model do

Choose a reason for hiding this comment

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

Killall empty spec files! :)


camp.grants.build(user: self.user, amount: num_grants)
camp.assign_attributes(
minfunded: (camp.grants_received + num_grants) >= camp.minbudget,

Choose a reason for hiding this comment

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

A little note here: We'd be way better off taking these columns off of the camp model entirely, and simply writing methods to mimic what they do; there's no need to store booleans like this in the DB if they can be inferred simply from other info on the model.

# camp.rb
def minfunded
  grants_received >= minbudget
end

def fullyfunded
  grants_received >= maxbudget
end

Also note that a slightly more idiomatic naming might be fully_funded? and min_funded?

end

def wallet_for(event)
grant_wallets.find_or_create_by(event: event, user: self) do |w|

Choose a reason for hiding this comment

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

Do we need user: self here? I believe the user might be set to self automatically by calling grant_wallets here.

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.

2 participants