-
Notifications
You must be signed in to change notification settings - Fork 2
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
FFS-2408: Rename site_id to client_agency_id #446
Conversation
raise ArgumentError.new("Site missing id") if @id.blank? | ||
raise ArgumentError.new("Site #{@id} missing required attribute `agency_name`") if @agency_name.blank? | ||
raise ArgumentError.new("Site #{@id} missing required attribute `pinwheel.environment`") if @pinwheel_environment.blank? | ||
raise ArgumentError.new("Client Agency missing id") if @id.blank? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ehh... I might need to walk this back and double check if it required translation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully we never see these errors! (So I don't think they need translation.)
|
||
# Then handle the index - Rails will automatically handle the reversal | ||
remove_index :users, [:email, :site_id] if index_exists?(:users, [:email, :site_id]) | ||
add_index :users, [:email, :client_agency_id], unique: true unless index_exists?(:users, [:email, :client_agency_id]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's some weirdness going on here. I really shouldn't need the true unless index_exists?
. I'm planning on resetting the db and inspecting the schema up til this point- prior to this migration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, that's a lot of find-and-replace. Nice work.
<%= link_to t("cbv.employer_searches.show.review_button_text"), cbv_flow_summary_path, class: "usa-button usa-button--outline margin-top-5", data: { turbo_frame: "_top" } %> | ||
<% else %> | ||
<%= link_to t("cbv.employer_searches.show.exit_button_text", agency_short_name: current_agency.agency_short_name), current_agency.agency_contact_website, class: "usa-button usa-button--outline margin-top-5", data: { turbo_frame: "_top" } %> | ||
<% end %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of strange indenting changes in these ERB files (this file, the previous one, cbv/summaries/show.html.erb to name a fiew). Might be worth checking your IDE settings if there is some setting about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's definitely strange and inconsistent. I've added rubocop to my "pre-test" commands so hopefully it'll capture and cleanup some stuff. I've been having issues with Rubymine or Cursor automatically fixing these changes.
raise ArgumentError.new("Site missing id") if @id.blank? | ||
raise ArgumentError.new("Site #{@id} missing required attribute `agency_name`") if @agency_name.blank? | ||
raise ArgumentError.new("Site #{@id} missing required attribute `pinwheel.environment`") if @pinwheel_environment.blank? | ||
raise ArgumentError.new("Client Agency missing id") if @id.blank? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully we never see these errors! (So I don't think they need translation.)
sign_in nyc_user | ||
end | ||
|
||
it "sends an invitation" do | ||
post :create, params: { | ||
site_id: 'nyc', | ||
client_agency_id: 'nyc', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah something weird is going on with your indenting... I'm surprised Rubocop didn't catch this one.
…ow-use-new-database * origin/main: FFS-2408: Rename site_id to client_agency_id (#446) FFS-2351: Change calculation of account_count to be correct in filed events (#445) Patching vulnerabilities | esbuild, net-imap (#450) Address vulnerability (#443) fix: upgrade postcss from 8.5.0 to 8.5.1 (#441) fix: upgrade sass from 1.83.2 to 1.83.4 (#440) 2401: Use different syntax (#437) updated tests for clarity fixed typo cleaned up based on PR comments removed trailing whitespace/rubocop fixes wrapped token creation for users.rake in a transaction, added test assertion to assure that the user api_access_tokens.count only changes by a factor of 1 updated vitest to v 3.0.5 address failed security scan rename vitest add github action fixed postcss build error added .vitest to git ignore updated package.json to use module setup project to work with vitest and debugging remove employer_search.spec comment out test write test scripts for pinwheel.js update apiservice to fetchInternalApiService for clarity refactored fetch into its own file refactored code to be a little more self evident remove outdated snapshots comment refactored api calls with tests added tests for trackUserAction api call move trackUserAction out of pinwheel into analytics file minor changes stub for employersearch test installed vitest
Resolves FFS-2408.
Changes
The objective of this PR is rename
site_id
toclient_agency_id
throughout the codebase. There are many ancillary services, comments, and helper classes which also share a similar naming convention. These have been renamed as well.site_id
toclient_agency_id
current_site
has been renamed tocurrent_agency
site_translation
has been renamed toclient_agency_translation
site_config
has been renamed toagency_config
site_id
related columnsContext for reviewers
Acceptance testing
:alert: Deploy block! @ffs-eng I just merged PR [#123] and will be doing acceptance testing in demo - please don't deploy until I'm finished!
)