DEV: Introduce syntax_tree for ruby formatting (#161)

This commit is contained in:
David Taylor 2022-12-29 12:29:26 +00:00 committed by GitHub
parent ff6b767796
commit c73ac98748
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
13 changed files with 144 additions and 150 deletions

View File

@ -55,3 +55,12 @@ jobs:
- name: Rubocop
if: ${{ !cancelled() }}
run: bundle exec rubocop .
- name: Syntax Tree
if: ${{ !cancelled() }}
run: |
if test -f .streerc; then
bundle exec stree check Gemfile $(git ls-files '*.rb') $(git ls-files '*.rake')
else
echo "Stree config not detected for this repository. Skipping."
fi

View File

@ -80,7 +80,7 @@ jobs:
- name: Get yarn cache directory
id: yarn-cache-dir
run: echo "::set-output name=dir::$(yarn cache dir)"
run: echo "dir=$(yarn cache dir)" >> $GITHUB_OUTPUT
- name: Yarn cache
uses: actions/cache@v3
@ -130,7 +130,7 @@ jobs:
shell: bash
run: |
if [ 0 -lt $(find plugins/${{ github.event.repository.name }}/spec -type f -name "*.rb" 2> /dev/null | wc -l) ]; then
echo "::set-output name=files_exist::true"
echo "files_exist=true" >> $GITHUB_OUTPUT
fi
- name: Plugin RSpec
@ -142,7 +142,7 @@ jobs:
shell: bash
run: |
if [ 0 -lt $(find plugins/${{ github.event.repository.name }}/test/javascripts -type f \( -name "*.js" -or -name "*.es6" \) 2> /dev/null | wc -l) ]; then
echo "::set-output name=files_exist::true"
echo "files_exist=true" >> $GITHUB_OUTPUT
fi
- name: Plugin QUnit

View File

@ -1,2 +1,2 @@
inherit_gem:
rubocop-discourse: default.yml
rubocop-discourse: stree-compat.yml

2
.streerc Normal file
View File

@ -0,0 +1,2 @@
--print-width=100
--plugins=plugin/trailing_comma

View File

@ -1,7 +1,8 @@
# frozen_string_literal: true
source 'https://rubygems.org'
source "https://rubygems.org"
group :development do
gem 'rubocop-discourse'
gem "rubocop-discourse"
gem "syntax_tree"
end

View File

@ -6,6 +6,7 @@ GEM
parallel (1.22.1)
parser (3.1.2.1)
ast (~> 2.4.1)
prettier_print (1.2.0)
rainbow (3.1.1)
regexp_parser (2.6.0)
rexml (3.2.5)
@ -27,6 +28,8 @@ GEM
rubocop-rspec (2.13.2)
rubocop (~> 1.33)
ruby-progressbar (1.11.0)
syntax_tree (5.1.0)
prettier_print (>= 1.2.0)
unicode-display_width (2.3.0)
PLATFORMS
@ -34,6 +37,7 @@ PLATFORMS
DEPENDENCIES
rubocop-discourse
syntax_tree
BUNDLED WITH
2.1.4

View File

@ -5,10 +5,7 @@ module ::AdPlugin
requires_plugin AdPlugin.plugin_name
def index
render_json_dump(
house_ads: HouseAd.all.map(&:to_hash),
settings: HouseAdSetting.all
)
render_json_dump(house_ads: HouseAd.all.map(&:to_hash), settings: HouseAdSetting.all)
end
def show
@ -17,11 +14,7 @@ module ::AdPlugin
def create
ad = HouseAd.create(house_ad_params)
if ad.valid?
render_json_dump(house_ad: ad.to_hash)
else
render_json_error(ad)
end
ad.valid? ? render_json_dump(house_ad: ad.to_hash) : render_json_error(ad)
end
def update
@ -31,18 +24,14 @@ module ::AdPlugin
ad = HouseAd.create(house_ad_params.except(:id))
end
if ad.valid?
render_json_dump(house_ad: ad.to_hash)
else
render_json_error(ad)
end
ad.valid? ? render_json_dump(house_ad: ad.to_hash) : render_json_error(ad)
end
def destroy
if ad = HouseAd.find(house_ad_params[:id])
ad.destroy
else
render_json_error(I18n.t('not_found'), status: 404)
render_json_error(I18n.t("not_found"), status: 404)
end
end

View File

@ -37,7 +37,7 @@ module ::AdPlugin
end
def self.alloc_id
DistributedMutex.synchronize('adplugin-house-ad-id') do
DistributedMutex.synchronize("adplugin-house-ad-id") do
max_id = AdPlugin.pstore_get("ad:_id")
max_id = 1 unless max_id
AdPlugin.pstore_set("ad:_id", max_id + 1)
@ -46,7 +46,7 @@ module ::AdPlugin
end
def self.find(id)
if r = AdPlugin::pstore_get("ad:#{id}")
if r = AdPlugin.pstore_get("ad:#{id}")
from_hash(r)
else
nil
@ -54,18 +54,18 @@ module ::AdPlugin
end
def self.all
PluginStoreRow.where(plugin_name: AdPlugin.plugin_name)
PluginStoreRow
.where(plugin_name: AdPlugin.plugin_name)
.where("key LIKE 'ad:%'")
.where("key != 'ad:_id'")
.map do |psr|
from_hash(PluginStore.cast_value(psr.type_name, psr.value))
end.sort_by { |ad| ad.id }
.map { |psr| from_hash(PluginStore.cast_value(psr.type_name, psr.value)) }
.sort_by { |ad| ad.id }
end
def save
if self.valid?
self.id = self.class.alloc_id if self.id.to_i <= 0
AdPlugin::pstore_set("ad:#{id}", to_hash)
AdPlugin.pstore_set("ad:#{id}", to_hash)
self.class.publish_if_ads_enabled
true
else
@ -80,15 +80,11 @@ module ::AdPlugin
end
def to_hash
{
id: @id,
name: @name,
html: @html
}
{ id: @id, name: @name, html: @html }
end
def destroy
AdPlugin::pstore_delete("ad:#{id}")
AdPlugin.pstore_delete("ad:#{id}")
self.class.publish_if_ads_enabled
end
@ -97,6 +93,5 @@ module ::AdPlugin
AdPlugin::HouseAdSetting.publish_settings
end
end
end
end

View File

@ -3,67 +3,65 @@
module ::AdPlugin
class HouseAdSetting
DEFAULTS = {
topic_list_top: '',
topic_above_post_stream: '',
topic_above_suggested: '',
post_bottom: '',
topic_list_between: ''
topic_list_top: "",
topic_above_post_stream: "",
topic_above_suggested: "",
post_bottom: "",
topic_list_between: "",
}
def self.all
settings = DEFAULTS.dup
PluginStoreRow.where(plugin_name: AdPlugin.plugin_name)
PluginStoreRow
.where(plugin_name: AdPlugin.plugin_name)
.where("key LIKE 'ad-setting:%'")
.each do |psr|
settings[psr.key[11..-1].to_sym] = psr.value
end
.each { |psr| settings[psr.key[11..-1].to_sym] = psr.value }
settings
end
def self.settings_and_ads
settings = AdPlugin::HouseAdSetting.all
ad_names = settings.values.map { |v| v.split('|') }.flatten.uniq
ad_names = settings.values.map { |v| v.split("|") }.flatten.uniq
ads = AdPlugin::HouseAd.all.select { |ad| ad_names.include?(ad.name) }
{
settings: settings.merge(
settings:
settings.merge(
after_nth_post: SiteSetting.house_ads_after_nth_post,
after_nth_topic: SiteSetting.house_ads_after_nth_topic,
house_ads_frequency: SiteSetting.house_ads_frequency
house_ads_frequency: SiteSetting.house_ads_frequency,
),
creatives: ads.inject({}) { |h, ad| h[ad.name] = ad.html; h }
creatives:
ads.inject({}) do |h, ad|
h[ad.name] = ad.html
h
end,
}
end
def self.update(setting_name, value)
unless DEFAULTS.keys.include?(setting_name.to_sym)
raise Discourse::NotFound
end
raise Discourse::NotFound unless DEFAULTS.keys.include?(setting_name.to_sym)
ad_names = value&.split('|') || []
ad_names = value&.split("|") || []
if value && ad_names.any? { |v| v !~ HouseAd::NAME_REGEX }
raise Discourse::InvalidParameters
end
raise Discourse::InvalidParameters if value && ad_names.any? { |v| v !~ HouseAd::NAME_REGEX }
unless ad_names.empty?
ad_names = (HouseAd.all.map(&:name) & ad_names)
end
ad_names = (HouseAd.all.map(&:name) & ad_names) unless ad_names.empty?
new_value = ad_names.join('|')
new_value = ad_names.join("|")
if value.nil? || new_value == DEFAULTS[setting_name.to_sym]
AdPlugin::pstore_delete("ad-setting:#{setting_name}")
AdPlugin.pstore_delete("ad-setting:#{setting_name}")
else
AdPlugin::pstore_set("ad-setting:#{setting_name}", new_value)
AdPlugin.pstore_set("ad-setting:#{setting_name}", new_value)
end
publish_settings
end
def self.publish_settings
MessageBus.publish('/site/house-creatives', settings_and_ads)
MessageBus.publish("/site/house-creatives", settings_and_ads)
end
end
end

View File

@ -9,13 +9,13 @@
register_asset "stylesheets/adplugin.scss"
add_admin_route 'admin.adplugin.house_ads.title', 'houseAds'
add_admin_route "admin.adplugin.house_ads.title", "houseAds"
enabled_site_setting :discourse_adplugin_enabled
module ::AdPlugin
def self.plugin_name
'discourse-adplugin'.freeze
"discourse-adplugin".freeze
end
def self.pstore_get(key)
@ -32,11 +32,11 @@ module ::AdPlugin
end
after_initialize do
require_dependency File.expand_path('../app/models/house_ad', __FILE__)
require_dependency File.expand_path('../app/models/house_ad_setting', __FILE__)
require_dependency File.expand_path('../app/controllers/house_ads_controller', __FILE__)
require_dependency File.expand_path('../app/controllers/house_ad_settings_controller', __FILE__)
require_dependency 'application_controller'
require_dependency File.expand_path("../app/models/house_ad", __FILE__)
require_dependency File.expand_path("../app/models/house_ad_setting", __FILE__)
require_dependency File.expand_path("../app/controllers/house_ads_controller", __FILE__)
require_dependency File.expand_path("../app/controllers/house_ad_settings_controller", __FILE__)
require_dependency "application_controller"
add_to_serializer :site, :house_creatives do
AdPlugin::HouseAdSetting.settings_and_ads
@ -45,7 +45,7 @@ after_initialize do
add_to_serializer :topic_view, :tags_disable_ads do
return false if !SiteSetting.tagging_enabled || !SiteSetting.no_ads_for_tags.present?
return false if object.topic.tags.empty?
!(SiteSetting.no_ads_for_tags.split('|') & object.topic.tags.map(&:name)).empty?
!(SiteSetting.no_ads_for_tags.split("|") & object.topic.tags.map(&:name)).empty?
end
class ::AdstxtController < ::ApplicationController
@ -59,18 +59,18 @@ after_initialize do
end
class AdPlugin::Engine < ::Rails::Engine
engine_name 'adplugin'
engine_name "adplugin"
isolate_namespace AdPlugin
end
AdPlugin::Engine.routes.draw do
root to: 'house_ads#index'
resources :house_creatives, only: [:index, :show, :create, :update, :destroy], controller: 'house_ads'
resources :house_settings, only: [:update], controller: 'house_ad_settings'
root to: "house_ads#index"
resources :house_creatives, only: %i[index show create update destroy], controller: "house_ads"
resources :house_settings, only: [:update], controller: "house_ad_settings"
end
Discourse::Application.routes.append do
get '/ads.txt' => "adstxt#index"
mount ::AdPlugin::Engine, at: '/admin/plugins/pluginad', constraints: AdminConstraint.new
get "/ads.txt" => "adstxt#index"
mount ::AdPlugin::Engine, at: "/admin/plugins/pluginad", constraints: AdminConstraint.new
end
end

View File

@ -1,11 +1,11 @@
# frozen_string_literal: true
require 'rails_helper'
require "rails_helper"
describe AdPlugin::HouseAdSetting do
let(:defaults) { AdPlugin::HouseAdSetting::DEFAULTS }
describe '#all' do
describe "#all" do
subject { AdPlugin::HouseAdSetting.all }
it "returns defaults when nothing has been set" do
@ -13,60 +13,58 @@ describe AdPlugin::HouseAdSetting do
end
it "returns defaults and overrides" do
AdPlugin::pstore_set('ad-setting:topic_list_top', 'Banner')
expect(subject[:topic_list_top]).to eq('Banner')
expect(subject.except(:topic_list_top)).to eq(
defaults.except(:topic_list_top)
)
AdPlugin.pstore_set("ad-setting:topic_list_top", "Banner")
expect(subject[:topic_list_top]).to eq("Banner")
expect(subject.except(:topic_list_top)).to eq(defaults.except(:topic_list_top))
end
end
describe '#update' do
describe "#update" do
before do
AdPlugin::HouseAd.create(name: "Banner", html: "<p>Banner</p>")
AdPlugin::HouseAd.create(name: "Donate", html: "<p>Donate</p>")
end
it "can set override for the first time" do
expect {
AdPlugin::HouseAdSetting.update(:topic_list_top, 'Banner|Donate')
}.to change { PluginStoreRow.count }.by(1)
expect(AdPlugin::HouseAdSetting.all[:topic_list_top]).to eq('Banner|Donate')
expect { AdPlugin::HouseAdSetting.update(:topic_list_top, "Banner|Donate") }.to change {
PluginStoreRow.count
}.by(1)
expect(AdPlugin::HouseAdSetting.all[:topic_list_top]).to eq("Banner|Donate")
end
it "can update an existing override" do
AdPlugin::pstore_set('ad-setting:topic_list_top', 'Banner')
expect {
AdPlugin::HouseAdSetting.update(:topic_list_top, 'Banner|Donate')
}.to_not change { PluginStoreRow.count }
expect(AdPlugin::HouseAdSetting.all[:topic_list_top]).to eq('Banner|Donate')
AdPlugin.pstore_set("ad-setting:topic_list_top", "Banner")
expect { AdPlugin::HouseAdSetting.update(:topic_list_top, "Banner|Donate") }.to_not change {
PluginStoreRow.count
}
expect(AdPlugin::HouseAdSetting.all[:topic_list_top]).to eq("Banner|Donate")
end
it "removes ad names that don't exist" do
AdPlugin::HouseAdSetting.update(:topic_list_top, 'Coupon|Banner|Donate')
expect(AdPlugin::HouseAdSetting.all[:topic_list_top]).to eq('Banner|Donate')
AdPlugin::HouseAdSetting.update(:topic_list_top, "Coupon|Banner|Donate")
expect(AdPlugin::HouseAdSetting.all[:topic_list_top]).to eq("Banner|Donate")
end
it "can reset to default" do
AdPlugin::pstore_set('ad-setting:topic_list_top', 'Banner')
expect {
AdPlugin::HouseAdSetting.update(:topic_list_top, '')
}.to change { PluginStoreRow.count }.by(-1)
expect(AdPlugin::HouseAdSetting.all[:topic_list_top]).to eq('')
AdPlugin.pstore_set("ad-setting:topic_list_top", "Banner")
expect { AdPlugin::HouseAdSetting.update(:topic_list_top, "") }.to change {
PluginStoreRow.count
}.by(-1)
expect(AdPlugin::HouseAdSetting.all[:topic_list_top]).to eq("")
end
it "raises error on invalid setting name" do
expect {
AdPlugin::HouseAdSetting.update(:nope, 'Click Me')
}.to raise_error(Discourse::NotFound)
expect(AdPlugin::pstore_get('ad-setting:nope')).to be_nil
expect { AdPlugin::HouseAdSetting.update(:nope, "Click Me") }.to raise_error(
Discourse::NotFound,
)
expect(AdPlugin.pstore_get("ad-setting:nope")).to be_nil
end
it "raises error on invalid value" do
expect {
AdPlugin::HouseAdSetting.update(:topic_list_top, '<script>')
}.to raise_error(Discourse::InvalidParameters)
expect(AdPlugin::HouseAdSetting.all[:topic_list_top]).to eq('')
expect { AdPlugin::HouseAdSetting.update(:topic_list_top, "<script>") }.to raise_error(
Discourse::InvalidParameters,
)
expect(AdPlugin::HouseAdSetting.all[:topic_list_top]).to eq("")
end
end
end

View File

@ -1,14 +1,17 @@
# frozen_string_literal: true
require 'rails_helper'
require "rails_helper"
describe AdPlugin::HouseAd do
let(:valid_attrs) { {
name: 'Find A Mechanic',
html: '<div class="house-ad find-a-mechanic"><a href="https://mechanics.example.com">Find A Mechanic!</a></div>'
} }
let(:valid_attrs) do
{
name: "Find A Mechanic",
html:
'<div class="house-ad find-a-mechanic"><a href="https://mechanics.example.com">Find A Mechanic!</a></div>',
}
end
describe '#find' do
describe "#find" do
let!(:ad) { AdPlugin::HouseAd.create(valid_attrs) }
it "returns nil if no match" do
@ -22,7 +25,7 @@ describe AdPlugin::HouseAd do
end
end
describe '#all' do
describe "#all" do
it "returns empty array if no records" do
expect(AdPlugin::HouseAd.all).to eq([])
end
@ -44,7 +47,7 @@ describe AdPlugin::HouseAd do
expect(ad.name).to eq(valid_attrs[:name])
expect(ad.html).to eq(valid_attrs[:html])
expect(ad.id.to_i > 0).to eq(true)
ad2 = AdPlugin::HouseAd.from_hash(valid_attrs.merge(name: 'Find Another Mechanic'))
ad2 = AdPlugin::HouseAd.from_hash(valid_attrs.merge(name: "Find Another Mechanic"))
expect(ad2.save).to eq(true)
expect(ad2.id).to_not eq(ad.id)
end
@ -52,18 +55,18 @@ describe AdPlugin::HouseAd do
it "updates existing record" do
ad = AdPlugin::HouseAd.create(valid_attrs)
id = ad.id
ad.name = 'Sell Your Car'
ad.name = "Sell Your Car"
ad.html = '<div class="house-ad">Sell Your Car!</div>'
expect(ad.save).to eq(true)
ad = AdPlugin::HouseAd.find(id)
expect(ad.name).to eq('Sell Your Car')
expect(ad.name).to eq("Sell Your Car")
expect(ad.html).to eq('<div class="house-ad">Sell Your Car!</div>')
expect(ad).to be_valid
end
describe "errors" do
it "blank name" do
ad = AdPlugin::HouseAd.from_hash(valid_attrs.merge(name: ''))
ad = AdPlugin::HouseAd.from_hash(valid_attrs.merge(name: ""))
expect(ad.save).to eq(false)
expect(ad).to_not be_valid
expect(ad.errors.full_messages).to be_present
@ -82,8 +85,8 @@ describe AdPlugin::HouseAd do
end
it "duplicate name, different case" do
existing = AdPlugin::HouseAd.create(valid_attrs.merge(name: 'mechanic'))
ad = AdPlugin::HouseAd.create(valid_attrs.merge(name: 'Mechanic'))
existing = AdPlugin::HouseAd.create(valid_attrs.merge(name: "mechanic"))
ad = AdPlugin::HouseAd.create(valid_attrs.merge(name: "Mechanic"))
expect(ad.save).to eq(false)
expect(ad).to_not be_valid
expect(ad.errors[:name]).to be_present
@ -91,7 +94,7 @@ describe AdPlugin::HouseAd do
end
it "blank html" do
ad = AdPlugin::HouseAd.from_hash(valid_attrs.merge(html: ''))
ad = AdPlugin::HouseAd.from_hash(valid_attrs.merge(html: ""))
expect(ad.save).to eq(false)
expect(ad).to_not be_valid
expect(ad.errors.full_messages).to be_present
@ -100,7 +103,7 @@ describe AdPlugin::HouseAd do
end
it "invalid name" do
ad = AdPlugin::HouseAd.from_hash(valid_attrs.merge(name: '<script>'))
ad = AdPlugin::HouseAd.from_hash(valid_attrs.merge(name: "<script>"))
expect(ad.save).to eq(false)
expect(ad).to_not be_valid
expect(ad.errors[:name]).to be_present
@ -109,7 +112,7 @@ describe AdPlugin::HouseAd do
end
end
describe 'create' do
describe "create" do
it "can create new records" do
ad = AdPlugin::HouseAd.create(valid_attrs)
expect(ad).to be_a(AdPlugin::HouseAd)
@ -119,7 +122,7 @@ describe AdPlugin::HouseAd do
end
it "validates attributes" do
ad = AdPlugin::HouseAd.create(name: '', html: '')
ad = AdPlugin::HouseAd.create(name: "", html: "")
expect(ad).to be_a(AdPlugin::HouseAd)
expect(ad).to_not be_valid
expect(ad.errors.full_messages).to be_present
@ -127,7 +130,7 @@ describe AdPlugin::HouseAd do
end
end
describe 'destroy' do
describe "destroy" do
it "can delete a record" do
ad = AdPlugin::HouseAd.create(valid_attrs)
ad.destroy
@ -135,25 +138,23 @@ describe AdPlugin::HouseAd do
end
end
describe 'update' do
describe "update" do
let(:ad) { AdPlugin::HouseAd.create(valid_attrs) }
it "updates existing record" do
expect(
ad.update(
name: 'Mechanics 4 Hire',
html: '<a href="https://mechanics.example.com">Find A Mechanic!</a>'
)
name: "Mechanics 4 Hire",
html: '<a href="https://mechanics.example.com">Find A Mechanic!</a>',
),
).to eq(true)
after_save = AdPlugin::HouseAd.find(ad.id)
expect(after_save.name).to eq('Mechanics 4 Hire')
expect(after_save.name).to eq("Mechanics 4 Hire")
expect(after_save.html).to eq('<a href="https://mechanics.example.com">Find A Mechanic!</a>')
end
it "validates attributes" do
expect(
ad.update(name: '', html: '')
).to eq(false)
expect(ad.update(name: "", html: "")).to eq(false)
expect(ad).to_not be_valid
expect(ad.errors.full_messages).to be_present
expect(ad.errors.count).to eq(2)

View File

@ -1,46 +1,43 @@
# frozen_string_literal: true
require 'rails_helper'
require "rails_helper"
describe AdPlugin::HouseAdSettingsController do
let(:admin) { Fabricate(:admin) }
before do
AdPlugin::HouseAd.create(name: "Banner", html: "<p>Banner</p>")
end
before { AdPlugin::HouseAd.create(name: "Banner", html: "<p>Banner</p>") }
describe "update" do
let(:valid_params) { { value: 'Banner' } }
let(:valid_params) { { value: "Banner" } }
it "error if not logged in" do
put '/admin/plugins/pluginad/house_settings/topic_list_top.json', params: valid_params
put "/admin/plugins/pluginad/house_settings/topic_list_top.json", params: valid_params
expect(response.status).to eq(404)
end
it "error if not staff" do
sign_in(Fabricate(:user))
put '/admin/plugins/pluginad/house_settings/topic_list_top.json', params: valid_params
put "/admin/plugins/pluginad/house_settings/topic_list_top.json", params: valid_params
expect(response.status).to eq(404)
end
context "when logged in as admin" do
before do
sign_in(admin)
end
before { sign_in(admin) }
it "changes the setting" do
put '/admin/plugins/pluginad/house_settings/topic_list_top.json', params: valid_params
put "/admin/plugins/pluginad/house_settings/topic_list_top.json", params: valid_params
expect(response.status).to eq(200)
expect(AdPlugin::HouseAdSetting.all[:topic_list_top]).to eq(valid_params[:value])
end
it "errors on invalid setting name" do
put '/admin/plugins/pluginad/house_settings/nope-nope.json', params: valid_params
put "/admin/plugins/pluginad/house_settings/nope-nope.json", params: valid_params
expect(response.status).to eq(404)
end
it "errors on invalid setting value" do
put '/admin/plugins/pluginad/house_settings/topic_list_top.json', params: valid_params.merge(value: "Banner|<script>")
put "/admin/plugins/pluginad/house_settings/topic_list_top.json",
params: valid_params.merge(value: "Banner|<script>")
expect(response.status).to eq(400)
end
end