Correct tag count

[urvashigupta7]

Fixes #7965

Screenshot

It can be seen there are 4 research notes, 1question,5 wiki pages with tag one which sums to 10

The topic card for tag one is now showing the correct count.

[codecov]

Codecov Report

Merging #8048 into main will decrease coverage by 1.80%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #8048      +/-   ##
==========================================
- Coverage   81.69%   79.88%   -1.81%     
==========================================
  Files         100      100              
  Lines        5845     5866      +21     
==========================================
- Hits         4775     4686      -89     
- Misses       1070     1180     +110     

Impacted Files	Coverage Δ
app/controllers/users_controller.rb	81.11% <ø> (-1.05%)	⬇️
app/controllers/subscription_controller.rb	59.59% <100.00%> (-10.82%)	⬇️
app/models/tag.rb	97.50% <100.00%> (ø)
app/channels/application_cable/channel.rb	0.00% <0.00%> (-100.00%)	⬇️
app/channels/application_cable/connection.rb	0.00% <0.00%> (-100.00%)	⬇️
app/channels/user_channel.rb	0.00% <0.00%> (-83.34%)	⬇️
app/channels/user_notification_channel.rb	0.00% <0.00%> (-83.34%)	⬇️
app/channels/room_channel.rb	0.00% <0.00%> (-71.43%)	⬇️
app/controllers/questions_controller.rb	81.57% <0.00%> (-9.22%)	⬇️
... and 10 more

[urvashigupta7]

@jywarren @cesswairimu Please review 😄

[IshaGupta18]

This looks good to me @urvashigupta7

[jywarren]

Aha - so, i think here we actually do need to keep -shown_nids.count because that takes the total, but deducts the currently displayed posts because the link says X more posts -- does that make sense? I think your fix should still work because it resolves the spam filtering on line 119 of the controller.

Can you re-add the offset for the already-displayed posts? Thank you!!!

[urvashigupta7]

Just a doubt @jywarren 😅, for example, we have 2 posts with tag one and 1 post with tag one and two both,
and all the 3 posts gets displayed in the topic card for tag one. So, topic card of two should show 0 more posts since the post which had both tags(one and two) is displayed already in topic card of tag-one. I guess this is the thing we want here, right ?

[jywarren]

Aha - yes. So we could maybe use this syntax --

[Tag.counter(tag.name)-shown_nids.count - Tag.find_nodes_by_type(tag.name, type = 'note', limit = 3).where.not(nid: shown_nids).count, 0].max

?

[ebarry]

Thank you for taking this up @urvashigupta7 ! 🌟🔢

[urvashigupta7]

@ebarry 😄

[ebarry]

Thanks @jywarren ! Thank you @urvashigupta7 , what do you think of Jeff's last comment ?

[urvashigupta7]

@ebarry I tried the changes suggested by him. But, I think it is not producing the right results 🤔 Correct me if I am wrong 😅

[jywarren]

Suggested change

	<a style="padding-top:15px;text-decoration:underline;color:#808080;display:inline-block;" href="/tag/<%= tag.name %>"><%= [Tag.counter(tag.name) - shown_nids.count - Tag.find_nodes_by_type(tag.name, type = 'note', limit = 3).where.not(nid: shown_nids).count, 0].max %> <%= translation('tag.index.more_posts') %> &raquo;</a>
	<a style="padding-top:15px;text-decoration:underline;color:#808080;display:inline-block;" href="/tag/<%= tag.name %>"><%= [Tag.counter(tag.name) - Tag.find_nodes_by_type(tag.name, type = 'note').where(nid: shown_nids).count, 0].max %> <%= translation('tag.index.more_posts') %> &raquo;</a>

[gitpod-io]

[jywarren]

I'm really sorry - due to errors in our GitPod setup (#8177 (comment)) we can't test this out in GitPod. If you're able to pull this down to see if it works, we could complete it that way! Or I can try myself in the next few days. Thank you for your patience!!!

[urvashigupta7]

plots2/app/models/tag.rb

Line 95 in cd9bd3a

def self.find_nodes_by_type(tagnames, type = 'note', limit = 10)

This method suplies default value for limit, that means it can return maximum 10,but we don't want that, we want count of all the tags shown above it . Should I make another method which doesn't limit the tag count? Or Is there any other approach for doing this?

[urvashigupta7]

We can make a new method which takes three arguments - tag name, type, shown_nids and it will directly return the count of tags above it.

[jywarren]

I believe that due to the way .limit() works in Rails, we can pass false and it will not limit!

plots2/app/models/tag.rb

Line 114 in cd9bd3a

.limit(limit)

We can make a new method which takes three arguments - tag name, type, shown_nids and it will directly return the count of tags above it.

i can see the logic here but i worry it's not a widely enough used function to make it worthwhile! However, you're right that we could then write a simple unit test for it and so better protect it. I'm going to try pulling down this code to manually test, and see how we do.

[gitpod-io]

[jywarren]

Also closing and reopening to try to retrigger the tests, which look to have gotten stuck!

[jywarren]

Hi, @urvashigupta7 I've just had an insight that makes me feel kind of silly -- why do we really need to say X more posts anyways? Isn't it simpler to just show the count itself and just say X posts? That makes the math... nonexistent.

I also finally found that we had both a tag.count attribute maintained by the tag.run_count method, AND a Tag.counter(tag.name) method, so I searched and found Tag.counter(tag.name) is only used here. I'm going to switch it to the simpler and more efficient tag.count, and make some other small adjustments. in #8249.

Thank you so much for going through this journey together! It's been pretty convoluted and I want to be sure you feel good about it, and know how much we appreciate it. Is there another issue you'd like to try to take up that might end up being a little easier? This one was really not very typical, i have to say, but you've been great to work with. Thank you!!!

[urvashigupta7]

Thank you soo much @jywarren. I will look into other issues for sure.