Skip to content

Conversation

@thejonroberts
Copy link
Contributor

@thejonroberts thejonroberts commented Jul 23, 2024

Resolves #734

Specs are all green via script/backend rspec.

New to the project, not sure how deployment works, but I assumed there are no rolling deploys involved. If there are seperate server instances, this commit will need to be removed, then re-added after deployment.

One annoying thing is deprecation warnings in specs (9 total):

DEPRECATION WARNING: `Rails.application.secrets` is deprecated in favor of 
`Rails.application.credentials` and will be removed in Rails 7.2. 
(called from remind at /app/app/mailers/checkin_reminder_mailer.rb:16)

I think it is out of scope here, and I don't know how prod env is handled -- but will need to migrate from secrets to credentials before upgrading to 7.2. (Warning gem could suppress warnings while running specs til then).

Other than than, I think this is ready.

@thejonroberts thejonroberts force-pushed the upgrade-to-rails-7-thejonroberts branch from eadbbd9 to d0eef51 Compare July 23, 2024 19:08
it { is_expected.to validate_inclusion_of(:trackable_type).in_array(%w[Condition Symptom Treatment]) }

context "without foreign key checks" do
subject { create :tracking, :for_condition, :active }
Copy link
Contributor Author

@thejonroberts thejonroberts Jul 23, 2024

Choose a reason for hiding this comment

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

Fixed an error during upgrade, see shoulda-matchers issue.

end
context "on a past checkin" do
before { checkin.update!(date: Time.zone.today - 1.day) }
let(:checkin) { create(:checkin, user_id: user.id, date: Time.zone.today - 1.day) }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

update! in the before {} caused an lazy loading error (from Bullet), use a lazy let instead.

"postal_code" => "55403"
}
Geocoder::Lookup::Test.set_default_stub([minneapolis_geo])

Copy link
Contributor Author

@thejonroberts thejonroberts Jul 23, 2024

Choose a reason for hiding this comment

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

I had to do this before any updates, spec failed consistently because position would not save/validate as its geocoder position was nil

# backend/app/models/position.rb:21
  def geocoder_position
    @geocoder_position ||= Geocoder.search(postal_code).first
  end

Caused more issues until I matched location info to the Weather lookup VCR stub

module Concerns
end
end
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needed to autoload (zeitwerk) api/v1/concerns. Concerns::Notificatable wasn't working in comment/post serializer, for example.

# uri: mongodb://user:password@mongodb.domain.com:27017/flaredown_development

# Otherwise define the parameters separately.
# This defines the name of the default database that Mongoid can connect to.
Copy link
Contributor Author

@thejonroberts thejonroberts Jul 23, 2024

Choose a reason for hiding this comment

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

Changes in this file are all comments replacing comments. They are all from running rails g mongoid:config after upgrading Mongoid.


it { is_expected.to respond_to(:value) }
it { is_expected.to validate_inclusion_of(:value).to_allow(0..4) }
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Conditional validate in app/models/checkin/fiveable.rb concern: validates :value, inclusion: {in: (0..4)}, if: -> { value.present? } was the problem. I created this shared example while trying to fix it.

I like the it_behaves_like pattern for concerns, but can revert this shared example change if needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

looks good!

Bullet.console = true
# Bullet.growl = true
Bullet.rails_logger = true
Bullet.add_footer = 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.

Moved from top of file - config.after_initialize Bullet setup was done twice here.

# "info" includes generic and useful information about system operation, but avoids logging too much
# information to avoid inadvertent exposure of personally identifiable information (PII). If you
# want to log everything, set the level to "debug".
config.log_level = ENV.fetch("RAILS_LOG_LEVEL", "debug")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rails default is "info" but was previously set to debug in removed L51.

config.action_mailer.delivery_method = :test

# Randomize the order test cases are executed.
config.active_support.test_order = :random
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left this removed because it was not doing what it said - this only affects minitest.

I added --order random to the backend/.rspec and did see some failing tests!

@thejonroberts thejonroberts marked this pull request as ready for review July 24, 2024 04:19
@thejonroberts thejonroberts changed the title Upgrade to rails 7 thejonroberts Upgrade to rails 7 Aug 9, 2024
@compwron
Copy link
Collaborator

compwron commented Sep 22, 2024

wow! will review

@compwron compwron merged commit eef6abd into rubyforgood:master Sep 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Upgrade to Rails 7

2 participants