group subscription tags with number of subscribers #5224

GettyOrawo · 2019-03-21T15:19:56Z

Display After Refactors

PR is descriptively titled 📑 and links the original issue above 🔗
tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
code is in uniquely-named feature branch and has no merge conflicts 📁
screenshots/GIFs are attached 📎 in case of UI updation
ask @publiclab/reviewers for help, in a comment below

GettyOrawo · 2019-03-21T15:23:25Z

@publiclab/reviewers I need a review on this one please

plotsbot · 2019-03-21T15:34:59Z

	1 Warning
⚠️	It looks like you merged from master in this pull request. Please rebase to get rid of the merge commits – you may want to rewind the master branch and rebase instead of merging in from master, which can cause problems when accepting new code!

	2 Messages
📖	@GettyOrawo Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don’t be discouraged if you see errors – we’re here to help.
📖	It looks like you haven’t marked all the checkboxes. Help us review and accept your suggested changes by going through the steps one by one. If it is still a ‘Work in progress’, please include ‘[WIP]’ in the title.

Generated by 🚫 Danger

cesswairimu

This is fantastic!! 🎉 🎉
Thanks so much for working on this 🎈

jywarren

This was really nicely set up, easy to review, and you're almost there. I made a couple suggestions to simplify it even a little bit more, so that we just get a table of plain text, for now sorted by ranges of 10. This'll set us up really nicely for future refinements, too!

Thanks for writing a really clear PR and submitting a great screenshot of the feature working. And thanks for your help!

jywarren · 2019-03-21T18:48:18Z

app/controllers/stats_controller.rb

@@ -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("-") } } }


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!

nice that the group is automatically sorted, too!

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?

jywarren · 2019-03-21T18:49:08Z

app/views/stats/subscriptions.html.erb

+				<th>Tags</th>
+			</thead>
+		</tr>
+		<% @tags.each do |map| %>


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

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].

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

jywarren · 2019-03-21T18:49:29Z

app/views/stats/subscriptions.html.erb

+			</thead>
+		</tr>
+		<% @tags.each do |map| %>
+		<% map.each do |subscriptions,tags| %>


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

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

jywarren · 2019-03-21T18:53:15Z

app/views/stats/subscriptions.html.erb

+		<% map.each do |subscriptions,tags| %>
+		<tr>
+			<tbody>
+				<td><%=subscriptions%></td>


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>

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:

is this okay?

jywarren · 2019-03-21T18:54:57Z

app/views/stats/subscriptions.html.erb

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


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] :

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

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

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

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

No no. I hadn't used  tags here.

…ssify-tags-with-subscriber-numbers

jywarren

Just two more small changes and this is ready to merge! Great work!

jywarren · 2019-03-22T18:57:04Z

app/views/stats/subscriptions.html.erb

 <div class="col-md-8 col-md-offset-1">
    <h2>Subscriptions</h2>
-</div>
+ 	<table class="table inline-grid">
+		<tr>


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! 😄

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

jywarren · 2019-03-22T18:58:27Z

app/views/stats/subscriptions.html.erb

+			<tbody>
+				<td><%= range * 10 %> - <%= range * 10 + 9 %></td>
+				<% tags.each do |tag| %>
+				<td><%= tag[0]%>:<%=tag[1] %></td>


Oh, aha - yes, so instead of the <td> we need to also use  tags - and the <td> tags should go outside the each loop. That way the whole collection will be in a single table cell, but each tag will be in its own paragraph (on its own line). Make sense?

Yes though long it's clear. and more readable too.

Great! Actually let's tackle a few more things in a follow-up PR:

let's sort this DESCENDING or desc so we start with the most-used tags

let's hide anything more than 25 results and show a "View all" link to show them?

let's make the tag names into links that go to /tag/TAGNAME

and we can do more! This is a great start!

…b.com/GettyOrawo/plots2 into classify-tags-with-subscriber-numbers

GettyOrawo · 2019-03-23T10:05:21Z

Hey @jywarren I cleared the suggested reviews. Any more fixes I need to do on this one?

welcome · 2019-03-25T19:31:57Z

Congrats on merging your first pull request! 🙌🎉⚡️
Your code will likely be published to PublicLab.org in the next few days, but first it will be published to https://stable.publiclab.org/ (it will take some minutes for this to load, and until then you may see logs from the build process). Please test out your work on this testing server and report back with a comment that all has gone well!
Now that you've completed this, you can help someone else take their first step!
Reach out to someone else working on theirs on Public Lab's code welcome page. Thanks!

