-
Notifications
You must be signed in to change notification settings - Fork 6
Allow sending set of provider-specific CSV files #420 (reopened) #458
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| class FileUploadJob < ApplicationJob | ||
| # Consider removing concurrency limits due to SolidQueue blocking issues | ||
| # or use a more specific key to avoid blocking all jobs for a language | ||
| limits_concurrency to: 3, key: ->(_language_id, content_id, _content_type) { "hard-limit" } | ||
| limits_concurrency to: 3, key: ->(*args) { "hard-limit" } | ||
|
|
||
| retry_on AzureFileShares::Errors::ApiError, wait: :exponentially_longer, attempts: 3 | ||
| retry_on Timeout::Error, wait: :exponentially_longer, attempts: 2 | ||
|
|
@@ -12,34 +12,34 @@ class FileUploadJob < ApplicationJob | |
| Rails.logger.error "Suggestion: Check provider names for invalid characters if Azure API errors" | ||
| end | ||
|
|
||
| def perform(language_id, content_id, content_type, share = ENV["AZURE_STORAGE_SHARE_NAME"]) | ||
| def perform(language_id, file_id, provider_id = nil, share = ENV["AZURE_STORAGE_SHARE_NAME"]) | ||
| @language = Language.find(language_id) | ||
| @processor = LanguageContentProcessor.new(language) | ||
| @share = share | ||
| @file_id = file_id.to_sym | ||
| @processor = LanguageContentProcessor.new(language) | ||
|
|
||
| send_provider_content(content_id) if content_type == "provider" | ||
| send_language_content(content_id.to_sym) if content_type == "file" | ||
| send_provider_content(provider_id) if provider_id.present? | ||
| send_language_content if provider_id.blank? | ||
| end | ||
|
|
||
| private | ||
|
|
||
| attr_reader :language, :processor, :share | ||
| attr_reader :language, :file_id, :share, :processor | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick: 🔤 would make the diff a little easier.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please explain what would make the diff easier? And what to you mean by "diff" here? |
||
|
|
||
| def send_provider_content(provider_id) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider keyword so you throw an error if provider_id is not defined?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why throwing an error? |
||
| provider = language.providers.find(provider_id) | ||
| return unless provider | ||
|
|
||
| processor.provider_files.each do |file| | ||
| FileWorker.new( | ||
| share:, | ||
| name: file.name[provider], | ||
| path: file.path, | ||
| file: file.content[provider], | ||
| ).send | ||
| end | ||
| file = processor.provider_files[file_id] | ||
| return unless provider && file | ||
|
|
||
| FileWorker.new( | ||
| share:, | ||
| name: file.name[provider], | ||
| path: file.path, | ||
| file: file.content[provider], | ||
| ).send | ||
| end | ||
|
|
||
| def send_language_content(file_id) | ||
| def send_language_content | ||
| file = processor.language_files[file_id] | ||
| return unless file | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,18 +1,11 @@ | ||
| class CsvGenerator::TopicAuthors < CsvGenerator::Base | ||
| def initialize(language, **args) | ||
| @language = language | ||
| @args = args | ||
| end | ||
|
|
||
| private | ||
|
|
||
| attr_reader :language, :args | ||
|
|
||
| def headers | ||
| %w[TopicID AuthorID] | ||
| end | ||
|
|
||
| def scope | ||
| language.topics.active.map { |topic| [ topic.id, 0 ] } | ||
| topics_collection.active.map { |topic| [ topic.id, 0 ] } | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,13 +12,38 @@ def perform | |
| # this is needed to avoid loading all files into memory at once | ||
| # Field 'name' is a lambda to allow dynamic naming based on the provider | ||
| def provider_files | ||
| [ | ||
| FileToUpload.new( | ||
| { | ||
| single_provider: FileToUpload.new( | ||
| content: ->(provider) { XmlGenerator::SingleProvider.new(provider).perform }, | ||
| name: ->(provider) { "#{language.file_storage_prefix}#{provider.name.parameterize}.xml" }, | ||
| path: "#{language.file_storage_prefix}CMES-Pi/assets/XML", | ||
| ), | ||
| ] | ||
| files: FileToUpload.new( | ||
| content: ->(provider) { CsvGenerator::Files.new(provider).perform }, | ||
| name: ->(provider) { "#{language.file_storage_prefix}#{provider.name.parameterize}-file.csv" }, | ||
| path: "#{language.file_storage_prefix}CMES-v2/assets/csv", | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider hoisting these up to the top since they are the same in all the methods? On less magic value (the path) stuffed into that interpolation.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please explain what do you mean by "hoisting these up to the top"? Extracting some parts of the code? |
||
| ), | ||
| topics: FileToUpload.new( | ||
| content: ->(provider) { CsvGenerator::Topics.new(provider).perform }, | ||
| name: ->(provider) { "#{language.file_storage_prefix}#{provider.name.parameterize}-topic.csv" }, | ||
| path: "#{language.file_storage_prefix}CMES-v2/assets/csv", | ||
| ), | ||
| tag_details: FileToUpload.new( | ||
| content: ->(provider) { CsvGenerator::TagDetails.new(provider, language:).perform }, | ||
| name: ->(provider) { "#{language.file_storage_prefix}#{provider.name.parameterize}-tag.csv" }, | ||
| path: "#{language.file_storage_prefix}CMES-v2/assets/csv", | ||
| ), | ||
| topic_tags: FileToUpload.new( | ||
| content: ->(provider) { CsvGenerator::TopicTags.new(provider, language:).perform }, | ||
| name: ->(provider) { "#{language.file_storage_prefix}#{provider.name.parameterize}-topic-tag.csv" }, | ||
| path: "#{language.file_storage_prefix}CMES-v2/assets/csv", | ||
| ), | ||
| topic_authors: FileToUpload.new( | ||
| content: ->(provider) { CsvGenerator::TopicAuthors.new(provider).perform }, | ||
| name: ->(provider) { "#{language.file_storage_prefix}#{provider.name.parameterize}-topic-author.csv" }, | ||
| path: "#{language.file_storage_prefix}CMES-v2/assets/csv", | ||
| ), | ||
| } | ||
| end | ||
|
|
||
| # Field 'content' is a lambda to allow lazy evaluation | ||
|
|
@@ -65,7 +90,7 @@ def language_files | |
| name: "#{language.file_storage_prefix}TopicTag.csv", | ||
| path: "#{language.file_storage_prefix}CMES-v2/assets/csv", | ||
| ), | ||
| topic_authors: FileToUpload.new( | ||
| topic_authors: FileToUpload.new( | ||
| content: ->(language) { CsvGenerator::TopicAuthors.new(language).perform }, | ||
| name: "#{language.file_storage_prefix}TopicAuthor.csv", | ||
| path: "#{language.file_storage_prefix}CMES-v2/assets/csv", | ||
|
|
@@ -79,11 +104,13 @@ def language_files | |
|
|
||
| def process_language_content! | ||
| language_files.keys.each do |file_id| | ||
| FileUploadJob.perform_later(language.id, file_id.to_s, "file") | ||
| FileUploadJob.perform_later(language.id, file_id.to_s) | ||
| end | ||
|
|
||
| language.providers.distinct.find_each do |provider| | ||
| FileUploadJob.perform_later(language.id, provider.id, "provider") | ||
| provider_files.keys.each do |file_id| | ||
| FileUploadJob.perform_later(language.id, file_id.to_s, provider.id) | ||
| end | ||
| end | ||
| end | ||
| 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.
Just curious -- why the env var in the signature? That gets serialized into the runner and then when its hydrated its already evaluated so old/failed jobs would write to the enque-time value, not the current value
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 is only default value and not the signature itself, right? Or do I miss something?