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

Views methods #41

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

Conversation

katafira
Copy link
Collaborator

Purpose

Finished adding methods for the dashboard views. They allow the admin to see:

  1. The number of registered users minus the admins
  2. The number of registered offspring
  3. The number of registered users with no offspring
  4. The number of registered users with two or more offspring

@katafira @agarciavera @sebjimenez @sergio-ocon @delaluzparra

@@ -13,13 +13,11 @@
//= require jquery
//= require jquery_ujs
//= require turbolinks

//
//= require_tree .
Copy link
Member

Choose a reason for hiding this comment

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

Still not needed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If I remove those lines linking between administration tabs will not work. Also, I do use datatables, that's why it is there.

@katafira
Copy link
Collaborator Author

katafira commented Aug 24, 2016

I think that this is ready to merge, please review @alexvkcr @agarciavera @sebjimenez @sergio-ocon @delaluzparra

@katafira
Copy link
Collaborator Author

katafira commented Aug 24, 2016

@sergio-ocon Please check my proposal for the view using datatables in the "Horarios" view in the main navbar menu. It is responsive and columns expand depending on contens, so empty days columns are not as wide as days that have class.
The main problem with your version is that it is not responsive, therefore upon zooming, for example, the tables go crazy.
output_bebo4d

def user_0_offspr_count
count = 0
User.all.each do |u|
count += 1 if u.no_offspring?
Copy link
Member

Choose a reason for hiding this comment

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

u.offsprings.empty?

Copy link
Member

Choose a reason for hiding this comment

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

Creating a joint view and then using
User.select(:offspring_id).distinct.count
will be more efficient

Copy link
Member

@chargio chargio Aug 24, 2016

Choose a reason for hiding this comment

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

User.count - User.joins(:offsprings).uniq.count
gives you the number of Users without offspring in 0.7 ms

@katafira katafira changed the title Views methods #WIP Views methods Aug 25, 2016
@katafira
Copy link
Collaborator Author

@alexvkcr @agarciavera @sebjimenez @sergio-ocon @delaluzparra
This PR will remain as a #WIP until @alexvkcr 's PR #14 is merged. *Reason: there are methods in admin_controller that should be in the admin model, but for now it does not exist. Said PR is solving that issue. *

@katafira katafira force-pushed the views_methods branch 2 times, most recently from 439992e to 0ff95d7 Compare August 29, 2016 09:44
@katafira katafira changed the title #WIP Views methods Views methods Sep 1, 2016
@katafira
Copy link
Collaborator Author

katafira commented Sep 1, 2016

@alexvkcr @agarciavera @sebjimenez @sergio-ocon Please review asap.

@alexvkcr
Copy link
Collaborator

alexvkcr commented Sep 1, 2016

LGTM @sergio-ocon
but should there be specs for the methods included here?
If so, should they be included in this PR or a new one?

@chargio
Copy link
Member

chargio commented Sep 1, 2016

Every PR needs to have tests

@@ -17,4 +17,12 @@ class User < ActiveRecord::Base
validates :phone, format: { with: VALID_TELEPHONE_REGEX}
# The offspring need to have a parent, they will be destroyed if the parent is
has_many :offsprings, dependent: :destroy

def no_offspring?
offsprings.empty? || offsprings.count.zero?
Copy link
Member

Choose a reason for hiding this comment

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

?

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.

3 participants