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.

Sign up for GitHub

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

Jump to bottom

group subscription tags with number of subscribers #5224

Merged
merged 8 commits into from Mar 25, 2019
Merged

group subscription tags with number of subscribers #5224

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
aac8f35
group tags with number of subscribers
GettyOrawo Mar 21, 2019
fbdb8f6
cleanup
GettyOrawo Mar 21, 2019
a595c57
fix codeclimate errors
GettyOrawo Mar 21, 2019
b33bef7
refactoring the subscriber count range and stats display
GettyOrawo Mar 22, 2019
83b647c
Merge branch 'master' of https://github.com/publiclab/plots2 into cla…
GettyOrawo Mar 22, 2019
e536c80
fix codeclimate failing lint test
GettyOrawo Mar 22, 2019
bf8d12f
fix indent on stats view
GettyOrawo Mar 23, 2019
8a6eab3
Merge branch 'classify-tags-with-subscriber-numbers' of https://githu…
GettyOrawo Mar 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/controllers/stats_controller.rb
View file
Open in desktop
Expand Up @@ -5,7 +5,7 @@ def subscriptions
@tags[tag.tagname] = @tags[tag.tagname] || 0
@tags[tag.tagname] += 1
end
render plain: @tags.inspect, status: 200
@tags = @tags.group_by { |_k, v| v }.map { |k, v| { k => v.map { |x| x.join("-") } } }
Copy link
Member

Choose a reason for hiding this comment

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

So let's take this for example:

> tags
=> {"boagd"=>3, "boa"=>34, "boag"=>39, "b"=>66}

see how if group by v/10 then 34 becomes 3, 66 becomes 6, so ones within 10 of each other get added to the same group?

> tags.group_by { |k, v| v/10 }
=> {0=>[["boagd", 3]], 3=>[["boa", 34], ["boag", 39]], 6=>[["b", 66]]}

I think the second map is something we can do in the template though, since it relates to how we want to display the data. So i'll suggest:

Suggested change
@tags = @tags.group_by { |_k, v| v }.map { |k, v| { k => v.map { |x| x.join("-") } } }
@tags = @tags.group_by { |k, v| v/10 }
@tags = @tags.reverse # because it's naturally sorted "ascending", so let's flip it!

Copy link
Member

Choose a reason for hiding this comment

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

nice that the group is automatically sorted, too!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes actually this works better and is way cleaner. The ruby hash doesn't have the reverse method though I think it's safe to use sort?

end

def range
Expand Down
26 changes: 25 additions & 1 deletion app/views/stats/subscriptions.html.erb
View file
Open in desktop
Expand Up @@ -4,4 +4,28 @@
</div>
<div class="col-md-8 col-md-offset-1">
<h2>Subscriptions</h2>
</div>
<table class="table inline-grid">
<tr>
Copy link
Member

Choose a reason for hiding this comment

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

Not a huge deal, but would you mind using just 2 spaces for indents? We've tried to standardize a bit on this to help keep our code tidier. Thanks, and sorry for the minute details! 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes of course. sorry I switched to a new editor that I'm still finding my way around. I'll fix that.

<thead>
<th>Subscriptions</th>
<th>Tags</th>
</thead>
</tr>
<% @tags.each do |map| %>
Copy link
Member

Choose a reason for hiding this comment

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

Here we could rename map to group, since each item in @tags represents a group of tags.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<% @tags.each do |map| %>
<% @tags.each do |range, tags| %>

We could do this because the key will represent the range of numbers, and the value will contain the tags themselves, for example ['tagname', 46].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh alright. so I'll do away with joining tags to the count with hyphens (-)

<% map.each do |subscriptions,tags| %>
Copy link
Member

Choose a reason for hiding this comment

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

Here we wouldn't have to unpack things, we could skip this line!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I'll just use the tags map as it is.

<tr>
<tbody>
<td><%=subscriptions%></td>
Copy link
Member

Choose a reason for hiding this comment

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

Here we could put this, because we'd want to expand back out to the full ranges (since we divided by 10 earlier) -- try this out:

Suggested change
<td><%=subscriptions%></td>
<td><% (range - 1) * 10 - <%= range*10 %></td>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes makes perfect sense. I figured this <%=range*10%> - <%=range*10 + 9%> displays from the minimum range to the range + 9 just so we have it kind of like this:

count

is this okay?

<td>
<select style="width: 50%; height: 50px;">
<%tags.each do |tag| %>
<option><%=tag%></option>
Copy link
Member

Choose a reason for hiding this comment

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

Then here, I actually think we could just display a list of tags, one per line in plain text, not in a select, so just the tag's name, a colon, and the true count next to it -- remember each of these is in the format ['tagname', 888] :

<p><%= tag[0] %>: <%= tag[1] %></p>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this works okay, the display is however a little off.

new-stats

Is there something I can do about this or should i just leave it at this?

Copy link
Member

Choose a reason for hiding this comment

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

Huh, that is so weird. Let me look at the code, but I'm not sure why the <p> tags aren't making newlines. We could try <br /> instead, between each line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No no. I hadn't used <p> tags here.

<%end%>
</select>
</td>
</tbody>
</tr>
<% end %>
<% end %>
</table>
</div>