-
-
Notifications
You must be signed in to change notification settings - Fork 107
Fix wiki tag popularity to exclude deleted content #1185
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.
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
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: samdark <47294+samdark@users.noreply.github.com>
Co-authored-by: samdark <47294+samdark@users.noreply.github.com>
views/wiki/_sidebar.php
Outdated
if (UserPermissions::canManageWiki()) { | ||
$tagQuery->orderBy(['frequency' => SORT_DESC]); | ||
} else { | ||
$tagQuery->select(['id' => 'wiki_tag_id', 'name', 'wiki_tags.slug', 'frequency' => 'COUNT(*)']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely avoid queries in views.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed by moving the query logic from the view to the WikiController. The popular tags data is now fetched in the controller and passed to the view as a parameter, following proper MVC architecture. Commit: ebc20ba
Co-authored-by: samdark <47294+samdark@users.noreply.github.com>
controllers/WikiController.php
Outdated
if (UserPermissions::canManageWiki()) { | ||
$tagQuery->orderBy(['frequency' => SORT_DESC]); | ||
} else { | ||
$tagQuery->select(['id' => 'wiki_tag_id', 'name', 'wiki_tags.slug', 'frequency' => 'COUNT(*)']) | ||
->joinWith(['wikis']) | ||
->andWhere(['wikis.status' => Wiki::STATUS_PUBLISHED]) | ||
->groupBy(['wiki_tag_id', 'name', 'slug']) | ||
->orderBy(['frequency' => SORT_DESC]); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (UserPermissions::canManageWiki()) { | |
$tagQuery->orderBy(['frequency' => SORT_DESC]); | |
} else { | |
$tagQuery->select(['id' => 'wiki_tag_id', 'name', 'wiki_tags.slug', 'frequency' => 'COUNT(*)']) | |
->joinWith(['wikis']) | |
->andWhere(['wikis.status' => Wiki::STATUS_PUBLISHED]) | |
->groupBy(['wiki_tag_id', 'name', 'slug']) | |
->orderBy(['frequency' => SORT_DESC]); | |
} | |
$tagQuery->orderBy(['frequency' => SORT_DESC]); | |
if (!UserPermissions::canManageWiki()) { | |
$tagQuery->select(['id' => 'wiki_tag_id', 'name', 'wiki_tags.slug', 'frequency' => 'COUNT(*)']) | |
->joinWith(['wikis']) | |
->andWhere(['wikis.status' => Wiki::STATUS_PUBLISHED]) | |
->groupBy(['wiki_tag_id', 'name', 'slug']) | |
->orderBy(['frequency' => SORT_DESC]); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied the suggested refactor to reduce code duplication. The query now sets base conditions first, then applies conditional modifications for non-admin users. Commit: 8091986
widgets/WikiTaglist.php
Outdated
if (UserPermissions::canManageWiki()) { | ||
$query->where('frequency > 1') | ||
->orderBy(['frequency' => SORT_DESC]); | ||
// } else { | ||
// $query->select(['id' => 'news_tag_id', 'name', 'news_tags.slug', 'frequency' => 'COUNT(*)']) | ||
// ->joinWith(['news']) | ||
// ->andWhere(['news.status' => News::STATUS_PUBLISHED]) | ||
// ->groupBy(['news_tag_id', 'name', 'slug']) | ||
// ->orderBy(['frequency' => SORT_DESC]); | ||
// } | ||
} else { | ||
$query->select(['id' => 'wiki_tag_id', 'name', 'wiki_tags.slug', 'frequency' => 'COUNT(*)']) | ||
->joinWith(['wikis']) | ||
->andWhere(['wikis.status' => Wiki::STATUS_PUBLISHED]) | ||
->groupBy(['wiki_tag_id', 'name', 'slug']) | ||
->orderBy(['frequency' => SORT_DESC]); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (UserPermissions::canManageWiki()) { | |
$query->where('frequency > 1') | |
->orderBy(['frequency' => SORT_DESC]); | |
// } else { | |
// $query->select(['id' => 'news_tag_id', 'name', 'news_tags.slug', 'frequency' => 'COUNT(*)']) | |
// ->joinWith(['news']) | |
// ->andWhere(['news.status' => News::STATUS_PUBLISHED]) | |
// ->groupBy(['news_tag_id', 'name', 'slug']) | |
// ->orderBy(['frequency' => SORT_DESC]); | |
// } | |
} else { | |
$query->select(['id' => 'wiki_tag_id', 'name', 'wiki_tags.slug', 'frequency' => 'COUNT(*)']) | |
->joinWith(['wikis']) | |
->andWhere(['wikis.status' => Wiki::STATUS_PUBLISHED]) | |
->groupBy(['wiki_tag_id', 'name', 'slug']) | |
->orderBy(['frequency' => SORT_DESC]); | |
} | |
$query->where('frequency > 1') | |
->orderBy(['frequency' => SORT_DESC]); | |
if (!UserPermissions::canManageWiki()) { | |
$query->select(['id' => 'wiki_tag_id', 'name', 'wiki_tags.slug', 'frequency' => 'COUNT(*)']) | |
->joinWith(['wikis']) | |
->andWhere(['wikis.status' => Wiki::STATUS_PUBLISHED]) | |
->groupBy(['wiki_tag_id', 'name', 'slug']); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied the suggested refactor pattern. The widget now sets base query conditions first, then applies conditional filtering for non-admin users. Also removed duplicate orderBy call as suggested. Commit: 8091986
Co-authored-by: samdark <47294+samdark@users.noreply.github.com>
Fixes #382 where deleted spam content was still appearing in the "Popular Tags" section of the wiki.
Problem
The wiki sidebar was showing tags from deleted articles as popular tags, allowing spam content that had been deleted to still pollute the tag popularity rankings. This occurred because:
WikiTaglist
widget was using pre-calculated frequency values without filtering by wiki statusSolution
Modified both locations to dynamically calculate tag frequency while excluding deleted content for regular users:
Changes Made
UserPermissions::canManageWiki()
- Consistent with existingcanManageNews()
patternWikiTaglist
widget - Now filters by wiki status for non-admin usersArchitecture
Following proper MVC separation:
WikiController::actionIndex()
): Handles data fetching logic for popular tagsviews/wiki/_sidebar.php
): Pure presentation logic, receives data via parametersWikiTaglist
): Maintains same filtering logic for widget usageBehavior
This follows the exact same pattern already implemented in
NewsTaglist
widget, ensuring consistency across the codebase while maintaining proper architectural separation.Testing
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.