Skip to content
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

HOTT-4931 Drop CDS Env Var #1716

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
82e8782
remove use_cds and update tests and code so CDS Sync and Taric Sync w…
HWallenberg Feb 8, 2024
968a1fb
Fix linting errors
HWallenberg Feb 8, 2024
0742c26
Moved DataMigration out of foreach loop
HWallenberg Feb 8, 2024
497bae1
Update specs to use service and added exception handler for detecting…
HWallenberg Feb 13, 2024
76a3715
specs updated for uk worker on a taric process exception handling
HWallenberg Feb 13, 2024
9212de5
Fixed rollback tests and linting
HWallenberg Feb 13, 2024
ef78254
fixed linting error
HWallenberg Feb 13, 2024
0f21b3f
Removed Taric tests from cds_synchronizer and added wrong environment…
HWallenberg Feb 14, 2024
c24b2fb
Tidied up wrong environment error specs for Taric
HWallenberg Feb 14, 2024
dafd070
tidied up taric_synchronizer, added wrong environment code and specs …
HWallenberg Feb 14, 2024
380a5b6
changed CDS_synchronizer back the way it was and updated the spec wit…
HWallenberg Feb 23, 2024
7629b1b
Renamed uk rollback controller spec back to generic name and removed …
HWallenberg Feb 23, 2024
97d0c12
Fixed linting error
HWallenberg Feb 27, 2024
54911ac
Merged Main to Feature Branch
HWallenberg Mar 7, 2024
b3c6771
fix specs following merge conflict resolution
HWallenberg Mar 7, 2024
f58d0f1
removed CDS from terraform/admin
HWallenberg Mar 7, 2024
007a649
Taric Importer uses Create_Update_Entry for Taric_Synchronizer_spec a…
HWallenberg Mar 7, 2024
48202cc
HOTT-4931 Removed convoluted dead code
jebw Mar 8, 2024
f34ed5f
HOTT-4931 Lint fixes
jebw Mar 8, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion app/lib/cds_synchronizer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ def download
end

def apply
raise WrongEnvironmentError unless TradeTariffBackend.uk?

check_tariff_updates_failures
check_sequence

Expand Down Expand Up @@ -64,14 +66,15 @@ def apply
end

def rollback(rollback_date, keep: false)
raise WrongEnvironmentError unless TradeTariffBackend.uk?

Rails.autoloaders.main.eager_load

TradeTariffBackend.with_redis_lock do
date = Date.parse(rollback_date.to_s)

updates = TariffSynchronizer::CdsUpdate.where { issue_date > date }
update_filenames = updates.pluck(:filename)

Sequel::Model.db.transaction do
# Delete actual data
oplog_based_models.each do |model|
Expand Down
13 changes: 9 additions & 4 deletions app/lib/taric_importer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ def import
return unless proceed_with_import?(filename)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These changes may not be required as Taric Updates and importing is processed in app/lib/tariff_synchronizer/taric_update_downloader.rb. It could be that this code is actually redundant now...


handler = XmlProcessor.new(@taric_update.issue_date)

file = TariffSynchronizer::FileService.file_as_stringio(@taric_update)
XmlParser::Reader.new(file, 'record', handler).parse
post_import(file_path: @taric_update.file_path, filename:)
Expand Down Expand Up @@ -58,19 +59,23 @@ def taric_failed_log(exception, hash)
private

def proceed_with_import?(filename)
Copy link
Contributor

Choose a reason for hiding this comment

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

This check always returns true (because use_cds? is false)

So the method can go

return true unless TradeTariffBackend.use_cds?

TariffSynchronizer::TaricUpdate.find(filename: filename[0, 30]).blank?
end

def post_import(file_path:, filename:)
create_update_entry(file_path:, filename:) if TradeTariffBackend.use_cds?
create_update_entry(file_path:, filename:)
Rails.logger.info "Successfully imported Taric file: #{@taric_update.filename}"
end

