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

View for event participants #80

Merged
merged 2 commits into from Aug 3, 2017

Conversation

Projects
None yet
4 participants
@nikhilgupta1211
Contributor

nikhilgupta1211 commented Jul 11, 2017

No description provided.

@nikhilgupta1211

This comment has been minimized.

Contributor

nikhilgupta1211 commented Jul 11, 2017

events#show view

image

events#participants view

image

def participants
@breadcrumbs = [label: 'events', url: events_path]
@breadcrumbs << { label: @event.name, url: event_path(@event) }
@breadcrumbs << { label: 'participants' }

This comment has been minimized.

@bgeuken

bgeuken Jul 11, 2017

Member

Since this is static data, I would prefer to assign all data to @breadcrumbs at once.

This comment has been minimized.

@nikhilgupta1211

nikhilgupta1211 Jul 11, 2017

Contributor

yes ok

%th State
%th Role
%th Change role to event organizer
- @event.requests.each_with_index do |e,i|

This comment has been minimized.

@bgeuken

bgeuken Jul 11, 2017

Member

Please use more speaking variable names for iterators that go over more than one line of code, eg index and event

This comment has been minimized.

@nikhilgupta1211
@nikhilgupta1211

This comment has been minimized.

Contributor

nikhilgupta1211 commented Jul 11, 2017

image

@@ -4,6 +4,10 @@ class EventsController < InheritedResources::Base
skip_load_and_authorize_resource only: [:index, :show]
before_filter :set_types
def participants
@breadcrumbs = [{ label: 'events', url: events_path }, { label: @event.name, url: event_path(@event) }, { label: 'participants' }]

This comment has been minimized.

@bgeuken

bgeuken Jul 11, 2017

Member

Of course a matter of taste. I would prefer this format:

@breadcrumbs = [
  { label: 'events', url: events_path },
  { label: @event.name, url: event_path(@event) },
  { label: 'participants' }
] 

@ChrisBr @nikhilgupta1211 What do you say?

@ChrisBr

I also don't like how you grouped your commits as the view is basically useless without the controller action, right? Also the tests should belong to the implementation ...

%th State
%th Role
%th Change role to event organizer
- @event.requests.map(&:user).uniq.each_with_index do |user,index|

This comment has been minimized.

@ChrisBr

ChrisBr Jul 12, 2017

Member

You could use with_index(1) so you don't need to add +1 in the loop.
https://apidock.com/ruby/Enumerator/with_index

Also I don't really like the query and mapping etc. I would prefer to do that in the view in a proper way.

User.where(request: @event).includes(:request).distinct.

This would also trigger several n+1 queries
http://guides.rubyonrails.org/active_record_querying.html#eager-loading-associations

This comment has been minimized.

@bgeuken

bgeuken Jul 12, 2017

Member

Right, the query needs some love. I guess a join would make sense here

This comment has been minimized.

@nikhilgupta1211

nikhilgupta1211 Jul 12, 2017

Contributor

@ChrisBr thank you :), I didn't knew about the eager loading associations.

Before it was taking 12 queries now it is taking only 1 after changing it to :
User.includes(:requests, :profile).where(requests: {event: @event})

@@ -4,6 +4,14 @@ class EventsController < InheritedResources::Base
skip_load_and_authorize_resource only: [:index, :show]
before_filter :set_types
def participants

This comment has been minimized.

@ChrisBr

ChrisBr Jul 12, 2017

Member

Hm, @bgeuken what do you think about introducing controller namespaces as I don't like introducing participants here.

http://jeromedalbert.com/how-dhh-organizes-his-rails-controllers/

This comment has been minimized.

@bgeuken

bgeuken Jul 12, 2017

Member

Yes, why not. Or, if the PR get's to complex we could refactor this once this got merged.

This comment has been minimized.

@nikhilgupta1211

nikhilgupta1211 Jul 13, 2017

Contributor

Can you please tell that should we do the refactoring in this PR only ?

This comment has been minimized.

@bgeuken

bgeuken Jul 14, 2017

Member

Keep it for now. It can be refactored on a separate PR.

This comment has been minimized.

@nikhilgupta1211

nikhilgupta1211 Jul 14, 2017

Contributor

ok thanks :)

