feat(clone): export domain/app #560

kgajowy · 2021-10-08T08:27:53Z

Substitute this line for a meaningful title for your changes

Overview

Please write a description. If the PR is hard to understand, provide a quick explanation of the code.

Designs

Link to the related design prototypes (if applicable)

Testing instructions

Please explain how to test the PR: ID of a dataset, steps to reach the feature, etc.

Feature relevant tickets

Link to the related task manager tickets

Checklist before submitting

Meaningful commits and code rebased on develop.
If this PR adds feature that should be tested for regressions when
deploying to staging/production, please add brief testing instructions
to the deploy checklist (docs/deployment-checklist.md)
Update CHANGELOG file

vercel · 2021-10-08T08:28:12Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

marxan-storybook – ./app

🔍 Inspect: https://vercel.com/vizzuality1/marxan-storybook/EAQUYSbAv8yLxX2R9KJbHdviHb2K
✅ Preview: https://marxan-storybook-git-feature-clone-feature-base-vizzuality1.vercel.app

marxan – ./app

🔍 Inspect: https://vercel.com/vizzuality1/marxan/3CmuhjQUtztBZ9y93BZS5Nhc56Rc
✅ Preview: https://marxan-git-feature-clone-feature-base-vizzuality1.vercel.app

Dyostiq · 2021-10-08T12:45:48Z

api/apps/api/src/modules/clone/export/domain/export/export.ts

