Dump n load #1588
Dump n load #1588
Conversation
There was a problem hiding this comment.
Hey! Just a few comments on the first few files if you want to continue working on this tomorrow! There are also a few questions for you in there to either research yourself if you think that's fun and you have time. Thought that could be a new thing we can try out if you're in to that. Otherwise we can discuss them Thursday, no prob :)
| load_resource except: %i[show create destroy] | ||
| load_and_authorize_resource class: Scenario, only: %i[index show destroy] | ||
| load_resource except: %i[show create destroy dump] | ||
| load_and_authorize_resource class: Scenario, only: %i[index show create destroy dump] |
There was a problem hiding this comment.
| load_and_authorize_resource class: Scenario, only: %i[index show create destroy dump] | |
| load_and_authorize_resource class: Scenario, only: %i[index show destroy dump] |
Adding create here will create the scenario twice.
| file = params.permit(:dump)[:dump] | ||
| unless file&.respond_to?(:path) | ||
| redirect_back fallback_location: root_path, alert: 'No file provided' | ||
| return | ||
| end | ||
|
|
||
| raw_data = JSON.parse(File.read(file.path)) | ||
| data_array = raw_data.is_a?(Array) ? raw_data : [raw_data] | ||
|
|
||
| @scenarios = data_array.map { |sd| ScenarioPacker::Load.new(sd.with_indifferent_access).scenario } | ||
|
|
||
| if @scenarios.one? | ||
| new_id = @scenarios.first.id | ||
| redirect_to inspect_scenario_path( | ||
| id: new_id, | ||
| api_scenario_id: new_id | ||
| ), | ||
| notice: 'Scenario created' | ||
| else | ||
| api_id = @scenarios.first.id | ||
| @api_scenario = Scenario.find(api_id) | ||
| @scenario = Scenario::Editable.new(@api_scenario) | ||
| render :load_results | ||
| end | ||
| end |
There was a problem hiding this comment.
Remember: we should try to make methods be not more than 12 lines. So we can smell a possible refactor here!
The question as always: which logic belongs where?
Small hint: The ScenarioPacker can currently Load one scenario from a parsed JSON. So his job is loading stuff from a parsed JSON.
Second hint: we can decide to use before hooks to validate incoming request data.
| filename = | ||
| if payload.one? | ||
| "#{scenarios.first.id}-dump.json" | ||
| else | ||
| "#{ids.join('-')}-dump.json" | ||
| end |
There was a problem hiding this comment.
| filename = | |
| if payload.one? | |
| "#{scenarios.first.id}-dump.json" | |
| else | |
| "#{ids.join('-')}-dump.json" | |
| end | |
| filename = "#{ids.join('-')}-dump.json" |
If there was only one in the ids array, the join would just return the id.
| type = params[:type] || 'user_input' | ||
| ids = ScenarioSetFetcher.fetch(type: type, params: params) |
There was a problem hiding this comment.
- It can be dangerous to just forward raw params further into the code, how can we do this differently?
- Who should now default keywords? The service class, the view, or the controller, or is it shared knowledge?
…rubocop corrected, sanitized params
|
I tried to spend some time addressing code smells @noracato - please let me know if you have more feedback or if you want to go over anything together |
PR for MVP:
This PR:
Outstanding issues:
Closes #1569, #1570, #1571