@@ -16,4 +16,28 @@ def users_for_event(state)
req = req.where(state: state) if state != 'all'
user_email = req.map { |e| e.user.email }.uniq
end
def check_states(user)
requests = user.requests.where(event: @event)

This comment has been minimized.

@ChrisBr

ChrisBr Jul 12, 2017

Member

Is there a way to not trigger another db query again?

@@ -16,4 +16,27 @@ def users_for_event(state)
req = req.where(state: state) if state != 'all'
user_email = req.map { |e| e.user.email }.uniq
end
def check_states(requests)
if requests.map { |r| r.event = @event }.count > 1

This comment has been minimized.

@bgeuken

bgeuken Jul 13, 2017

Member

I would prefer to do this in sql, like joining the events and then limit the result to 2.

%th State
%th Role
%th Change role to event organizer
- User.includes(:requests, :profile).where(requests: {event: @event}).each.with_index(1) do |user,index|

This comment has been minimized.

@ChrisBr

ChrisBr Jul 13, 2017

Member

While thinking about it: What about having a has_many through association in Request
http://guides.rubyonrails.org/association_basics.html#the-has-many-through-association

and then you can do in the controller

@users = @event.users.distinct

Caution: Untested and please check if the distinct is necessary

@@ -16,4 +16,27 @@ def users_for_event(state)
req = req.where(state: state) if state != 'all'
user_email = req.map { |e| e.user.email }.uniq
end
def check_states(requests)
if requests.map { |r| r.event = @event }.count > 1

This comment has been minimized.

@ChrisBr

ChrisBr Jul 13, 2017

Member

Hm, this is not nice! You could do sth like this requests.where(event: @event).count but that would still trigger n + 1 queries. @bgeuken any better solution? We could also iterate over the requests in the view but I think this is strange because the table is about users and not about requests and then we would need to do always request.user.foo to access attributes ...

This comment has been minimized.

@nikhilgupta1211

nikhilgupta1211 Jul 14, 2017

Contributor

I checked requests.where(event: @event).count do not trigger n+1 queries

@coveralls

This comment has been minimized.

coveralls commented Jul 14, 2017

Coverage Status

Coverage increased (+0.007%) to 92.01% when pulling de08bf1 on nikhilgupta1211:evenorgview into c598ebb on openSUSE:master.

%th State
%th Role
%th Change role to event organizer
- @event.users.includes(:profile).distinct.each.with_index(1) do |user,index|

This comment has been minimized.

@bgeuken

bgeuken Jul 14, 2017

Member

Much nicer 👍

This comment has been minimized.

@bgeuken

bgeuken Jul 14, 2017

Member

But since within the loop we still query the requests, you might want to include the requests here.

This comment has been minimized.

@nikhilgupta1211

nikhilgupta1211 Jul 14, 2017

Contributor

No we are not quering for the requests here as we are just sending all the requests of a user in the method.

This comment has been minimized.

@bgeuken

bgeuken Jul 14, 2017

Member

But user requests would result in another db query, right? Including the requests before would cause rails to preload them and thus reduce the number of queries.

This comment has been minimized.

@nikhilgupta1211

nikhilgupta1211 Jul 14, 2017

Contributor

I tried but it is still taking the same number of queries

This comment has been minimized.

@bgeuken

bgeuken Jul 14, 2017

Member

Ok. Then nevermind

This comment has been minimized.

@ChrisBr

ChrisBr Jul 18, 2017

Member

I would also move this to the controler

controller#participants

@users = @event.users.includes(:profile)

view

@users.each.with_index(1) ...

@openSUSE openSUSE deleted a comment from coveralls Jul 14, 2017

@openSUSE openSUSE deleted a comment from coveralls Jul 14, 2017

@openSUSE openSUSE deleted a comment from coveralls Jul 14, 2017

@openSUSE openSUSE deleted a comment from coveralls Jul 14, 2017

@openSUSE openSUSE deleted a comment from coveralls Jul 14, 2017

@openSUSE openSUSE deleted a comment from coveralls Jul 14, 2017

@openSUSE openSUSE deleted a comment from coveralls Jul 14, 2017

@openSUSE openSUSE deleted a comment from coveralls Jul 14, 2017

@openSUSE openSUSE deleted a comment from coveralls Jul 14, 2017

visit event_path(events(:party))
click_link 'Participants'
page.should have_content "Participants of Death Star's destruction celebration"

This comment has been minimized.

