Skip to content
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

FEATURE: Add group and category restriction to house ads #205

Merged
merged 3 commits into from
Apr 9, 2024

Conversation

janzenisaac
Copy link
Contributor

@janzenisaac janzenisaac commented Apr 5, 2024

Description

This PR adds the ability to apply group and category restrictions to a house ad.

What is included

  • In order to get the group and category selectors to work within admin/assets/javascripts/discourse/controllers/admin-plugins-house-ads-show.js I needed to modernize the file.
  • I dropped the bufferedProperty implementation in favor of a vanilla ember approach
  • I added category_ids and group_ids to our house ads model
  • I added tests for group / category restrictions
  • I added a preview button to display the house ad
  • /site.json would return a object called house_creatives and a list of key value pairs that matched the ad name with the html, like so:
{ AD_KEY: ad.html }

I need access to the category ids on the client to conditionally render the house ads so the new format will be:

{ AD_KEY: { html: ad.html, category_ids: ad.category_ids } }

Screenshots

Screenshot 2024-04-08 at 2 39 22 PM

Preview Video

preview.mov

@janzenisaac janzenisaac changed the title FEATURE: Add group and category conditions to an ad FEATURE: Add group and category restriction to house ads Apr 8, 2024
@janzenisaac janzenisaac force-pushed the add-group-and-category-conditions branch from 5ceeef1 to c8d7716 Compare April 8, 2024 20:48
@janzenisaac janzenisaac marked this pull request as ready for review April 8, 2024 20:49
Copy link

@markvanlan markvanlan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple tiny things -- only question I had on specs were, is there coverage for making sure that anon-allowed ads do show for logged in users? Approving :)

});
});
);
this.savingStatus = I18n.t("saved");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you could change this to a fn get savingText that tracks saving to make it a bit easier to follow

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have three different states for savingStatus

  • ""
  • "saving"
  • "saved"

I think we will need to keep it as is, so that we can set the saving status during the ajax call. OR we drop the saving status and just have two states, "" or saved then move it to a getter.

Comment on lines 15 to 16
visible_to_logged_in_users: false,
visible_to_anons: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a ad is set to visible_to_logged_in_users: false and visible_to_anons: true we don't display the ad to authenticated users.

Comment on lines 59 to 60
visible_to_logged_in_users: true,
visible_to_anons: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a ad has visible_to_logged_in_users: true and visible_to_anons: true we do display the ad for authenticated users

@janzenisaac janzenisaac merged commit 554f03f into main Apr 9, 2024
5 checks passed
@janzenisaac janzenisaac deleted the add-group-and-category-conditions branch April 9, 2024 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants