From bac350e8772547c44e6dfe911afd621ef1277f8a Mon Sep 17 00:00:00 2001 From: Alex Date: Wed, 8 Jul 2020 14:07:53 -0400 Subject: [PATCH] [deploy] Add role and docs for user subscription liquid tag (#9170) * Create new restriced_liquid_tag role * Update role spec * Use new role over admin * Add documentation for user_subscription_liquid_tag * Stub roles to help avoid flaky specs * Add user approved liquid tags to edit view * Remove "simply". Coding is hard, yo! * Update liquid_help * Update liquid help * Update and refactor dashboard specs * Refactor role check for liquid tag docs * Improve docs wording --- app/controllers/articles_controller.rb | 15 ++++++ app/liquid_tags/user_subscription_tag.rb | 7 +-- .../liquid_tags/user_subscription_tag.rb | 9 ++++ app/models/role.rb | 1 + app/views/pages/_editor_liquid_help.html.erb | 11 ++++ docs/frontend/liquid-tags.md | 17 +++--- spec/factories/articles.rb | 3 +- spec/factories/users.rb | 8 +++ .../liquid_tags/user_subscription_tag_spec.rb | 10 ++-- spec/models/role_spec.rb | 7 ++- spec/policies/liquid_tag_policy_spec.rb | 9 ++-- spec/requests/dashboard_spec.rb | 52 +++++++++++-------- 12 files changed, 106 insertions(+), 43 deletions(-) create mode 100644 app/models/liquid_tags/user_subscription_tag.rb diff --git a/app/controllers/articles_controller.rb b/app/controllers/articles_controller.rb index 83bc4c129..df96e61b4 100644 --- a/app/controllers/articles_controller.rb +++ b/app/controllers/articles_controller.rb @@ -18,6 +18,8 @@ class ArticlesController < ApplicationController rowspan size span src start strong title value width ].freeze + RESTRICTED_LIQUID_TAGS = [UserSubscriptionTag].freeze + def feed skip_authorization @@ -63,6 +65,7 @@ class ArticlesController < ApplicationController @version = @article.has_frontmatter? ? "v1" : "v2" @user = @article.user @organizations = @user&.organizations + set_user_approved_liquid_tags end def manage @@ -189,6 +192,18 @@ class ArticlesController < ApplicationController @organizations = @user&.organizations @tag = Tag.find_by(name: params[:template]) @prefill = params[:prefill].to_s.gsub("\\n ", "\n").gsub("\\n", "\n") + set_user_approved_liquid_tags + end + + def set_user_approved_liquid_tags + @user_approved_liquid_tags = + if @user + RESTRICTED_LIQUID_TAGS.filter_map do |liquid_tag| + liquid_tag if liquid_tag::VALID_ROLES.any? { |role| @user.has_role?(*Array(role)) } + end + else + [] + end end def handle_user_or_organization_feed diff --git a/app/liquid_tags/user_subscription_tag.rb b/app/liquid_tags/user_subscription_tag.rb index 3eb034897..00d6fc2ba 100644 --- a/app/liquid_tags/user_subscription_tag.rb +++ b/app/liquid_tags/user_subscription_tag.rb @@ -1,9 +1,10 @@ class UserSubscriptionTag < LiquidTagBase PARTIAL = "liquids/user_subscription".freeze VALID_CONTEXTS = %w[Article].freeze - VALID_ROLES = %i[ - admin - super_admin + VALID_ROLES = [ + :admin, + [:restricted_liquid_tag, LiquidTags::UserSubscriptionTag], + :super_admin, ].freeze SCRIPT = <<~JAVASCRIPT.freeze diff --git a/app/models/liquid_tags/user_subscription_tag.rb b/app/models/liquid_tags/user_subscription_tag.rb new file mode 100644 index 000000000..90356416d --- /dev/null +++ b/app/models/liquid_tags/user_subscription_tag.rb @@ -0,0 +1,9 @@ +module LiquidTags + class UserSubscriptionTag < ApplicationRecord + resourcify + # This class exists to take advantage of Rolify for limiting authorization + # on internal reports. + # NOTE: It is not backed by a database table and should not be expected to + # function like a traditional Rails model + end +end diff --git a/app/models/role.rb b/app/models/role.rb index 26b0375d3..08e454fed 100644 --- a/app/models/role.rb +++ b/app/models/role.rb @@ -6,6 +6,7 @@ class Role < ApplicationRecord comment_banned podcast_admin pro + restricted_liquid_tag single_resource_admin super_admin tag_moderator diff --git a/app/views/pages/_editor_liquid_help.html.erb b/app/views/pages/_editor_liquid_help.html.erb index 4fae23ec9..475160602 100644 --- a/app/views/pages/_editor_liquid_help.html.erb +++ b/app/views/pages/_editor_liquid_help.html.erb @@ -21,6 +21,17 @@ ID at the end of a comment URL. To get the comment link, click either the timestamp or the menu button in the top right corner on a comment and then click "Permalink". Here's an example:

{% devcomment 2d1a %} + <% if @user_approved_liquid_tags.include? UserSubscriptionTag %> +

<%= community_name %> User Subscriptions

+

This tag can only be used in articles.

+

You can add call-to-action text that will show above the subscribe button:

+ {% user_subscription If you'd like to receive future updates, subscribe below! %} +

+ If a reader is signed out, the button will prompt them to sign in first + to subscribe. If the reader is signed in, the button will prompt them + to subscribe. +

+ <% end %>

<%= community_name %> Podcast Episode Embed

All you need is the full link of the podcast episode:

{% podcast https://dev.to/basecspodcast/s2e2--queues-irl %} diff --git a/docs/frontend/liquid-tags.md b/docs/frontend/liquid-tags.md index e101cbacd..7bdcebb80 100644 --- a/docs/frontend/liquid-tags.md +++ b/docs/frontend/liquid-tags.md @@ -113,21 +113,24 @@ https://github.com/thepracticaldev/dev.to/pull/3801 To only allow users with specific roles to use a liquid tag, you need to define a `VALID_ROLES` constant on the liquid tag itself. It needs to be an `Array` of -valid roles. For [single admin resource roles](/internal), it needs to be an -`Array` with the role and the resource. Here's an example: +valid roles. For [single resource roles](/internal), it needs to be an `Array` +with the role and the resource. Here's an example: ```ruby class NewLiquidTag < LiquidTagBase VALID_ROLES = [ :admin, - [:single_resource_admin, NewLiquidTag] + [:restricted_liquid_tag, LiquidTags::UserSubscriptionTag] ].freeze end ``` -Here we are saying that the `NewLiquidTag` is only usable by users with the -`admin` role or with a role of `:single_resource_admin` and a specified resource -of `NewLiquidTag`. +Here we are saying that the `UserSubscriptionTag` is only usable by users with +the `admin` role or with a role of `:restricted_liquid_tag` and a specified +resource of `LiquidTags::UserSubscriptionTag`. + +`LiquidTags::UserSubscriptionTag` is a resource model so we that can play nicely +with the [Rolify][rolify] gem. See [/internal](/internal) for more information. **REMINDER: if you do not define a `VALID_ROLES` constant, the liquid tag will be usable by all users by default.** @@ -151,3 +154,5 @@ end **REMINDER: if you do not define a `VALID_CONTEXTS` constant the liquid tag will be usable in all contexts by default.** + +[rolify]: https://github.com/RolifyCommunity/rolify diff --git a/spec/factories/articles.rb b/spec/factories/articles.rb index a9630c635..7a83872a5 100644 --- a/spec/factories/articles.rb +++ b/spec/factories/articles.rb @@ -56,8 +56,7 @@ FactoryBot.define do end end - # TODO: (Alex Smith) - update roles before release trait :with_user_subscription_tag_role_user do - after(:build) { |article| article.user.add_role(:super_admin) } + after(:build) { |article| article.user.add_role(:restricted_liquid_tag, LiquidTags::UserSubscriptionTag) } end end diff --git a/spec/factories/users.rb b/spec/factories/users.rb index 8b08fd7ad..5aaaaa74d 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -55,6 +55,14 @@ FactoryBot.define do after(:build) { |user, options| user.add_role(:single_resource_admin, options.resource) } end + trait :restricted_liquid_tag do + transient do + resource { nil } + end + + after(:build) { |user, options| user.add_role(:restricted_liquid_tag, options.resource) } + end + trait :super_plus_single_resource_admin do transient do resource { nil } diff --git a/spec/liquid_tags/user_subscription_tag_spec.rb b/spec/liquid_tags/user_subscription_tag_spec.rb index df2c9a273..3161a8eaa 100644 --- a/spec/liquid_tags/user_subscription_tag_spec.rb +++ b/spec/liquid_tags/user_subscription_tag_spec.rb @@ -4,9 +4,13 @@ RSpec.describe UserSubscriptionTag, type: :liquid_tag do setup { Liquid::Template.register_tag("user_subscription", described_class) } let(:subscriber) { create(:user) } - let(:author) { create(:user, :super_admin) } # TODO: (Alex Smith) - update roles before release - let(:article_with_user_subscription_tag) do - create(:article, :with_user_subscription_tag_role_user, with_user_subscription_tag: true) + let(:author) { create(:user) } + let(:article_with_user_subscription_tag) { create(:article, :with_user_subscription_tag_role_user, with_user_subscription_tag: true) } + + # Stub roles because adding them normally can cause flaky specs + before do + allow(author).to receive(:has_role?).and_call_original + allow(author).to receive(:has_role?).with(:restricted_liquid_tag, LiquidTags::UserSubscriptionTag).and_return(true) end context "when rendering" do diff --git a/spec/models/role_spec.rb b/spec/models/role_spec.rb index d0b5f2a4d..e3d53628f 100644 --- a/spec/models/role_spec.rb +++ b/spec/models/role_spec.rb @@ -8,10 +8,9 @@ RSpec.describe Role, type: :model do describe "::ROLES" do it "contains the correct values" do expected_roles = %w[ - admin banned chatroom_beta_tester comment_banned - podcast_admin pro single_resource_admin super_admin - tag_moderator mod_relations_admin tech_admin - trusted warned workshop_pass + admin banned chatroom_beta_tester comment_banned podcast_admin pro restricted_liquid_tag + single_resource_admin super_admin tag_moderator mod_relations_admin tech_admin trusted + warned workshop_pass ] expect(described_class::ROLES).to eq(expected_roles) end diff --git a/spec/policies/liquid_tag_policy_spec.rb b/spec/policies/liquid_tag_policy_spec.rb index ab76b0e08..33ef8f73f 100644 --- a/spec/policies/liquid_tag_policy_spec.rb +++ b/spec/policies/liquid_tag_policy_spec.rb @@ -40,12 +40,15 @@ RSpec.describe LiquidTagPolicy, type: :policy do end it "handles single resource roles" do - # TODO: (Alex Smith) - update roles to new liquid tag role for more relevant example/use - user = create(:user, :single_resource_admin, resource: Article) + user = create(:user) + # Stub roles because adding them normally can cause flaky specs + single_resource_role = [:restricted_liquid_tag, LiquidTags::UserSubscriptionTag] + allow(user).to receive(:has_role?).and_call_original + allow(user).to receive(:has_role?).with(*single_resource_role).and_return(true) parse_context = { source: article, user: user } allow(liquid_tag).to receive(:parse_context).and_return(parse_context) - stub_const("#{liquid_tag.class}::VALID_ROLES", [[:single_resource_admin, Article]]) + stub_const("#{liquid_tag.class}::VALID_ROLES", [single_resource_role]) expect do Pundit.authorize(user, liquid_tag, action, policy_class: described_class) end.not_to raise_error diff --git a/spec/requests/dashboard_spec.rb b/spec/requests/dashboard_spec.rb index 6663c43ef..f8ab3b15b 100644 --- a/spec/requests/dashboard_spec.rb +++ b/spec/requests/dashboard_spec.rb @@ -40,7 +40,9 @@ RSpec.describe "Dashboards", type: :request do end it "renders subscriptions for articles with subscriptions" do - user.add_role(:admin) # TODO: (Alex Smith) - update roles before release + allow(user).to receive(:has_role?).and_call_original + allow(user).to receive(:has_role?).with(:restricted_liquid_tag, + LiquidTags::UserSubscriptionTag).and_return(true) article_with_user_subscription_tag = create(:article, user: user, with_user_subscription_tag: true) create(:user_subscription, subscriber_id: second_user.id, @@ -314,75 +316,81 @@ RSpec.describe "Dashboards", type: :request do end end - # TODO: (Alex Smith) - update roles before release describe "GET /dashboard/subscriptions" do + let(:author) { create(:user) } + let(:article_with_user_subscription_tag) { create(:article, user: author, with_user_subscription_tag: true) } + let(:params) do + { source_type: article_with_user_subscription_tag.class.name, source_id: article_with_user_subscription_tag.id } + end + before do - sign_in user + # Stub roles because adding them normally can cause flaky specs + allow(author).to receive(:has_role?).and_call_original + allow(author).to receive(:has_role?).with(:restricted_liquid_tag, + LiquidTags::UserSubscriptionTag).and_return(true) + + sign_in author end it "renders subscriptions" do - user.add_role(:admin) - article_with_user_subscription_tag = create(:article, user: user, with_user_subscription_tag: true) user_subscription = create(:user_subscription, subscriber_id: second_user.id, subscriber_email: second_user.email, author_id: article_with_user_subscription_tag.user_id, user_subscription_sourceable: article_with_user_subscription_tag) - get "/dashboard/subscriptions", params: { source_type: article_with_user_subscription_tag.class.name, source_id: article_with_user_subscription_tag.id } + get "/dashboard/subscriptions", params: params expect(response.body).to include(user_subscription.subscriber_email) end it "displays a message if no subscriptions are found" do - get "/dashboard/subscriptions", params: { source_type: article.class.name, source_id: article.id } + get "/dashboard/subscriptions", params: params expect(response.body).to include("You don't have any subscribers for this") end it "raises unauthorized when trying to access a source the user doesn't own" do - user.add_role(:admin) - article_with_user_subscription_tag = create(:article, :with_user_subscription_tag_role_user, with_user_subscription_tag: true) + unauthorized_article = create(:article, :with_user_subscription_tag_role_user, with_user_subscription_tag: true) create(:user_subscription, subscriber_id: second_user.id, subscriber_email: second_user.email, - author_id: article_with_user_subscription_tag.user_id, - user_subscription_sourceable: article_with_user_subscription_tag) + author_id: unauthorized_article.user_id, + user_subscription_sourceable: unauthorized_article) + unauthorized_article_params = { source_type: unauthorized_article.class.name, source_id: unauthorized_article.id } expect do - get "/dashboard/subscriptions", params: { source_type: article_with_user_subscription_tag.class.name, source_id: article_with_user_subscription_tag.id } + get "/dashboard/subscriptions", params: unauthorized_article_params end.to raise_error(Pundit::NotAuthorizedError) end it "raises an error for disallowed source_types" do + invalid_source_type_params = { source_type: "Comment", source_id: 1 } expect do - get "/dashboard/subscriptions", params: { source_type: "Comment", source_id: 1 } + get "/dashboard/subscriptions", params: invalid_source_type_params end.to raise_error(ActiveRecord::RecordNotFound) end it "raises an error when the source can't be found" do + nonexistant_article_params = { source_type: article.class.name, source_id: article.id + 999 } expect do - get "/dashboard/subscriptions", params: { source_type: article.class.name, source_id: article.id + 999 } + get "/dashboard/subscriptions", params: nonexistant_article_params end.to raise_error(ActiveRecord::RecordNotFound) end it "renders pagination if minimum amount of subscriptions" do - user.add_role(:admin) - article_with_user_subscription_tag = create(:article, user: user, with_user_subscription_tag: true) create_list(:user_subscription, 102, # Current pagination limit is 100 - author: user, + author: author, user_subscription_sourceable: article_with_user_subscription_tag) - get "/dashboard/subscriptions", params: { source_type: article_with_user_subscription_tag.class.name, source_id: article_with_user_subscription_tag.id } + get "/dashboard/subscriptions", params: params expect(response.body).to include "pagination" end it "does not render pagination if less than one full page" do - user.add_role(:admin) - article_with_user_subscription_tag = create(:article, user: user, with_user_subscription_tag: true) create_list(:user_subscription, 5, - author: user, + author: author, user_subscription_sourceable: article_with_user_subscription_tag) - get "/dashboard/subscriptions", params: { source_type: article_with_user_subscription_tag.class.name, source_id: article_with_user_subscription_tag.id } + get "/dashboard/subscriptions", params: params expect(response.body).not_to include "pagination" end end