@bgeuken

bgeuken Jul 14, 2017

Member

Please also test at least one or two rows here, like are all users listed here (eg. count the rows)? and check one or two users in detail (eg. do they have correct state, role, etc.)

This comment has been minimized.

@nikhilgupta1211

This comment has been minimized.

@ChrisBr

ChrisBr Jul 25, 2017

Member

And also that the user is not able to see the row if he doesn't have the ability.

This comment has been minimized.

@nikhilgupta1211

nikhilgupta1211 Jul 25, 2017

Contributor

yes ok :)

This comment has been minimized.

@bgeuken

bgeuken Jul 26, 2017

Member

@nikhilgupta1211 The permission check is still missing, right?

This comment has been minimized.

@nikhilgupta1211

nikhilgupta1211 Jul 26, 2017

Contributor

@bgeuken sorry didn't get you

This comment has been minimized.

@bgeuken

bgeuken Jul 27, 2017

Member

The test Christian asked for. To test that a user can not see things he is not entitled to

This comment has been minimized.

@nikhilgupta1211

nikhilgupta1211 Jul 28, 2017

Contributor

@bgeuken https://github.com/openSUSE/travel-support-program/pull/80/files#diff-f20a1b372768b0b5eebe83bc6aa154f8R87 I am checking here that a tsp member is not able to see a user of a shipment request

when 'incomplete'
content_tag(:span, state, class: 'label label-warning')
when 'accepted'
content_tag(:span, state, class: 'label label-info')

This comment has been minimized.

@ChrisBr

ChrisBr Jul 18, 2017

Member

[Nitpick] I would say this should be also success

This comment has been minimized.

@ChrisBr

ChrisBr Jul 18, 2017

Member

I guess we're lacking the Reimbursable states, don't we?

@@ -13,6 +13,8 @@ class Event < ActiveRecord::Base
has_many :shipments, inverse_of: :event, dependent: :restrict_with_exception
# Mails for an event
has_many :event_emails
# Users for an event
has_many :users, through: :requests

This comment has been minimized.

@ChrisBr

ChrisBr Jul 18, 2017

Member

I think it would make sense to already include the distinct here and drop it from the view.
Something like

has_many :users, -> { distinct }, through: :requests

(untested)

This comment has been minimized.

@nikhilgupta1211

nikhilgupta1211 Jul 18, 2017

Contributor

yes ok

%th State
%th Role
%th Change role to event organizer
- @event.users.includes(:profile).distinct.each.with_index(1) do |user,index|

This comment has been minimized.

@ChrisBr

ChrisBr Jul 18, 2017

Member

I would also move this to the controler

controller#participants

@users = @event.users.includes(:profile)

view

@users.each.with_index(1) ...
@coveralls

This comment has been minimized.

coveralls commented Jul 19, 2017

Coverage Status

Coverage increased (+0.01%) to 92.015% when pulling 318cda8 on nikhilgupta1211:evenorgview into c598ebb on openSUSE:master.

@openSUSE openSUSE deleted a comment from coveralls Jul 19, 2017

@openSUSE openSUSE deleted a comment from coveralls Jul 19, 2017

@nikhilgupta1211

This comment has been minimized.

Contributor

nikhilgupta1211 commented Jul 19, 2017

image
@ChrisBr @bgeuken I have updated the PR please have a look at it :)

@coveralls

This comment has been minimized.

coveralls commented Jul 19, 2017

Coverage Status

Coverage increased (+0.02%) to 92.038% when pulling d0eb0c5 on nikhilgupta1211:evenorgview into 158b441 on openSUSE:master.

@bgeuken

This comment has been minimized.

Member

bgeuken commented Jul 19, 2017

LGTM

@bgeuken

This comment has been minimized.

Member

bgeuken commented Jul 20, 2017

UI looks good to me as well

@nikhilgupta1211 nikhilgupta1211 referenced this pull request Jul 21, 2017

Merged

Event Organizer #83

{ label: @event.name, url: event_path(@event) },
{ label: 'participants' }
]
@users = @event.users

This comment has been minimized.

@ChrisBr

ChrisBr Jul 21, 2017

Member

Can you test please that it only returns users of requests where the user has abilities

This comment has been minimized.

@nikhilgupta1211

nikhilgupta1211 Jul 24, 2017

Contributor