+
+  complete(archiveLocation: ArchiveLocation) {
+    this.archiveLocation = archiveLocation;
+    this.apply(new ArchiveReady(this.id, this.archiveLocation));


even if parts are not finished?

good point!

fixed in 99777f0

Sikora00 · 2021-10-11T06:59:04Z

api/apps/api/src/modules/clone/export/application/request-export.ts

+  async export(id: ResourceId, kind: ResourceKind): Promise<ExportId> {
+    const parts: ClonePart[] = await this.resourcePieces.resolveFor(id, kind);
+    const exportInstance = this.eventPublisher.mergeObjectContext(
+      Export.project(id, parts),


ToDo: add scenario case. Right?

Don't think so, why? Resolved pieces (components) could also be a scenario ones.

So when the kind will be ResourceKind.Scenario then it will still create an Export by Export.project?

Think of Export as coordinator of the request/work to do, regardless of what resource is exported (project or scenario).

Thus, relevant adapters of export/import for given piece will know how to act.

This may not be the best approach overall, so suggestions are welcomed!

I think the approach of leaving the resolution of work to do to adapters can work well.

I see the main difference in the top-level entity, which can be either a project (with zero? maybe at least one, or more scenarios) or a scenario.

One nuance is whether a scenario adapter should behave differently depending on whether it's being run as part of a project export or when cloning a single scenario within a project - the main difference here may actually be that we don't need to be thinking about downloadable artifacts but everything can/should happen in one single pass within the platform.

If we are fine to implement cloning as a standalone procedure (pros: separation, speed/performance gains; cons: complicating the feature, keeping two processes in sync) we can still change the approach.

We shall also keep in mind that adapters may use some shared parts (one - extracting data, second variant for clone - putting it back, second variant for export - wrapping it into some file(s)

but Export.project(id suggests it is only for the project, and id is a project id, kind is not used here
and for kind === scenario, Export.project is used as well what is surprising

Sikora00 · 2021-10-11T07:00:25Z

api/apps/api/src/modules/clone/export/domain/export/export.ts

+  private constructor(
+    public readonly id: ExportId,
+    public readonly resourceId: ResourceId,
+    public readonly resourceKind: ResourceKind,


Are you sure that kind is a good discriminator? Where it will affect the app logic?

most likely shouldn't affect application logic. Could be that the same export may be used for both project and single-scenario export but not sure at this moment.

What you would propose?

Sikora00 · 2021-10-11T07:01:23Z

api/apps/api/src/modules/clone/export/domain/export/clone-part/clone-part.ts

+import { PieceId } from './piece.id';
+import { PieceLocation } from './piece-location';
+
+export class ClonePart {


What is that? Could you provide an example?

@Sikora00 its a component part listed in HLD. Will update the name to reflect it.

Renamed in b56996e

hotzevzl · 2021-10-11T16:06:35Z

@kgajowy overall the approach looks good to me. One thing I recommend to keep in mind is that the core process here is really the cloning of projects or scenarios, with artifacts being a (fundamental) byproduct in the case of the project download/upload workflow (rather than cloning without intermediate steps).

I imagine this may mean still passing through some kind of intermediate representation of a scenario and project, of which the exportable and importable artifact may be a complex serialization - also taking into account that any such intermediate representation will often be something we should not assume that will reliably fit into memory, so persisting components' IR in some for may be unavoidable. But I'd consider the exportable/importable artifacts as a further step - all of this to be validated in terms of possible approaches, of course.

kgajowy · 2021-10-12T05:43:48Z

@kgajowy overall the approach looks good to me. One thing I recommend to keep in mind is that the core process here is really the cloning of projects or scenarios, with artifacts being a (fundamental) byproduct in the case of the project download/upload workflow (rather than cloning without intermediate steps).

I imagine this may mean still passing through some kind of intermediate representation of a scenario and project, of which the exportable and importable artifact may be a complex serialization - also taking into account that any such intermediate representation will often be something we should not assume that will reliably fit into memory, so persisting components' IR in some for may be unavoidable. But I'd consider the exportable/importable artifacts as a further step - all of this to be validated in terms of possible approaches, of course.

What would you propose @hotzevzl ? I am not sure if we can have any reliable data representation useful just for cloning (even the sqls) which would make it easy to then represent them in a different manner. How I imagined in there is that cloning would work exactly like export->import but without sharing the archive (thats why we keep geojsons/shapefiles rather than csv's with geometry)

Dyostiq · 2021-10-18T15:08:22Z

api/apps/api/src/modules/clone/export/application/request-export.ts

+    const parts: ExportComponent[] = await this.resourcePieces.resolveFor(
+      id,
+      kind,
+    );


what does ResourcePieces do?
Scans resources and determines what is exportable?

@Dyostiq yes. Could be the name used shall be resourcePiecesResolver ?

Dyostiq · 2021-10-18T15:11:58Z

api/apps/api/src/modules/clone/export/application/request-export.ts

+  async export(id: ResourceId, kind: ResourceKind): Promise<ExportId> {
+    const parts: ClonePart[] = await this.resourcePieces.resolveFor(id, kind);
+    const exportInstance = this.eventPublisher.mergeObjectContext(
+      Export.project(id, parts),


but Export.project(id suggests it is only for the project, and id is a project id, kind is not used here
and for kind === scenario, Export.project is used as well what is surprising

Dyostiq · 2021-10-18T15:13:46Z

api/apps/api/src/modules/clone/export/application/request-export.ts

+  ) {}
+
+  async export(id: ResourceId, kind: ResourceKind): Promise<ExportId> {
+    const parts: ExportComponent[] = await this.resourcePieces.resolveFor(


Suggested change

const parts: ExportComponent[] = await this.resourcePieces.resolveFor(

const pieces: ExportComponent[] = await this.resourcePieces.resolveFor(

2082b21
fixed together with the Export.project.

Thanks for catching this!

kgajowy changed the title ~~feat(clone): domain~~ feat(clone): export domain Oct 8, 2021

vercel bot deployed to Preview – marxan-storybook October 8, 2021 08:28 View deployment

vercel bot deployed to Preview – marxan October 8, 2021 08:28 View deployment

feat(clone): domain

70da55e

kgajowy force-pushed the feature/clone-feature-base branch from 5610427 to 70da55e Compare October 8, 2021 08:29

vercel bot deployed to Preview – marxan-storybook October 8, 2021 08:29 View deployment

vercel bot deployed to Preview – marxan October 8, 2021 08:31 View deployment

kgajowy requested review from Dyostiq, hotzevzl and Sikora00 October 8, 2021 08:38

kgajowy changed the title ~~feat(clone): export domain~~ feat(clone): export domain/app Oct 8, 2021

Dyostiq reviewed Oct 8, 2021

View reviewed changes

feat(clone): domain snapshots and intentions return values

99777f0

vercel bot deployed to Preview – marxan October 11, 2021 05:46 View deployment

vercel bot deployed to Preview – marxan-storybook October 11, 2021 05:46 View deployment

kgajowy marked this pull request as ready for review October 11, 2021 05:50

Sikora00 suggested changes Oct 11, 2021

View reviewed changes

vercel bot deployed to Preview – marxan-storybook October 11, 2021 08:46 View deployment

vercel bot deployed to Preview – marxan October 11, 2021 08:46 View deployment

feat(clone): rename clone-piece to export-component

96ee57e

kgajowy force-pushed the feature/clone-feature-base branch from b56996e to 96ee57e Compare October 11, 2021 10:51

vercel bot deployed to Preview – marxan-storybook October 11, 2021 10:52 View deployment

vercel bot deployed to Preview – marxan October 11, 2021 10:52 View deployment

kgajowy requested review from Sikora00 and Dyostiq October 13, 2021 05:28

Dyostiq reviewed Oct 18, 2021

View reviewed changes

refactor(clone): replace hardcoded project export

2082b21

vercel bot deployed to Preview – marxan-storybook October 19, 2021 07:26 View deployment

vercel bot deployed to Preview – marxan October 19, 2021 07:26 View deployment

kgajowy merged commit 80c2e39 into develop Oct 25, 2021

kgajowy deleted the feature/clone-feature-base branch October 25, 2021 06:51

kgajowy mentioned this pull request Dec 23, 2021

docs(process): development flow #691

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(clone): export domain/app #560

feat(clone): export domain/app #560

kgajowy commented Oct 8, 2021

vercel bot commented Oct 8, 2021 •

edited

Loading

Dyostiq Oct 8, 2021

kgajowy Oct 8, 2021

kgajowy Oct 11, 2021

Sikora00 Oct 11, 2021

kgajowy Oct 11, 2021

Sikora00 Oct 11, 2021

kgajowy Oct 11, 2021

hotzevzl Oct 11, 2021

kgajowy Oct 12, 2021

Dyostiq Oct 18, 2021

Sikora00 Oct 11, 2021

kgajowy Oct 11, 2021

kgajowy Oct 11, 2021

Sikora00 Oct 11, 2021

kgajowy Oct 11, 2021

kgajowy Oct 11, 2021

hotzevzl commented Oct 11, 2021

kgajowy commented Oct 12, 2021

Dyostiq Oct 18, 2021

kgajowy Oct 19, 2021

Dyostiq Oct 18, 2021

Dyostiq Oct 18, 2021

kgajowy Oct 19, 2021

	const parts: ExportComponent[] = await this.resourcePieces.resolveFor(
	const pieces: ExportComponent[] = await this.resourcePieces.resolveFor(

feat(clone): export domain/app #560

feat(clone): export domain/app #560

Conversation

kgajowy commented Oct 8, 2021

Substitute this line for a meaningful title for your changes

Overview

Designs

Testing instructions

Feature relevant tickets

Checklist before submitting

vercel bot commented Oct 8, 2021 • edited Loading

marxan-storybook – ./app

marxan – ./app

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hotzevzl commented Oct 11, 2021

kgajowy commented Oct 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented Oct 8, 2021 •

edited

Loading