Skip to content

Show and mange the statistics at the home page.#552

Merged
lastpotion merged 8 commits intomasterfrom
update/home/statistics
Jan 20, 2017
Merged

Show and mange the statistics at the home page.#552
lastpotion merged 8 commits intomasterfrom
update/home/statistics

Conversation

@lastpotion
Copy link
Copy Markdown
Contributor

@lastpotion lastpotion commented Jan 20, 2017

🎩 What? Why?

This PR adds to the homepage of Decidim some stats about it's current usage.

At the moment we only can show users and processes, because the other engines are not connected ( I guess it's a bad idea to create dependency between them ).

Also I added an option on the admin panel to show or not them at the home page.

📌 Related Issues

📋 Subtasks

  • Show stats as the desing require.
  • Create the migration and add to controller the respective fields.
  • Basic control of the show behavior of stats on the admin panel.
  • Test it.

📷 Screenshots (optional)

Home
Admin

👻 GIF

@lastpotion lastpotion added the wip label Jan 20, 2017
@lastpotion lastpotion self-assigned this Jan 20, 2017
</div>

<div class="field">
<%= form.check_box :show_statistics, label: t(".questions.show_statistics") %>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You might not need the label part here. If you need it, then add the missing locale

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think it's needed, let's use the conventions found on other fields.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Jan 20, 2017

Current coverage is 97.17% (diff: 100%)

Merging #552 into master will decrease coverage by 0.03%

@@             master       #552   diff @@
==========================================
  Files           257        257          
  Lines          4007       4031    +24   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           3895       3917    +22   
- Misses          112        114     +2   
  Partials          0          0          

Powered by Codecov. Last update a1c6791...994f53f

@lastpotion lastpotion removed the wip label Jan 20, 2017
@lastpotion lastpotion changed the title Update/home/statistics Show and mange the statistics at the home page. Jan 20, 2017
Copy link
Copy Markdown
Contributor

@oriolgual oriolgual left a comment

Choose a reason for hiding this comment

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

Add show_statistics to the organization factory too

@@ -0,0 +1,5 @@
class AddShowStatisticsToOrganization < ActiveRecord::Migration[5.0]
def change
add_column :decidim_organizations, :show_statistics, :boolean, default: false
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd set it as true as default

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@oriolgual don't quite like setting default values on the database. Looks like something that should be done at the model, as well as in the Form. When creating new models, Rails isn't able to inspect the table for default values, so the checkbox will show unchecked.

Copy link
Copy Markdown
Contributor

@josepjaume josepjaume left a comment

Choose a reason for hiding this comment

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

Real nice work 💅

Just as an observation: Statistics look like something we might leverage a presenter for, as well as most of the stuff present on the home page. Otherwise we'll keep cluttering the HomepageController.

No need to do it right now, just some food for thought.

def users
@users ||= Decidim::User.where(organization: current_organization)
end

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Extra new line 🚎

it "should have the correct values for the statistics" do
within ".users-count" do
expect(page).to have_content("4")
end
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

OriolCI: Add new line

}

context "when organization show_statistics attribute is false" do
let(:organization) { create(:organization, show_statistics: false) }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

OriolCI: Add new line

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.

5 participants