yes ok

@coveralls

This comment has been minimized.

coveralls commented Jul 24, 2017

Coverage Status

Coverage increased (+0.02%) to 92.043% when pulling a86e15c on nikhilgupta1211:evenorgview into c6bb8b1 on openSUSE:master.

# Users for an event
has_many :users, through: :requests
has_many :users, -> { distinct }, through: :travel_sponsorships

This comment has been minimized.

@bgeuken

bgeuken Jul 24, 2017

Member

I think it would be nicer here to keep the "through: :requests" and add a scope for the travel sponsorship.

That's more flexible and allows us to query for other request types in the future.

This comment has been minimized.

@bgeuken

bgeuken Jul 24, 2017

Member

And move the destinct to the scope as well.

This comment has been minimized.

@nikhilgupta1211

nikhilgupta1211 Jul 25, 2017

Contributor

sorry but can you please tell how to define a scope for the requests in the has_many : users statement

This comment has been minimized.

@nikhilgupta1211

nikhilgupta1211 Jul 25, 2017

Contributor

I have made some changes can you please tell if they are ok

has_many :users, -> { distinct }, through: :requests do
# Users with travel requests
def travel
where("requests.type == 'TravelSponsorship'")

This comment has been minimized.

@ChrisBr

ChrisBr Jul 25, 2017

Member

Please use the ruby hash syntax instead!

This comment has been minimized.

@ChrisBr

ChrisBr Jul 25, 2017

Member

Anyway, not sure if this is what @bgeuken meant ...

visit event_path(events(:party))
click_link 'Participants'
page.should have_content "Participants of Death Star's destruction celebration"

This comment has been minimized.

@ChrisBr

ChrisBr Jul 25, 2017

Member

And also that the user is not able to see the row if he doesn't have the ability.

{ label: @event.name, url: event_path(@event) },
{ label: 'participants' }
]
@users = @event.users.travel

This comment has been minimized.

@ChrisBr

ChrisBr Jul 25, 2017

Member

You're still not using accessible_by(current_ability), this can cause some privacy issues if you access data without checking the ability.

This comment has been minimized.

@nikhilgupta1211

nikhilgupta1211 Jul 25, 2017

Contributor

Actually, accessible_by checks if the current ability can fetch that record and if we use it here https://github.com/openSUSE/travel-support-program/pull/80/files#diff-3a4e95c1bc41d3110d2f9ac10064e79bR13 it will not be able to fetch any records because tsp members have no extra permission to view a User record.

A tsp member can only read the User Profile of a user https://github.com/openSUSE/travel-support-program/blob/master/app/models/ability.rb#L137

So we can define, can :read, Request, type: "TravelSponsorship" in the tsp role and then fetch the requests using accessible_by and then fetch the users from it, but I think it is better to use users only with the requests type as TravelSponsorship as a tsp member can view all these requests.

This comment has been minimized.

@ChrisBr

ChrisBr Jul 25, 2017

Member

I don't think that this is a good idea as it would move the ability logic out of the ability file ...

This comment has been minimized.

@nikhilgupta1211

nikhilgupta1211 Jul 25, 2017

Contributor

But we are just using the travel request scope here so why it is moving out the ability logic ?

This comment has been minimized.

@ChrisBr

ChrisBr Jul 31, 2017

Member

Would it be possible to change the association to user_profile and use the user profile here instead? I think that would make it way more clear!

has_many :users, -> { distinct }, through: :requests do
# Users with travel requests
def travel
where('type = ?', 'TravelSponsorship')

This comment has been minimized.

@ChrisBr

ChrisBr Jul 25, 2017

Member

Please use ruby hash syntax

This comment has been minimized.

@nikhilgupta1211

nikhilgupta1211 Jul 25, 2017

Contributor

I tried using the ruby hash syntax but it is not working

This comment has been minimized.

@ChrisBr

ChrisBr Jul 26, 2017

Member

What did you try? Sth like this should work:

where(requests: { type: 'TravelSponsorship' } )

This comment has been minimized.

@nikhilgupta1211

nikhilgupta1211 Jul 26, 2017

Contributor

Sorry actually I tried this where(type: 'TravelSponsorship') .

Thanks :)

def travel
where(requests: { type: 'TravelSponsorship' } )
end
end

This comment has been minimized.

@bgeuken
@coveralls

