Feature/clh/add standardrb by caseyhelbling · Pull Request #1136 · CommitChange/houdini

caseyhelbling · 2025-04-23T03:57:05Z

NOTE: DO NOT discuss internal CommitChange information in your PR; this PR will be public.
Link back to the issue in the Tix repo when you need to do that.

Adds standardrb. Runs standardrb --fix, and fix-unsafely. Then fix all the items it couldn't fix by hand. Then update specs to passing.

app/mailers/testing.rb

…est env

caseyhelbling

Comments on why the changes are made. Some questions too... More to come.

caseyhelbling · 2025-04-24T03:07:20Z

app/controllers/campaigns_controller.rb

      params[:campaign][:end_datetime] = Chronic.parse(params[:campaign][:end_datetime]) if params[:campaign][:end_datetime].present?
    end
-    current_campaign.update_attributes params[:campaign]
+    current_campaign.update params[:campaign]


Standard wants you to call update instead of update_attributes.

caseyhelbling · 2025-04-24T03:08:38Z

app/controllers/concerns/controllers/api_new/nonprofit/current.rb

-			end
+    def current_nonprofit
+      result = Nonprofit.find_by(houid: params[:nonprofit_id])
+      if Rails.version < "5" && result.nil?


Seems like we could remove this.

We can. Please remove as a separate PR.

app/controllers/events_controller.rb

caseyhelbling · 2025-04-24T03:10:26Z

app/controllers/image_attachments_controller.rb

-		end
-	end
+  def remove
+    @image = ImageAttachment.find { |img| img.file_url == params[:src] }


find instead of select . first

app/controllers/maps_controller.rb

caseyhelbling · 2025-04-24T20:30:07Z

app/legacy_lib/periodic_report_adapter/cancelled_recurring_donations_report.rb

-      :cancelled_at_gt_or_eq => (Time.current - 1.month).beginning_of_month,
-      :cancelled_at_lt => Time.current.beginning_of_month
+      cancelled_at_gt_or_eq: 1.month.ago.beginning_of_month,
+      cancelled_at_lt: Time.current.beginning_of_month


surprised this didn't update to Time.zone.current...

caseyhelbling · 2025-04-24T20:32:04Z

app/legacy_lib/psql.rb

  def self.execute(statement)
-    puts statement if ENV['RAILS_ENV'] != 'production' && ENV['RAILS_LOG_LEVEL'] == 'debug' # log to STDOUT on dev/staging
-    return Qx.execute_raw(raw_expr_str(statement))
+    Rails.logger.debug statement if ENV["RAILS_ENV"] != "production" && ENV["RAILS_LOG_LEVEL"] == "debug" # log to STDOUT on dev/staging


I'm not entirely sure what this line is supposed to be doing... I assume logging to debug is fine, though.

Your guess is as good as mine

caseyhelbling · 2025-04-24T20:34:38Z

app/legacy_lib/qexpr.rb

