Allow sending set of provider-specific CSV files #420 (reopened) #458
Conversation
johnpaulashenfelter
left a comment
johnpaulashenfelter
left a comment
There was a problem hiding this comment.
Could of minor comments.
What would be most useful?
| private | ||
|
|
||
| attr_reader :language, :processor, :share | ||
| attr_reader :language, :file_id, :share, :processor |
There was a problem hiding this comment.
Nitpick: 🔤 would make the diff a little easier.
There was a problem hiding this comment.
Could you please explain what would make the diff easier? And what to you mean by "diff" here?
| attr_reader :language, :processor, :share | ||
| attr_reader :language, :file_id, :share, :processor | ||
|
|
||
| def send_provider_content(provider_id) |
There was a problem hiding this comment.
Consider keyword so you throw an error if provider_id is not defined?
There was a problem hiding this comment.
Why throwing an error?
| 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.
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.
There was a problem hiding this comment.
Could you please explain what do you mean by "hoisting these up to the top"? Extracting some parts of the code?
| 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"]) |
There was a problem hiding this comment.
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.
It is only default value and not the signature itself, right? Or do I miss something?
Reopening PR #430 which was accidentally merged prematurely.
Original PR
Summary
This PR enables the ability to transmit multiple CSV files specific to each provider.
Changes
FileUploadJobto accept language or provider parametersFiles Changed