This comment has been minimized.

coveralls commented Jul 26, 2017

Coverage Status

Coverage increased (+0.02%) to 92.048% when pulling 0e6ed8f on nikhilgupta1211:evenorgview into c6bb8b1 on openSUSE:master.

@openSUSE openSUSE deleted a comment from coveralls Jul 27, 2017

@openSUSE openSUSE deleted a comment from coveralls Jul 27, 2017

@openSUSE openSUSE deleted a comment from coveralls Jul 27, 2017

@openSUSE openSUSE deleted a comment from coveralls Jul 27, 2017

page.should have_content "Participants of Death Star's destruction celebration"
page.all('.table tr').count.should == 7
page.should_not have_content 'john.skywalker@rebel-alliance.org' # User with a shipment request

This comment has been minimized.

@nikhilgupta1211

nikhilgupta1211 Jul 27, 2017

Contributor

@bgeuken I have added a shipment request and defined this so that a tsp member is not able to see the user of a shipment request.

@coveralls

This comment has been minimized.

coveralls commented Jul 27, 2017

Coverage Status

Coverage increased (+0.03%) to 92.033% when pulling 5fe55ec on nikhilgupta1211:evenorgview into c3e49e5 on openSUSE:master.

@coveralls

This comment has been minimized.

coveralls commented Jul 27, 2017

Coverage Status

Coverage increased (+0.03%) to 92.535% when pulling 8281a24 on nikhilgupta1211:evenorgview into 25bd8e6 on openSUSE:master.

@coveralls

This comment has been minimized.

coveralls commented Jul 27, 2017

Coverage Status

Coverage increased (+0.03%) to 92.535% when pulling f50dbf6 on nikhilgupta1211:evenorgview into 25bd8e6 on openSUSE:master.

{ label: @event.name, url: event_path(@event) },
{ label: 'participants' }
]
@users = @event.users.travel

This comment has been minimized.

@ChrisBr

ChrisBr Jul 31, 2017

Member

Would it be possible to change the association to user_profile and use the user profile here instead? I think that would make it way more clear!

# Users for an event
has_many :users, -> { distinct }, through: :requests do
# Users with travel requests
def travel

This comment has been minimized.

@ChrisBr

ChrisBr Jul 31, 2017

Member

This scope belongs to requests association I would say

@coveralls

This comment has been minimized.

coveralls commented Aug 1, 2017

Coverage Status

Coverage increased (+0.02%) to 92.535% when pulling 935fd72 on nikhilgupta1211:evenorgview into bc75e22 on openSUSE:master.

@ChrisBr

Otherwise lgtm

@@ -13,6 +13,8 @@
= link_to t(:new_travel), new_travel_sponsorship_path(:event_id => resource), :class => 'btn btn-primary'
- if resource.accepting_shipments?
= link_to t(:new_shipment), new_shipment_path(:event_id => resource), :class => 'btn btn-primary'
- if user_signed_in? && can?(:participants, resource)

This comment has been minimized.

@ChrisBr

ChrisBr Aug 2, 2017

Member

Is this ability already defined?

@ChrisBr

ChrisBr approved these changes Aug 3, 2017

LGTM!
Please rebase before I can merge

nikhilgupta1211 added some commits Jul 11, 2017

Added a route event/:id/participants to view event participants
Also added the permissions for tsp member and a link to /participant in the events#show view

Created a view for event#participants
@nikhilgupta1211

This comment has been minimized.

Contributor

nikhilgupta1211 commented Aug 3, 2017

@ChrisBr I have rebased it :)

@coveralls

This comment has been minimized.

coveralls commented Aug 3, 2017

Coverage Status

Coverage increased (+0.02%) to 92.535% when pulling 96f55f2 on nikhilgupta1211:evenorgview into b626c5d on openSUSE:master.

@bgeuken

This comment has been minimized.

Member

bgeuken commented Aug 3, 2017

@nikhilgupta1211 The first 2 commits are also in #90, right?

@nikhilgupta1211

This comment has been minimized.

Contributor

nikhilgupta1211 commented Aug 3, 2017

@bgeuken Yes that branch has to be rebased after we merge this :)

@bgeuken

bgeuken approved these changes Aug 3, 2017

@bgeuken bgeuken merged commit b54f769 into openSUSE:master Aug 3, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 92.535%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment