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

use cost-surfaces as part of path for cost surface-related endpoints (rather than cost-surface) [MRXN23-479] [MRXN23-480] #1541

Conversation

hotzevzl
Copy link
Member

@hotzevzl hotzevzl commented Oct 9, 2023

@vercel
Copy link

vercel bot commented Oct 9, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
marxan ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 10, 2023 0:51am

@hotzevzl hotzevzl force-pushed the chore/api/MRXN23-479_update-paths-of-cost-surface-endpoints-to-match-conventions branch from a347b35 to 2e58a58 Compare October 9, 2023 12:09
@hotzevzl hotzevzl requested a review from yulia-bel October 9, 2023 12:50
@hotzevzl hotzevzl marked this pull request as ready for review October 9, 2023 12:55
@hotzevzl hotzevzl force-pushed the chore/api/MRXN23-479_update-paths-of-cost-surface-endpoints-to-match-conventions branch from 73bd0e9 to 3a43199 Compare October 9, 2023 15:02
@Req() req: RequestWithAuthenticatedUser,
@Body() dto: UploadShapefileDto,
): Promise<JsonApiAsyncJobMeta> {
const outcome = await this.projectsService.addProtectedAreaFor(
Copy link
Contributor

Choose a reason for hiding this comment

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

just a question/suggestion - maybe move the addProtedtedAreaFor() to ProjectProtectedAreasService.. its basically just doing the file validation and acl though and then passes to AddProtectedAreaService, so maybe not worth it.. but being inspired by this nice cleanup, feels a bit like there are too much services involved in project protected areas

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, it does make sense, but then we enter the rabbit hole of cleaning up ProjectsService which is another sprawling chaos, and... I'd rather stop at the controller layer for this PR 😬

@hotzevzl hotzevzl changed the title use cost-surfaces as part of path for cost surface-related endpoints (rather than cost-surface) [MRXN23-479] use cost-surfaces as part of path for cost surface-related endpoints (rather than cost-surface) [MRXN23-479] [MRXN23-480] Oct 9, 2023
@hotzevzl hotzevzl force-pushed the chore/api/MRXN23-479_update-paths-of-cost-surface-endpoints-to-match-conventions branch from 3a43199 to 905d98f Compare October 9, 2023 16:02
This was long overdue - I should have done this while splitting the
sprawling blob of monolithing e2e runs into subsets, but well, better
now than never. Project-related tests within API e2e tests were among
the longest-running, and the split by concern in this commit should
make each subset more manageable (for example, leading to shorter run
times if needing to trigger a new run of a subset manually because of
spurious failures in CI).
@hotzevzl hotzevzl force-pushed the chore/api/MRXN23-479_update-paths-of-cost-surface-endpoints-to-match-conventions branch from 905d98f to c8dd035 Compare October 10, 2023 12:49
@hotzevzl hotzevzl merged commit 640c75a into develop Oct 10, 2023
61 checks passed
@hotzevzl hotzevzl deleted the chore/api/MRXN23-479_update-paths-of-cost-surface-endpoints-to-match-conventions branch October 10, 2023 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants