FIX: Topic referrals by user were wrong #2999
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
You've signed the CLA, riking. Thank you! This pull request is ready for review. |
|
While checking the build error, I discovered that the tests were extremely bad. So I rewrote the integration test, and realized that I missed that another report had the same problem. I think that the same solution can be used for the original change, too, reducing the diff significantly. |
|
Yup, that was easier. New before/after:
|
|
👍 Will merge once rebased ;) |
Due to what seems to be a bug in ActiveRecord, the distinct: true option is not recognized on counts with string column names. This commit fixes that by moving the DISTINCT into the count string. For robustness, the integration spec for IncomingLinksReport was rewritten to be an actual integration spec, running the actual interface on actual fake data.
riking
force-pushed
the
fix_top_referrers_topic_count
branch
from
ce5c5bf
to
728e8a2
Compare
|
@ZogStriP Rebased :) |
ZogStriP
added a commit
that referenced
this pull request
Nov 25, 2014
FIX: Topic referrals by user were wrong
|
❤️ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit fixes the "Top Referrers (Last 30 Days)" display in the
Admin panel, specifically the "Topics" column. Previously, the Clicks
and Topics column of the table would be identical. This commit fixes
that by counting the number of different topics that were referred to.
Raw SQL queries are used because it was easier for me to write, and it
was easier to extract the data needed to pass on from the first query to
the second (the user ids).
The data shuffling after the queries shouldn't be too bad, I just needed
to get it into the report format. Not sure how I would eliminate the
.sort_by at the end without having to preserve order everywhere, which
is not a guarantee I am willing to rely on. (The second query is
unordered for this reason.)
Fixes discrepancy noticed here and since degraded https://meta.discourse.org/t/top-referrers-difference-between-views-and-topics/15812/6
Before | After