Conversation
…to feature/events-organizations
| @@ -15,6 +16,7 @@ def index | |||
|
|
|||
| def new | |||
| @camp = Camp.new | |||
| private | ||
|
|
||
| def load_event! | ||
| @event = Event.find(params[:event_id]) |
There was a problem hiding this comment.
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]) |
There was a problem hiding this comment.
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
| def load_camp! | ||
| @camp = Camp.find(params[:camp_id]) | ||
| @event = Event.find(params[:event_id]) | ||
| not_found if @event.nil? if @event.nil? |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
| </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' %> |
There was a problem hiding this comment.
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
| <!-- If user grants are > 0, allow user to asign them --> | ||
| <% if @camp.grantingtoggle %> | ||
| <% if current_user && current_user.grants > 0 && !@camp.fullyfunded %> | ||
| <% if current_user && current_user.grants_left_for(@event) > 0 && !@camp.fullyfunded %> |
There was a problem hiding this comment.
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 | |||
|
|
||
| camp.grants.build(user: self.user, amount: num_grants) | ||
| camp.assign_attributes( | ||
| minfunded: (camp.grants_received + num_grants) >= camp.minbudget, |
There was a problem hiding this comment.
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| |
There was a problem hiding this comment.
Do we need user: self here? I believe the user might be set to self automatically by calling grant_wallets here.
2 participants
@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 :)