Cleaning up Admin::DashboardController #313
Conversation
|
the |
CGA1123
changed the title
Admin::DashboardController
CGA1123
changed the title
Admin::DashboardController
|
Looks related to #304. Thanks for the cleanup! |
| @@ -46,12 +31,28 @@ def stats | |||
| @responded_topics = Topic.where(id: responded_topic_ids) | |||
| @closed_topic_count = @topics.closed.count | |||
|
|
|||
| @posts = Post.where('created_at >= ? AND created_at <= ?', @start_date, @end_date) | |||
| @posts = Post.where(created_at: @start_date..@end_date) | |||
There was a problem hiding this comment.
Nice, didn't know you could write it that way
Allowed certain html attributes to be whitelisted by the sanitize method, so that translations can use some formatting.
Improve readability imo
|
For moving to code from I don't know whether this is the best solution though. |
|
Also, I'm not sure that the calculation for delays works properly, what happens if the second post on a topic is not from an agent?
|
|
I think it would be ideal to move code from As for the delays, you may be right. Perhaps adding a scope on |
|
To keep consistent, would some code currently in For example |
| @agents_responded_topic_count = @agents.each.map { |agent| [agent.id, @responded_topics.where(assigned_user: agent).count] }.to_h | ||
| @agents_closed_topic_count = @agents.each.map { |agent| [agent.id, Topic.undeleted.where(assigned_user: agent).closed.count] }.to_h | ||
| @agents_post_count = @agents.each.map { |agent| [agent.id, @posts.where(user: agent, kind: 'reply').count] }.to_h | ||
| @agents_delays = @agents.each.map { |agent| [agent.id, median(@responded_topics.where(assigned_user: agent).map { |t| t.posts.second.created_at - t.created_at })] }.to_h |
There was a problem hiding this comment.
Working on moving these assignments into a single iteration.
|
Only problem with moving some of this code into the model is the dependence on |
|
What do you think about testing this? Could you write some tests? |
|
I'm more familiar with rspec than minitest but I can give it a shot. |
|
Awesome work! I am really hoping we can add some tests to validate that the stats are showing what they should. Do you have any ideas for this? |
|
Thanks! Yeah I suppose it's pretty straightforward, just involves a lot of setting up for the tests. |
|
Can anyone point me in the right direction for testing whether the controller instance variables are being set correctly in minitest. Maybe the equivalent for |
|
You can access instance variables using Also, this has some conflicts after I merged #319 from @Rynaro which touched some of the same files. |
Conflicts: app/views/admin/dashboard/_agent_stats.html.erb app/views/admin/dashboard/stats.html.erb
|
@scott Thanks! I'll have a look and try to get those test written asap. I fixed the conflicts. |
|
@scott What do you think about changing It makes it easier to test, and I think it's a bit sleeker. |
|
@scott I'm not great with fixtures, so writing test for the DB queries made by the controller is pretty difficult for me. However, I've added tests for other functionality. |
Fixes Conflicts: Gemfile.lock app/controllers/admin/dashboard_controller.rb app/views/admin/dashboard/stats.html.erb
I noticed that some views within the project include code that would ideally be within controllers.