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

[Feature] Statistics page #5464

Merged
merged 2 commits into from Jan 24, 2015

Conversation

@SansPseudoFix
Copy link
Contributor

commented Dec 8, 2014

We create a more readable page for stats. At the moment, it's basic. But we prefer be sure to do it well.
I dont' know for others pods, but framasphere's users like check stats :)

statistics_preview

@SansPseudoFix SansPseudoFix force-pushed the SansPseudoFix:statistics_design branch 2 times, most recently from 3aa6510 to a7eaa24 Dec 8, 2014

end

def datas
@@result = {

This comment has been minimized.

Copy link
@jhass

jhass Dec 8, 2014

Member

Please don't use class variables, also data is an uncountable noun, there's zero need for a sideffet method here and I think you should just use a hash or even use the existing presenter, so @data = StatisticsPresenter.new

@jhass

This comment has been minimized.

Copy link
Member

commented Dec 8, 2014

Please use Bootstrap.

Also I think that's a bit plain, how about some boxes:

+-------------------------------+
|        Statistic name         |
|                               |
+-------------------------------+
|                               |
|                               |
|            123.123            |
|                               |
|                               |
|                               |
+-------------------------------+

in a fluid grid?

@@ -47,6 +47,7 @@ en:
_contacts: "Contacts"
welcome: "Welcome!"
_terms: "terms"
_statistics: "Statistics :"

This comment has been minimized.

This comment has been minimized.

Copy link
@SansPseudoFix

SansPseudoFix Dec 8, 2014

Author Contributor

Hm, yeah, french syntax, sorry :P

@@ -1365,3 +1366,14 @@ en:
default: "The secret code did not match with the image"
user: "The secret image and code were different"
failed: "Human verification failed"

statistics:
name: "Name: "

This comment has been minimized.

Copy link
@jhass

jhass Dec 8, 2014

Member

Avoid trailing whitespace, either add it in the view if you have to, or, preferably interpolate the value: t('.foo', something: 123) -> something: "Something: %{something}"

@jhass

This comment has been minimized.

Copy link
Member

commented Dec 8, 2014

I think a simple controller test would be nice that just checks that the response was a success, so we notice that we break the page by for example changing the presenter. spec/controllers should have plenty of examples to follow along.

@SansPseudoFix

This comment has been minimized.

Copy link
Contributor Author

commented Dec 8, 2014

Please use Bootstrap.

Also I think that's a bit plain, how about some boxes:

Yeah, sure. This is really wip. I wanted to use boxes too.

And for statistics_controller.rb, I will talk about with @augier.

respond_to :json


respond_to :html

This comment has been minimized.

Copy link
@jaywink

jaywink Dec 8, 2014

Contributor

This needs to respond to :json too, as it did before, otherwise that will break a few pod lists ;)

This comment has been minimized.

Copy link
@SansPseudoFix

SansPseudoFix Dec 8, 2014

Author Contributor

Oh, ok. Thx :)

@SansPseudoFix SansPseudoFix force-pushed the SansPseudoFix:statistics_design branch from a7eaa24 to b60577c Dec 10, 2014

@SansPseudoFix

This comment has been minimized.

Copy link
Contributor Author

commented Dec 10, 2014

Still WIP :)
wip_statistics

%h4
= t('statistics.registrations')
%br
= @as_registrations

This comment has been minimized.

Copy link
@ghost

ghost Dec 10, 2014

I think there's a way to loop here. TODO.

This comment has been minimized.

Copy link
@jhass

jhass Dec 10, 2014

Member

If you use the presenter you could probably utilize its as_json method which returns a hash.

This comment has been minimized.

Copy link
@ghost

ghost Dec 10, 2014

We can do that indeed.

.span-3
%h4
= t('statistics.active_users')
%br

This comment has been minimized.

Copy link
@jhass

jhass Dec 10, 2014

Member

Can you try to to avoid using br for styling?

This comment has been minimized.

Copy link
@ghost

ghost Dec 10, 2014

Yeah, this part can be refarcored anyway.

This comment has been minimized.

Copy link
@SansPseudoFix

SansPseudoFix Dec 10, 2014

Author Contributor

Yep, I can :)

.page-statistics h4{
margin: 0px;
padding: 10px 20px;
background-color: #E6EFFB;

This comment has been minimized.

Copy link
@jhass

jhass Dec 10, 2014

Member

Maybe add it to the color variables if no existing one seems fitting.

@SansPseudoFix

This comment has been minimized.

Copy link
Contributor Author

commented Dec 10, 2014

I don't know why I used a special grey for background, so now, I use $background-grey + smaller boxes:

wip_statistics2

@jhass

This comment has been minimized.

Copy link
Member

commented Dec 10, 2014

I think overall larger font sizes would look good. true/false is technical, let's translate it to Registrations -> Open/Closed

border-radius: 5px;
}

.page-statistics .span-3 .data{ margin-top: 15px; }

This comment has been minimized.

Copy link
@jhass

jhass Dec 10, 2014

Member

Actually why not apply some of SCSS features, most notably nesting:

.page-statistics {
  h1 {
  }

  .span-3 {
    .data {
    }
  }
}

This comment has been minimized.

Copy link
@SansPseudoFix

SansPseudoFix Dec 10, 2014

Author Contributor

Argh! It's just because I don't used to use framework. Thanks :)

@Flaburgan

This comment has been minimized.

Copy link
Member

commented Dec 10, 2014

Even if the route is "statistics", those informations are more or less the characteristics of the pods (registration, services enabled (you're missing them btw), version...). Maybe we could present it a little bit differently? With another more global title for example. And then, we could add a link to that page on diasporafoundation.org / poduti.me to help users choose their pod.

@SansPseudoFix

This comment has been minimized.

Copy link
Contributor Author

commented Dec 10, 2014

I think overall larger font sizes would look good.

Like this?
wip_statistics3

@jhass

This comment has been minimized.

Copy link
Member

commented Dec 10, 2014

Yeah, maybe even larger and maybe larger for the values.

posts: "Local posts"
comments: "Local comments"
version: "Version"
registrations: "Registrations opened"

This comment has been minimized.

Copy link
@jaywink

jaywink Dec 10, 2014

Contributor

Maybe just Registrations - if the result is Open/Closed

Also, some of the lines are all capital letters, some only first capital letter. The latter I think should be correct, so just first word has capital letter :)

@SansPseudoFix

This comment has been minimized.

Copy link
Contributor Author

commented Dec 10, 2014

(sorry for multi push, but we are two on this PR, and me and git...)
So, titles are now h3, so 24.5px (h2 is really big) and values are 25px.

wip_statistics4

@AugierLe42e it's up to you now :P

@ghost

This comment has been minimized.

Copy link

commented Dec 10, 2014

Looks ok to me. I have to find an elegant way to translate "true" values to "open".
I think I will eventually setup a local pod on my shitty netbook. Let's suffer !


result
end

This comment has been minimized.

Copy link
@ghost

ghost Dec 12, 2014

@jhass : I find this part a bit over-engineered. I have the feeling that there's a better way to do that, especially deal with translations but this is the least ugly way I found.

@jhass

This comment has been minimized.

Copy link
Member

commented Dec 12, 2014

Please make sure to use two spaces for indentation, no tabs. Also please try to avoid using the datastructure of things in in their names, that generally leads to bad names.

I guess it's time to refactor the presenter a bit, something like (completely untested):

class StatisticsPresenter

  def as_json options={}
    base_data.merge(user_counts)
             .merge(post_counts)
             .merge(comment_counts)
             .merge(services)
             .merge(legacy_services) # Remove in 0.6
  end

  def base_data
    {
      'name' => name,
      'network' => 'Diaspora',
      'version' => version,
      'registrations_open' => open_registrations?,
      'services' => available_services
    }
  end

  def name
    AppConfig.settings.pod_name
  end

  def version
    AppConfig.version_string
  end

  def open_registrations?
    AppConfig.settings.enable_registrations
  end

  def user_counts
    return {} unless expose_user_counts?
    {
      'total_users' => total_users,
      'active_users_monthly' => monthly_users,
      'active_users_halfyear' => halfyear_users
    }
  end

  def expose_user_counts?
    AppConfig.privacy.statistics.user_counts?
  end

  def total_users
    @total_users ||= User.joins(:person)
                         .where(people: {closed_account: false})
                         .where.not(username: nil)
  end

  def monthly_users
    @monthly_users ||= User.halfyear_actives.count
  end

  def halfyear_users
    @halfyear_users ||= User.monthly_actives.count
  end

  def post_counts
    return {} unless expose_posts_counts?
    {
      'local_posts' => local_posts
    }
  end

  def local_posts
    @local_posts ||= Post.where(type: "StatusMessage")
                         .joins(:author)
                         .where("owner_id IS NOT null")
                         .count
  end

  def expose_posts_counts?
    AppConfig.privacy.statistics.post_counts?
  end

  def comment_counts
    return {} unless expose_comment_counts?
    {
      'local_comments' => local_comments
    }
  end

  def expose_comment_counts?
    AppConfig.privacy.statistics.comment_counts?
  end


  def local_comments
    @local_comments ||= Comment.joins(:author)
                               .where("owner_id IS NOT null")
                               .count
  end

  def available_services
    Configuration::KNOWN_SERVICES.select {|service|
      AppConfig["services.#{service}.enable"]
    }.map(&:to_s)
  end

  def legacy_services
    Configuration::KNOWN_SERVICES.each_with_object({}) {|service, result|
      result[service.to_s] = AppConfig["services.#{service}.enable"]
    }
  end
end

Then a controller like

class StatisticsController < ApplicationController
  respond_to :html, :json
  use_bootstrap_for :statistics

  def statistics
   @statistics = StatisticsPresenter.new
   respond_to do |format|
    format.json { render json: @statistics }
    format.html { render layout: "application" }
   end
  end
end

With a app/views/statistics/statistics.html.haml like

= render 'statistics/statistic', locals: {name: t('.name'), value: @statistics.name}
= render 'statistics/statistic', locals: {name: t('.version'), value: @statistics.version}
= render 'statistics/statistic', locals: {name: t('.registrations'), value: registrations_status(@statistics)}
- if @statistics.expose_user_counts?
  = render 'statistics/statistic', locals {name: t('.total_users'), value: @statistics.total_users}
// and so on

and a app/views/statistics/_statistic.haml like

.span-3
  %h3
   = name
  .data
    = value

and a app/helpers/statistics_helper.rb like

module StatisticsHelper
  def registrations_status statistics
    if statistics.open_registrations?
      I18n.t('statistics.registrations.enabled')
    else
      I18n.t('statistics.registrations.disabled')
    end
  end
end

I don't think it hurts that much to repeat listing the available keys in the view, since the representation between the JSON and the HTML can differ quite a lot.

@ghost

This comment has been minimized.

Copy link

commented Dec 12, 2014

Thanks for the advice and having taken time to improve this.

@Flaburgan

This comment has been minimized.

Copy link
Member

commented Dec 15, 2014

.merge(legacy_services) # Remove in 0.6

This PR will be run only by pod with 0.5 or more so we don't need to add compatibility with the 0.4.1 format do we?

@jhass

This comment has been minimized.

Copy link
Member

commented Dec 15, 2014

We decided in #5296 to keep the old format in 0.5, you even voted in favor of that. See also changelog.

@ghost

This comment has been minimized.

Copy link

commented Dec 16, 2014

I see render 'statistics/statistic' with a file named _statistic.haml so I suppose partial haml files named with a starting _ and ending with only .haml is a convention ?

@jhass

This comment has been minimized.

Copy link
Member

commented Dec 16, 2014

The convention is _name.extension, it's a Rails thing not a HAML thing. But yes.

@SansPseudoFix

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2015

Well, what about display green header when service is available and grey if not ?

No service available :
stat_design_final

Twitter available :
stat_design_final_enabled

I'll clean up commits if you're okay with this proposition :)