-  def update(table_name, settings, os={})
-    Qexpr.new @tree.put(:update, "UPDATE".bold.light_blue + " #{table_name}".blue + "\nSET".bold.light_blue + " #{settings.map{|key,val| "#{key.to_s}=#{Qexpr.quote(val)}"}.join(', ')}".blue)
+  def update(table_name, settings, os = {})
+    Qexpr.new @tree.put(:update, "UPDATE".bold.light_blue + " #{table_name}".blue + "\nSET".bold.light_blue + " #{settings.map { |key, val| "#{key}=#{Qexpr.quote(val)}" }.join(", ")}".blue)


I'm curious what all this .bold and .light_blue and .bue stuff is intended to do ...

caseyhelbling · 2025-04-24T21:06:37Z

app/legacy_lib/query_roles.rb

+    Qx.select("host_id").from(:roles)
+      .where(user_id: user_id)
+      .and_where("roles.name IN ($names)", names: role_names)
+      .execute.pluck("host_id")


Is pluck ok here?

Based on the code of pluck, I'd say it is.

caseyhelbling · 2025-04-24T21:11:39Z

app/legacy_lib/query_users.rb

      .and_where("nonprofits.id=$id", id: np_id)
      .group_by("users.email")
-      .execute.map{|h| h['email']}
+      .execute.pluck("email")


Is pluck ok here?

caseyhelbling

Reviewed All the viles and left comments where I think little more looking is needed.

caseyhelbling · 2025-04-24T21:15:54Z

app/legacy_lib/search_vector.rb

 # License: AGPL-3.0-or-later WITH Web-Template-Output-Additional-Permission-3.0-or-later

 module SearchVector
+  ACCEPTED_TABLES = ["supporters", "payments"]


Unused. Ithink.

It does appear to be.

caseyhelbling · 2025-04-24T21:19:17Z

app/legacy_lib/update_donation.rb

      event = nil
    else
-      event = Event.where('id = ?', data[:event_id]).first
+      event = Event.where(id: data[:event_id]).first


possibly find_by(id: would be better.

It is, please switch to find_by

caseyhelbling · 2025-04-24T21:20:43Z

app/legacy_lib/update_payouts.rb

    if payment_ids.count < 1
      raise ArgumentError.new("No payments are available to reverse.")
    end
-    now = Time.current
+    Time.current


maybe this isn't needd.

caseyhelbling · 2025-04-24T21:21:04Z

app/legacy_lib/update_recurring_donations.rb

      donation.card = tokenizable
      rec_don.card_id = tokenizable

      rec_don.n_failures = 0
      rec_don.save!
      donation.save!
-      rec_don.supporter.supporter_notes.create!( content: "This supporter updated their card for their recurring donation with ID #{rec_don.id}", user: User.find(540))
+      rec_don.supporter.supporter_notes.create!(content: "This supporter updated their card for their recurring donation with ID #{rec_don.id}", user: User.find(540))


Magic user 540 again...

caseyhelbling · 2025-04-24T21:34:28Z

app/models/campaign_gift.rb

-	belongs_to :campaign_gift_option
-
-	validates :donation, presence: true
-	validates :campaign_gift_option, presence: true


implicit with belongs_to now.

caseyhelbling · 2025-04-25T01:08:32Z

app/controllers/nonprofits/supporters_controller.rb

    def bulk_delete
      supporter_ids = if params[:selecting_all]
-        QuerySupporters.full_filter_expr(current_nonprofit.id, params[:query]).select("supporters.id").execute.map { |h| h["id"] }
+        QuerySupporters.full_filter_expr(current_nonprofit.id, params[:query]).select("supporters.id").execute.pluck("id")


It would be ideal if we had a spec to verify this Controller action

caseyhelbling · 2025-04-25T01:08:40Z

app/controllers/nonprofits/tag_joins_controller.rb

    def modify
      supporter_ids = if params[:selecting_all]
-        QuerySupporters.full_filter_expr(current_nonprofit.id, params[:query]).select("supporters.id").execute.map { |h| h["id"] }
+        QuerySupporters.full_filter_expr(current_nonprofit.id, params[:query]).select("supporters.id").execute.pluck("id")


It would be ideal if we had a spec to verify this Controller action

gems/ruby-param-validation/lib/param_validation.rb

caseyhelbling · 2025-04-25T01:16:47Z

gems/ruby-param-validation/lib/param_validation.rb

-      msg = errors.collect { |e| e[:msg] }.join('\n')
-      raise ValidationError.new(msg, errors.collect { |e| e[:data] })
+      msg = errors.pluck(:msg).join('\n')
+      raise ValidationError.new(msg, errors.pluck(:data))


db/migrate/20171026102139_add_direct_debit_detail.rb

wwahammy

Thanks @caseyhelbling! Sorry about delay of a review, I had thought I had finished and submitted the review but hadn't.

Mostly good but please review the places you removed the presence validation for belong_to relations. As I explain in one of the comments, we have automatic presence checking for belongs_to turned off.

Another comment:

Please ignore the comments I made about the Stripe date time issue. I realized later that Time.at(integer) == Time.zone.at(integer) is true which is what I was most worried about.

Lastly, we'll need to figure out what is going on with the specs that are newly failing. Let me know if you need help with anything of them.

EDIT:

Oh, one more thing: please undo the changes to the migrations, it seems like bad practice to modify them. It would be wiser, on a seperate PR, to squash them into just the schema and remove the old migrations.

app/controllers/campaigns/donations_controller.rb

wwahammy · 2025-04-28T23:50:20Z

app/controllers/concerns/controllers/api_new/jbuilder_expansions.rb

+    def self.create_from(root_tree_node)
+      er = ExpansionTree.new
+      er.root_node = root_tree_node
+      er
+    end
+


What happened here? Why is this public instead of private?

wwahammy · 2025-04-28T23:53:07Z

app/controllers/nonprofits/custom_field_joins_controller.rb


      supporter_ids = if params[:selecting_all]
-        QuerySupporters.full_filter_expr(current_nonprofit.id, params[:query]).select("supporters.id").execute.map { |h| h["id"] }
+        QuerySupporters.full_filter_expr(current_nonprofit.id, params[:query]).select("supporters.id").execute.pluck("id")


I think it will. It would be nice if all of these were tested though.

wwahammy · 2025-04-28T23:53:45Z

app/controllers/nonprofits/supporters_controller.rb

    include Controllers::NonprofitHelper

-    before_action :authenticate_nonprofit_user!, except: [:new, :create]
+    before_action :authenticate_nonprofit_user!, except: [:create]


Why the change?

wwahammy · 2025-04-28T23:54:13Z

app/controllers/nonprofits/supporters_controller.rb

    def bulk_delete
      supporter_ids = if params[:selecting_all]
-        QuerySupporters.full_filter_expr(current_nonprofit.id, params[:query]).select("supporters.id").execute.map { |h| h["id"] }
+        QuerySupporters.full_filter_expr(current_nonprofit.id, params[:query]).select("supporters.id").execute.pluck("id")


It would be ideal if we had a spec to verify this Controller action

wwahammy · 2025-05-17T18:17:02Z

spec/lib/export/export_payments_spec.rb

 require "rails_helper"
 require "support/test_chunked_uploader"

+# TODO: 6 tests marked pending that do not pass in the RAILS_ENV=ci but do in test


wwahammy · 2025-05-17T18:18:24Z

spec/lib/export/export_payments_spec.rb

 require "rails_helper"
 require "support/test_chunked_uploader"

+# TODO: 6 tests marked pending that do not pass in the RAILS_ENV=ci but do in test


I'd rather we keep the same set of tests in test and CI so it would be good to roll these back and figure out what's going on.

wwahammy · 2025-05-18T17:36:38Z

app/controllers/nonprofits_controller.rb

-      countries = countries.map{ |code, name| [code.upcase, name] }.sort{ |a, b| a[1] <=> b[1] }
-      countries.push([Settings.intntl.other_country.upcase, I18n.t('nonprofits.donate.info.supporter.other_country')]) if Settings.intntl.other_country
+      countries = all_countries.slice(*Settings.intntl.all_countries)
+      countries = countries.map { |code, name| [code.upcase, name] }.sort_by { |a| a[1] }


What about sort_by?

wwahammy · 2025-05-18T17:39:15Z

app/controllers/static_controller.rb

-      end
-    elsif (ccs_method == 'github')
-      git_hash = File.read("#{Rails.root}/CCS_HASH")
+    if Settings.ccs&.ccs_method.presence == "github"


That looks right

wwahammy · 2025-05-18T17:41:59Z

spec/lib/fetch/fetch_misc_nonprofit_settings_spec.rb


      # TODO: figure out what this test is supposed to be doing
-      it "returns hash with empty misc settings" do
+      xit "returns hash with empty misc settings" do


If this previously worked, we'll need to get it working before merging.

caseyhelbling requested a review from wwahammy as a code owner April 23, 2025 03:57

caseyhelbling force-pushed the feature/clh/add-standardrb branch from db93d17 to 0a8d328 Compare April 23, 2025 04:06

caseyhelbling added 12 commits April 23, 2025 11:19

standardrb fix unsafely

da64de8

Fix static controller spec

0d3dd77

Adds standard rails gem

91845e6

fix safely

d3d2ae2

Fix unsafely

9f67723

Fix remaining lint errors

2c50490

Get more tests passing

4338657

Get more tests passing

8334741

Fix more tests

0a4513d

More test fixes

b43686d

Fix bad rebase

d4b0ef8

Fix call signature

d36a8b1

caseyhelbling force-pushed the feature/clh/add-standardrb branch from 0a8d328 to d36a8b1 Compare April 23, 2025 16:39

Fix file name

88cd90a

wwahammy reviewed Apr 23, 2025

View reviewed changes

app/mailers/testing.rb Outdated Show resolved Hide resolved

Mark some tests pending that don't passing in CI env but do pass in t…

e3c6f28

…est env

caseyhelbling commented Apr 24, 2025

View reviewed changes

caseyhelbling commented Apr 25, 2025

View reviewed changes

This was referenced Apr 28, 2025

Run bundle exec standardrb --fix #1145

Closed

Add standard and standard-rails gems #1148

Closed

Run bin/standardrb --fix #1149

Closed

wwahammy changed the base branch from to-ruby-3.2 to run-standardrb April 28, 2025 23:34

wwahammy added 3 commits April 28, 2025 18:35

Merge branch 'run-standardrb' into feature/clh/add-standardrb

f3bc07f

Merge branch 'run-standardrb' into feature/clh/add-standardrb

9203ce9

Merge branch 'run-standardrb' into feature/clh/add-standardrb

7ac3eef

Base automatically changed from run-standardrb to supporter_level_goal April 30, 2025 15:19

Merge branch 'supporter_level_goal' into feature/clh/add-standardrb

c2963cf

wwahammy mentioned this pull request Apr 30, 2025

Add standard-rails and run standardrb --fix #1161

Merged

wwahammy changed the base branch from supporter_level_goal to add-standard.yml April 30, 2025 17:10

Merge branch 'add-standard.yml' into feature/clh/add-standardrb

aa35d11

wwahammy force-pushed the feature/clh/add-standardrb branch from ba9554a to aa35d11 Compare April 30, 2025 17:11

Base automatically changed from add-standard.yml to supporter_level_goal April 30, 2025 20:47

wwahammy added 3 commits April 30, 2025 15:49

Merge branch 'supporter_level_goal' into feature/clh/add-standardrb

82dd5bb

Merge branch 'supporter_level_goal' into feature/clh/add-standardrb

5234251

Merge branch 'supporter_level_goal' into feature/clh/add-standardrb

056babe

wwahammy requested changes May 18, 2025

View reviewed changes

wwahammy added 3 commits May 18, 2025 12:53

Switch two files to belongs_to optional: false

093b8a8

Merge branch 'supporter_level_goal' into feature/clh/add-standardrb

16fe3b3

Merge branch 'supporter_level_goal' into feature/clh/add-standardrb

9f86ab6

Conversation

caseyhelbling commented Apr 23, 2025

Uh oh!

Uh oh!

caseyhelbling left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caseyhelbling left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wwahammy Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wwahammy Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wwahammy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wwahammy Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

wwahammy Apr 28, 2025 •

edited

Loading

wwahammy Apr 28, 2025 •

edited

Loading

wwahammy left a comment •

edited

Loading

wwahammy Apr 28, 2025 •

edited

Loading

wwahammy May 17, 2025 •

edited

Loading