def create_update_entry(file_path:, filename:)
file_size = determine_file_size(file_path)
issue_date = Date.parse(filename.scan(/[0-9]{8}/).last)
# handle a filename that doesn't have a date in the expected format of YYYY-MM-DD
date_match = filename.scan(/[0-9_-]{10}/).last
issue_date = if date_match
Date.parse(date_match)
else
Time.zone.today
end
TariffSynchronizer::TaricUpdate.find_or_create(
filename: filename[0, 30],
issue_date:,
Expand Down
6 changes: 5 additions & 1 deletion app/lib/taric_synchronizer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ def download
end

def apply
raise WrongEnvironmentError unless TradeTariffBackend.xi?

check_tariff_updates_failures
check_sequence

Expand Down Expand Up @@ -78,6 +80,8 @@ def apply
#
# NOTE: this does not remove records from initial seed
def rollback(rollback_date, keep: false)
raise WrongEnvironmentError unless TradeTariffBackend.xi?

TradeTariffBackend.with_redis_lock do
date = Date.parse(rollback_date.to_s)

Expand Down Expand Up @@ -122,7 +126,7 @@ def rollback(rollback_date, keep: false)
end
end

Rails.logger.info "Rolled back to #{date}. Forced keeping records: #{!!keep}"
Rails.logger.info "Rolled back to #{date}. Forced keeping records: #{!keep.nil?}"
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks odd? means that keep == false will print 'Force keeping records: true' , as will keep == true

end
rescue Redlock::LockError
Rails.logger.warn("Failed to acquire Redis lock for rollback to #{rollback_date}. Keep records: #{keep}")
Expand Down
1 change: 1 addition & 0 deletions app/lib/tariff_synchronizer.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
module TariffSynchronizer
class FailedUpdatesError < StandardError; end
class WrongEnvironmentError < StandardError; end

delegate :instrument, :subscribe, to: ActiveSupport::Notifications

Expand Down
6 changes: 1 addition & 5 deletions app/lib/trade_tariff_backend.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,6 @@ def admin_email
ENV.fetch('TARIFF_SYNC_EMAIL')
end

def use_cds?
ENV['CDS'] == 'true'
end

def patch_broken_taric_downloads?
ENV['PATCH_BROKEN_TARIC_DOWNLOADS'] == 'true'
end
Expand Down Expand Up @@ -168,7 +164,7 @@ def check_query_count?
end

def excess_query_threshold
@excess_query_threshold ||= ENV['EXCESS_QUERY_THRESHOLD'].presence&.to_i || 0
@excess_query_threshold ||= ENV['EXCESS_QUERY_THRESHOLD'].presence.to_i
end

def api_version(request)
Expand Down
2 changes: 1 addition & 1 deletion app/workers/apply_worker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ class ApplyWorker
sidekiq_options queue: :rollbacks, retry: false

def perform
if TradeTariffBackend.use_cds?
if TradeTariffBackend.uk?
CdsSynchronizer.apply
else
TaricSynchronizer.apply
Expand Down
2 changes: 1 addition & 1 deletion app/workers/download_worker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ class DownloadWorker
sidekiq_options queue: :rollbacks, retry: false

def perform
if TradeTariffBackend.use_cds?
if TradeTariffBackend.uk?
CdsSynchronizer.download
else
TaricSynchronizer.download
Expand Down
2 changes: 1 addition & 1 deletion app/workers/rollback_worker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ class RollbackWorker
sidekiq_options queue: :rollbacks, retry: false

def perform(date, redownload = false)
if TradeTariffBackend.use_cds?
if TradeTariffBackend.uk?
CdsSynchronizer.rollback(date, keep: redownload)
else
TaricSynchronizer.rollback(date, keep: redownload)
Expand Down
8 changes: 4 additions & 4 deletions lib/tasks/tariff.rake
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ namespace :tariff do
namespace :sync do
desc 'Update database by downloading and then applying TARIC or CDS updates via worker'
task download_apply_and_reindex: %i[environment class_eager_load] do
if TradeTariffBackend.use_cds?
if TradeTariffBackend.uk?
CdsUpdatesSynchronizerWorker.perform_async(true, true)
else
TaricUpdatesSynchronizerWorker.perform_async(true)
Expand All @@ -42,7 +42,7 @@ namespace :tariff do

desc 'Download pending Taric or CDS update files, Update tariff_updates table'
task download: %i[environment class_eager_load] do
if TradeTariffBackend.use_cds?
if TradeTariffBackend.uk?
CdsSynchronizer.download
else
TaricSynchronizer.download
Expand All @@ -51,7 +51,7 @@ namespace :tariff do

desc 'Apply pending updates for Taric or CDS'
task apply: %i[environment class_eager_load] do
if TradeTariffBackend.use_cds?
if TradeTariffBackend.uk?
CdsSynchronizer.apply
else
TaricSynchronizer.apply
Expand All @@ -61,7 +61,7 @@ namespace :tariff do
desc 'Rollback to specific date in the past'
task rollback: %w[environment class_eager_load] do
if ENV['DATE']
if TradeTariffBackend.use_cds?
if TradeTariffBackend.uk?
CdsSynchronizer.rollback(ENV['DATE'], keep: ENV['KEEP'])
else
TaricSynchronizer.rollback(ENV['DATE'], keep: ENV['KEEP'])
Expand Down
12 changes: 3 additions & 9 deletions spec/controllers/api/admin/rollbacks_controller_spec.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
RSpec.describe Api::Admin::RollbacksController do
describe 'POST to #create' do
before { login_as_api_user }
before do
login_as_api_user
end

let(:rollback_attributes) { attributes_for :rollback }
let(:record) do
Expand All @@ -16,14 +18,6 @@
expect(response.status).to eq 201
expect(response.location).to eq api_rollbacks_url
end

it 'performs a rollback' do
Sidekiq::Testing.inline! do
expect {
create(:rollback, date: 1.month.ago.beginning_of_day)
}.to change(Measure, :count).from(1).to(0)
end
end
end

context 'when rollback is not valid' do
Expand Down
6 changes: 6 additions & 0 deletions spec/factories/cds_update_factory.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,11 @@
trait :failed do
state { 'F' }
end

trait :with_measure do
after :create do |cds_update, _evaluator|
create :measure, operation_date: cds_update.issue_date, filename: cds_update.filename
end
end
end
end
2 changes: 1 addition & 1 deletion spec/formatters/description_formatter_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@

context 'when xi' do
before do
allow(TradeTariffBackend).to receive(:uk?).and_return(false)
allow(TradeTariffBackend).to receive(:service).and_return('xi')
end

it 'does not replace , with .' do
Expand Down
1 change: 1 addition & 0 deletions spec/integration/taric_synchronizer/rollback_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
first_rolled_back_update
second_rolled_back_update
non_rolled_back_update
allow(TradeTariffBackend).to receive(:service).and_return('xi')
end

it { is_expected.to have_rolled_back(first_rolled_back_update) }
Expand Down