@jhass

This comment has been minimized.

Copy link
Member

commented Jan 16, 2015

Then the heading shouldn't say "Available services", but just "Services" and the values should be "Available" / "Not available" or "Unavailable".

@ghost

This comment has been minimized.

Copy link

commented Jan 16, 2015

I find "Available" and "Not available" not explicit.

@SansPseudoFix

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2015

@AugierLe42e ah ! You see ? ;)

I think "available" is more explicite than "enabled" too.

Then the heading shouldn't say "Available services", but just "Services"

True.

@goobertron

This comment has been minimized.

Copy link

commented Jan 16, 2015

This is a stats page for podmins, right? If so, I agree that

the heading shouldn't say "Available services", but just "Services"

but think the values should be 'Enabled' / 'Not enabled' (rather than 'Available' / 'Not available') as all these services are available to the podmin if the podmin enables them. (They'd be unavailable to users of the pod if the podmin hasn't enabled them, but to say 'Not available' to a podmin suggest that the service can't be connected, so I think 'Not enabled' is better.)

@Flaburgan

This comment has been minimized.

Copy link
Member

commented Jan 16, 2015

I wouldn't say this is specifically for podmins. Podmins have their own stats in the admin panel.

@goobertron

This comment has been minimized.

Copy link

commented Jan 16, 2015

Ah, I thought this was a new design for stats in the admin panel. So this is a public view for the JSON stats, then? In which case, 'Available' / 'Not available' would be appropriate, I agree.

@jhass

This comment has been minimized.

Copy link
Member

commented Jan 16, 2015

Yes, it's a public HTML representation for /statistics.json basically.

@ghost ghost force-pushed the SansPseudoFix:statistics_design branch from f9a537f to e4e1bea Jan 18, 2015

@SansPseudoFix

This comment has been minimized.

Copy link
Contributor Author

commented Jan 19, 2015

@AugierLe42e cleaned up the code (thx mate).
So, what about compromise : all boxes visibles but colors if service available (or not) ?

@ghost

This comment has been minimized.

Copy link

commented Jan 19, 2015

@SansPseudoFix : ya welcome buddy !

%h1
= t('statistics.services')
- Configuration::KNOWN_SERVICES.each do |service|
= render 'statistics/statistic', name: "#{service.capitalize}", value: service_status(service, @statistics.available_services), activated: service_class(service, @statistics.available_services)

This comment has been minimized.

Copy link
@jhass

jhass Jan 19, 2015

Member

Did you get an error when requesting the mobile format, or why do you have an exact duplicate here?

If so, instead of duplicating the template, try rendering the same one for both formats:

respond_to do |format|
  format.json { }
  format.any(:html, :mobile) { render 'statistics/statistics.html.haml' }
end

This comment has been minimized.

Copy link
@ghost

ghost Jan 19, 2015

Did you get an error when requesting the mobile format, or why do you have an exact duplicate here?

No, just we didn't know how to do that.

This comment has been minimized.

Copy link
@jhass

jhass Jan 19, 2015

Member

I did neither, just googled it ;)