Help others take their first step

Now that you've merged your first pull request, you're the perfect person to help someone else out with this challenging first step. 🙌

https://code.publiclab.org

Try looking at this list of `first-timers-only` issues, and see if someone else is waiting for feedback, or even stuck! 😕

People often get stuck at the same steps, so you might be able to help someone get unstuck, or help lead them to some documentation that'd help. Reach out and be encouraging and friendly! 😄 🎉

Read about how to help support another newcomer here, or find other ways to offer mutual support here.

jywarren · 2019-03-25T19:33:37Z

Fantastic work!!! I'll open a new issue with the follow-ups. #5260

Thanks so much!!!!

* group tags with number of subscribers * cleanup * fix codeclimate errors * refactoring the subscriber count range and stats display * fix codeclimate failing lint test * fix indent on stats view

group tags with number of subscribers

aac8f35

GettyOrawo mentioned this pull request Mar 21, 2019

[WIP] Design subscription stats output #5151

Closed

5 tasks

cleanup

fbdb8f6

GettyOrawo mentioned this pull request Mar 21, 2019

Explore how to present subscription stats #4603

Closed

2 tasks

fix codeclimate errors

a595c57

cesswairimu changed the title ~~[WIP] group subscription tags with number of subscribers~~ group subscription tags with number of subscribers Mar 21, 2019

cesswairimu approved these changes Mar 21, 2019

View reviewed changes

jywarren requested changes Mar 21, 2019

View reviewed changes

GettyOrawo added 3 commits March 22, 2019 10:11

refactoring the subscriber count range and stats display

b33bef7

Merge branch 'master' of https://github.com/publiclab/plots2 into cla…

83b647c

…ssify-tags-with-subscriber-numbers

fix codeclimate failing lint test

e536c80

jywarren requested changes Mar 22, 2019

View reviewed changes

GettyOrawo added 2 commits March 23, 2019 12:28

fix indent on stats view

bf8d12f

Merge branch 'classify-tags-with-subscriber-numbers' of https://githu…

8a6eab3

…b.com/GettyOrawo/plots2 into classify-tags-with-subscriber-numbers

jywarren approved these changes Mar 25, 2019

View reviewed changes

jywarren merged commit 23ef859 into publiclab:master Mar 25, 2019

jywarren mentioned this pull request Mar 25, 2019

Refinements to new tag subscription statistics page #5260

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

group subscription tags with number of subscribers #5224

group subscription tags with number of subscribers #5224

GettyOrawo commented Mar 21, 2019 •

edited

Loading

GettyOrawo commented Mar 21, 2019

plotsbot commented Mar 21, 2019 •

edited

Loading

cesswairimu left a comment

jywarren left a comment

jywarren Mar 21, 2019

jywarren Mar 21, 2019

GettyOrawo Mar 22, 2019

jywarren Mar 21, 2019

jywarren Mar 21, 2019

GettyOrawo Mar 22, 2019 •

edited

Loading

jywarren Mar 21, 2019

GettyOrawo Mar 22, 2019

jywarren Mar 21, 2019

GettyOrawo Mar 22, 2019

jywarren Mar 21, 2019

GettyOrawo Mar 22, 2019

jywarren Mar 22, 2019

GettyOrawo Mar 23, 2019

jywarren left a comment

jywarren Mar 22, 2019

GettyOrawo Mar 23, 2019

jywarren Mar 22, 2019

GettyOrawo Mar 23, 2019

jywarren Mar 25, 2019

GettyOrawo commented Mar 23, 2019

welcome bot commented Mar 25, 2019

jywarren commented Mar 25, 2019

	@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!

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

group subscription tags with number of subscribers #5224

group subscription tags with number of subscribers #5224

Conversation

GettyOrawo commented Mar 21, 2019 • edited Loading

GettyOrawo commented Mar 21, 2019

plotsbot commented Mar 21, 2019 • edited Loading

cesswairimu left a comment

Choose a reason for hiding this comment

jywarren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GettyOrawo Mar 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jywarren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GettyOrawo commented Mar 23, 2019

welcome bot commented Mar 25, 2019

jywarren commented Mar 25, 2019

GettyOrawo commented Mar 21, 2019 •

edited

Loading

plotsbot commented Mar 21, 2019 •

edited

Loading

GettyOrawo Mar 22, 2019 •

edited

Loading