Skip to content

Commit

Permalink
FIX: Calculate no ads for groups server side (#200)
Browse files Browse the repository at this point in the history
If the selected group to not display ads to had its visibility set to
not be visible then this setting wouldn't work correctly because that
group wouldn't be available client side. The change moves that group
check to be server side so that we can correctly see all the groups that
should not see ads.
  • Loading branch information
oblakeerickson authored Feb 15, 2024
1 parent 1677f7a commit b0c9511
Show file tree
Hide file tree
Showing 8 changed files with 16 additions and 18 deletions.
20 changes: 4 additions & 16 deletions assets/javascripts/discourse/components/ad-component.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,25 +45,13 @@ export default Component.extend({
return topicType === "private_message";
},

@discourseComputed("currentUser.groups")
showToGroups(groups) {
const currentUser = this.currentUser;

if (
!currentUser ||
!groups ||
!this.siteSettings.no_ads_for_groups ||
this.siteSettings.no_ads_for_groups.length === 0
) {
@discourseComputed
showToGroups() {
if (!this.currentUser) {
return true;
}

let noAdsGroups = this.siteSettings.no_ads_for_groups
.split("|")
.filter(Boolean);
let currentGroups = groups.map((g) => g.id.toString());

return !currentGroups.any((g) => noAdsGroups.includes(g));
return this.currentUser.show_to_groups;
},

@discourseComputed(
Expand Down
1 change: 0 additions & 1 deletion config/settings.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ ad_plugin:
client: true
default: false
no_ads_for_groups:
client: true
default: ""
type: group_list
no_ads_for_categories:
Expand Down
4 changes: 4 additions & 0 deletions lib/adplugin/guardian_extensions.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,9 @@ def show_amazon_ads?
def show_adbutler_ads?
!self.user.in_any_groups?(SiteSetting.adbutler_exclude_groups_map)
end

def show_to_groups?
!self.user.in_any_groups?(SiteSetting.no_ads_for_groups_map)
end
end
end
4 changes: 4 additions & 0 deletions plugin.rb
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,10 @@ def self.pstore_delete(key)
scope.show_adbutler_ads?
end

add_to_serializer :current_user, :show_to_groups do
scope.show_to_groups?
end

class ::AdstxtController < ::ApplicationController
skip_before_action :preload_json, :check_xhr, :redirect_to_login_if_required

Expand Down
1 change: 1 addition & 0 deletions test/javascripts/acceptance/adsense-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ acceptance("AdSense", function (needs) {
trust_level: 1,
groups: [AUTO_GROUPS.trust_level_1],
show_adsense_ads: true,
show_to_groups: true,
});
await visit("/t/280"); // 20 posts

Expand Down
1 change: 1 addition & 0 deletions test/javascripts/acceptance/dfp-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ acceptance("DFP Ads", function (needs) {
staff: false,
trust_level: 1,
show_dfp_ads: true,
show_to_groups: true,
});
await visit("/t/280"); // 20 posts

Expand Down
2 changes: 1 addition & 1 deletion test/javascripts/acceptance/house-ad-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ acceptance("House Ads", function (needs) {
});

test("correct ads show", async (assert) => {
updateCurrentUser({ staff: false, trust_level: 1 });
updateCurrentUser({ staff: false, trust_level: 1, show_to_groups: true });
await visit("/t/280"); // 20 posts

assert
Expand Down
1 change: 1 addition & 0 deletions test/javascripts/acceptance/mixed-ads-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ acceptance("Mixed Ads", function (needs) {
staff: false,
trust_level: 1,
show_dfp_ads: true,
show_to_groups: true,
});
await visit("/t/280"); // 20 posts

Expand Down

0 comments on commit b0c9511

Please sign in to comment.