@ghost

This comment has been minimized.

Copy link

commented Jan 24, 2015

@jhass : Fixed, you can merge.

@jhass jhass added this to the next-major milestone Jan 24, 2015

@jhass

This comment has been minimized.

Copy link
Member

commented Jan 24, 2015

Thanks!

@jhass jhass merged commit dd9c2f2 into diaspora:develop Jan 24, 2015

1 check failed

continuous-integration/travis-ci The Travis CI build failed
Details
jhass added a commit that referenced this pull request Jan 24, 2015
@SansPseudoFix

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2015

Yeah \o/

@jaywink

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2015

When logged in on my local it shows like this:

selection_047

Anyone else? When logged out works fine..

@SansPseudoFix

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2015

Argh... Last time I tried, no. I'll try today.

@SansPseudoFix

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2015

Confirmed. This is format.any(:html, :mobile) { render 'statistics/statistics.html.haml' } the problem.

@ghost

This comment has been minimized.

Copy link

commented Jan 25, 2015

Looks like a problem with bootstrap and the rendering line.

@Flaburgan

This comment has been minimized.

Copy link
Member

commented Jan 26, 2015

How the hell do you guys success to mix git that way? This page now shows only two files modified when https://github.com/diaspora/diaspora/pull/5587/files shows a lot...

I'm sorry, but it looks like you break stuff, including the statistics.json route which now invert the monthly and the 6 monthly active users... see https://diaspora-fr.org/statistics.json for example.

@jaywink

This comment has been minimized.

Copy link
Contributor

commented Jan 26, 2015

Don't sweat @Flaburgan - mistakes happen to everyone, and part of the blame is the people who review the PR's. This is not a critical issue, but of course needs to be fixed, issue here: #5589

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.