MARXAN - 448 Cost surface - scenarioId guard #241
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). marxan – ./app🔍 Inspect: https://vercel.com/vizzuality1/marxan/CNaCwLFa1z3JhJg73ui1CD4i4Kdh marxan-storybook – ./app🔍 Inspect: https://vercel.com/vizzuality1/marxan-storybook/CFncvb7TUXcHWsjuPmLVQxbSMMU1 |
There was a problem hiding this comment.
@kgajowy nice cleanup, thank you!
see suggestion about sticking to existing naming convention for services (I think yours was simply a typo here) and about a pagination that was removed - once you've taken care of these, feel free to merge, thanks.
| import { UpdateScenarioPlanningUnitLockStatusDto } from './dto/update-scenario-planning-unit-lock-status.dto'; | ||
|
|
||
| @Injectable() | ||
| export class ScenarioService { |
There was a problem hiding this comment.
I suggest to stick to <Plural>Service (as for the recently-refactored ProjectsService), and <Plural>CrudService (this is already ok in this PR for ScenariosCrudService). Filename to be adjusted accordingly here.
There was a problem hiding this comment.
@hotzevzl I actually forgot to mention this but I have done this "miss" on purpose - to have a clean diff (otherwise git would mess with diff merging new/old content).
There was a problem hiding this comment.
(as you already reviewed, will change it in this PR)
| const results = await this.service.findAll(fetchSpecification); | ||
| return this.scenarioSerializer.serialize(results.data, results.metadata); |
There was a problem hiding this comment.
async findAll(fetchSpecification: FetchSpecification) {
return this.crudService.findAllPaginated(fetchSpecification);
}
we do 🤔
Will change tho.
Similar to
projects:crud-serviceand wrap existing functions,scenarioIdreceived existingguardwhich verifies existence of given Scenario,