tpi/not active companies #216
Conversation
app/views/tpi/search/index.html.erb
Outdated
| @@ -21,7 +21,8 @@ | |||
| <h3>Companies</h3> | |||
| <ul> | |||
| <% @companies.each do |company| %> | |||
| <li><%= link_to company.name, tpi_company_path(company) %></li> | |||
| <% company_name = company.is_active ? company.name : "#{company.name} (Not active)" %> | |||
| <li><%= link_to company_name, tpi_company_path(company) %></li> | |||
There was a problem hiding this comment.
I’m not sure on which pages to add the inscription "not active" to the name of the companies.
Perhaps it is necessary to create the “name” method in the “company” model so that the inscription appears on all pages?
There was a problem hiding this comment.
👍 maybe move it to view helpers (or decorators for admin pages)?
There was a problem hiding this comment.
We may need decorators/presenters layer for public interface too, but have to be distinguished from the admin one somehow.
| @@ -71,7 +71,7 @@ def user_download_all | |||
| end | |||
|
|
|||
| def user_download | |||
| companies_ids = @sector.companies.published.select(:id) | |||
| companies_ids = @sector.companies.published.active.select(:id) | |||
There was a problem hiding this comment.
Just curious - are we sure that scopes published and active are independent of each other? Or maybe inactive companies should be always unpublished?
If those are unrelated we could at least add another scope published_active as it looks those 2 go together at least in this controller.
There was a problem hiding this comment.
Active flag is different from the publishable status. So you can have published && inactive companies, and they should still show up on the frontend, just not on all the charts, because an inactive company is a company that is not getting new assessments, but has historical ones. But we could add an extra scope for when we want both published and active if that makes it more clear.
| @@ -112,7 +113,7 @@ def companies_scope(params) | |||
| TPISector.friendly.find(params[:id]).companies.published | |||
| else | |||
| Company.published | |||
| end | |||
| end.active | |||
There was a problem hiding this comment.
This look a bit strange and can be easily overlooked during future changes. If we add published_active scope (see prev comment) we won't need that here :)
app/models/company.rb
Outdated
| @@ -51,6 +51,8 @@ class Company < ApplicationRecord | |||
| validates_presence_of :name, :slug, :isin, :market_cap_group | |||
| validates_uniqueness_of :slug, :name | |||
|
|
|||
| scope :active, -> { where(is_active: true) } | |||
There was a problem hiding this comment.
can you also update company factory?
| @@ -0,0 +1,5 @@ | |||
| class AddIsActiveToCompany < ActiveRecord::Migration[6.0] | |||
| def change | |||
| add_column :companies, :is_active, :boolean, default: true | |||
There was a problem hiding this comment.
I would vote for not using is_ as a prefix for boolean columns. Just for the sake of consistency with already existing boolean columns in the database. What do you guys think about it?
app/models/company.rb
Outdated
| @@ -51,6 +52,9 @@ class Company < ApplicationRecord | |||
| validates_presence_of :name, :slug, :isin, :market_cap_group | |||
| validates_uniqueness_of :slug, :name | |||
|
|
|||
| scope :active, -> { where(active: true) } | |||
| scope :published_active, -> { where(active: true, visibility_status: 'published') } | |||
There was a problem hiding this comment.
We don't really need that scope. We can write Company.published.active
There was a problem hiding this comment.
I see that my comment goes against @kowal ;]. But I'm not sure what's the point of having extra scopes if we can combine scopes.
There was a problem hiding this comment.
I'd keep it simpler and separate scopes as well.
|
Gonna think of all the places, charts/exporters/importers where the |
db/schema.rb
Outdated
| @@ -247,6 +248,9 @@ | |||
| create_table "images", force: :cascade do |t| | |||
| t.string "link" | |||
| t.bigint "content_id", null: false | |||
| t.string "name" | |||
There was a problem hiding this comment.
No idea! Those changes are already on develop. WTF
There was a problem hiding this comment.
Here is the list of outstanding issues
- Admin CSV Importer should also update
activestate, when no value is present then default value is true. - Inactive companies not listed in the dropdown as inactive ones. They are gone
- inactive state not listed on company show page
- All sectors CP Performance chart data should only consider active companies (right @simaob ?)
|
@batraisk in case you missed it, Tomasz added a list of outstanding issues here. |
db/schema.rb
Outdated
| @@ -115,6 +116,7 @@ | |||
| t.datetime "created_at", precision: 6, null: false | |||
| t.datetime "updated_at", precision: 6, null: false | |||
| t.string "content_type" | |||
| t.string "youtube_link" | |||
There was a problem hiding this comment.
some remnants from other branch ;)
|
@simaob one more thing. Should inactive companies be in |
|
Might be best to ask on Basecamp. But if the name says inactive, it could show in the historical files. I think they become inactive when they "drop" them from the most recent assessments |
|
Ok. Let's just leave them there and then we can ask for feedback. |
|
Alright, are the other things sorted? 💃 |
That's last one. |
|
@batraisk is already working on it. |
No description provided.