Allow partial export #522

ClFeSc · 2022-09-16T10:08:43Z

This addresses the partial exports as described in #392.

Allow exporting a partial export
Allow importing a partial export
Find a way to migrate partial exports without too much hassle

ClFeSc · 2022-12-26T15:26:56Z

* [ ]  Find a way to migrate partial exports without too much hassle

This is currently depending on #581 as I need to run a migration in the reducer (or the frontend) but not the backend.

ClFeSc · 2022-12-26T15:27:51Z

@Dassderdie Apart from the missing call of the migration this should now be ready for review.

Dassderdie · 2022-12-27T00:57:28Z

Skimming through the code, it seemed like having a complete and partial StateExport results in more code than just having a StateExport and a TemplateExport, which are completely separate from each other.
In general, only allowing up-to-date imports in the backend and doing the migration in the frontend, could be advantageous.
- Less strain on the server (DOS)
- Old migrations can't be used for attacks.
- The user can be provided with better feedback during the migration (e.g. "Running migration 1 of 11")

ClFeSc · 2022-12-27T13:59:02Z

Skimming through the code, it seemed like having a complete and partial StateExport results in more code than just having a StateExport and a TemplateExport, which are completely separate from each other.

In general, only allowing up-to-date imports in the backend and doing the migration in the frontend, could be advantageous.

Less strain on the server (DOS)

Old migrations can't be used for attacks.

The user can be provided with better feedback during the migration (e.g. "Running migration 1 of 11")

I agree with both points made. However, I'd stick with the current solution for now. The second point depends on #581, and the first point would be a refactoring I'd like to do separately. Maybe we could even disallow migrations for partial imports for now, depending on your plan with #581.

...tial-import-overwrite/partial-import-overwrite-modal/partial-import-overwrite.component.html

shared/src/store/action-reducers/exercise.ts

...tial-export-selection/partial-export-selection-modal/partial-export-selection.component.html

...tial-import-overwrite/partial-import-overwrite-modal/partial-import-overwrite.component.html

...src/app/pages/exercises/exercise/shared/trainer-map-editor/trainer-map-editor.component.html

...artial-import-overwrite/partial-import-overwrite-modal/partial-import-overwrite.component.ts

frontend/src/app/pages/exercises/exercise/exercise/exercise.component.html

shared/src/store/action-reducers/exercise.ts

Co-authored-by: Julian Schmidt <julian.schmidt@student.hpi.de>

…ports

…lidation

…ent`

Dassderdie

I haven't tested it out again. I will do it after these changes are incorporated.

Dassderdie · 2023-01-09T19:54:42Z

...rc/app/pages/exercises/exercise/shared/partial-export/open-partial-export-selection-modal.ts

+import type { NgbModal } from '@ng-bootstrap/ng-bootstrap';
+import { PartialExportComponent } from './partial-export-modal/partial-export-modal.component';
+
+export function openPartialExportSelectionModal(ngbModalService: NgbModal) {


Suggested change

export function openPartialExportSelectionModal(ngbModalService: NgbModal) {

export function openPartialExportModal(ngbModalService: NgbModal) {

Dassderdie · 2023-01-09T19:54:56Z

...rcises/exercise/shared/partial-export/partial-export-modal/partial-export-modal.component.ts

+    templateUrl: './partial-export-modal.component.html',
+    styleUrls: ['./partial-export-modal.component.scss'],
+})
+export class PartialExportComponent {


Suggested change

export class PartialExportComponent {

export class PartialExportModalComponent {

Dassderdie · 2023-01-09T19:58:36Z

...d/src/app/pages/exercises/exercise/shared/trainer-map-editor/trainer-map-editor.component.ts

+            const importedPlainObject = JSON.parse(importedText) as object;
+            const importedInstance = plainToInstance(
+                PartialExport,
+                importedPlainObject
+            );
+            const migratedInstance = migratePartialExport(importedInstance);


If this fails the user should get an error message that the import is invalid.

I just noticed that this could not fail. However, I don't know whether this is good behavior. When importing an invalid file, the migration function will choose undefined for all three categories, which is valid, just not useful. I'm not quite sure how to proceed there.

Dassderdie · 2023-01-09T20:01:11Z

shared/src/store/action-reducers/exercise.ts

+                    draftState.alarmGroups = Object.fromEntries(
+                        Object.entries(draftState.alarmGroups).map(
+                            ([id, alarmGroup]) => {
+                                alarmGroup.alarmGroupVehicles = {};
+                                return [id, alarmGroup];
+                            }
+                        )
+                    );


Suggested change

draftState.alarmGroups = Object.fromEntries(

Object.entries(draftState.alarmGroups).map(

([id, alarmGroup]) => {

alarmGroup.alarmGroupVehicles = {};

return [id, alarmGroup];

}

)

);

for (const alarmGroup of draftState.alarmGroups) {

alarmGroup.alarmGroupVehicles = {};

}

Dassderdie · 2023-01-09T20:02:19Z

shared/src/store/action-reducers/exercise.ts

+    const templateType = ['mapImageTemplates', 'vehicleTemplates'] as const;
+    for (const property of templateType) {
+        const templates = copy[property];
+        if (templates !== undefined) {
+            for (const template of templates) {
+                template.id = uuid();
+            }
+        }
+    }


Suggested change

const templateType = ['mapImageTemplates', 'vehicleTemplates'] as const;

for (const property of templateType) {

const templates = copy[property];

if (templates !== undefined) {

for (const template of templates) {

template.id = uuid();

}

}

}

const templateTypes = ['mapImageTemplates', 'vehicleTemplates'] as const;

for (const templateType of templateTypes) {

const templates = copy[templateType];

if (templates !== undefined) {

for (const template of templates) {

template.id = uuid();

}

}

}

Dassderdie · 2023-01-09T20:03:58Z

...rcises/exercise/shared/partial-import/partial-import-modal/partial-import-modal.component.ts

+            const importInstance = plainToInstance(
+                PartialExport,
+                this.partialExport
+            );


Suggested change

const importInstance = plainToInstance(

PartialExport,

this.partialExport

);

You should be able to use partialExport directly, as this is just data.

Dassderdie · 2023-01-09T20:09:00Z

...d/src/app/pages/exercises/exercise/shared/trainer-map-editor/trainer-map-editor.component.ts

+            const importedPlainObject = JSON.parse(importedText) as object;
+            const importedInstance = plainToInstance(
+                PartialExport,
+                importedPlainObject
+            );
+            const migratedInstance = migratePartialExport(importedInstance);


Suggested change

const importedPlainObject = JSON.parse(importedText) as object;

const importedInstance = plainToInstance(

PartialExport,

importedPlainObject

);

const migratedInstance = migratePartialExport(importedInstance);

const importedPlainObject = JSON.parse(importedText) as object;

const migratedPartialExport = migratePartialExport(importedPlainObject);

if (!isValid(plainToInstance(

PartialExport,

migratedPartialExport

))) {

throw Error('PartialExport is invalid');

}

At the moment, you are passing an instance to the modal (instead of a plain object) and, therefore, to the action. I would suspect this to crash, if this instance is later modified in a reducer (e.g., a template gets renamed) as immer.js would find a prototype in the template object.

Remember that all the `.create()-factory-functions were there to not have instances in the state.

FYI: Instead of the factory functions, one could also add a token to those classes in which immer should ignore the prototypes. But having plain objects without prototypes could be more performant (immerjs/immer#941) and maybe less error-prone. #531

…ports

ClFeSc added enhancement New feature or request frontend Issues mainly related to the frontend backend Issues mainly related to the backend labels Sep 16, 2022

ClFeSc self-assigned this Sep 16, 2022

Dassderdie linked an issue Sep 30, 2022 that may be closed by this pull request

Export & Import (full & partial) #392

Open

10 tasks

Allow partial export

cf17cc3

ClFeSc force-pushed the feature/392-partial-exports branch from 79038e9 to cf17cc3 Compare December 22, 2022 23:04

ClFeSc added 2 commits December 26, 2022 16:11

Allow importing partial exports

f7c99b3

WIP: Migrate partial export

a4fbbbb

ClFeSc marked this pull request as ready for review December 26, 2022 15:27

ClFeSc requested a review from Dassderdie December 26, 2022 15:27

Remove alarmGroups when overwriting vehicleTemplates

d881d74

Dassderdie requested changes Jan 4, 2023

View reviewed changes

ClFeSc and others added 8 commits January 9, 2023 15:30

Apply suggestions from code review

548ba0f

Co-authored-by: Julian Schmidt <julian.schmidt@student.hpi.de>

Merge remote-tracking branch 'origin/dev' into feature/392-partial-ex…

a1e0789

…ports

Fix validation decorators on new parts

f294749

Perform migration of partial export in frontend & remove redundant va…

2375b6b

…lidation

Only remove alarmGroupVehicles and not all alarmGroups

99dc5a6

Rename PartialExportSelectionComponent to PartialExportComponent

4c29288

Rename PartialImportOverwriteComponent to `PartialImportModalCompon…

5df1efa

…ent`

Rename some variables for clarity

91b950f

Dassderdie requested changes Jan 9, 2023

View reviewed changes

ClFeSc added 2 commits January 26, 2023 11:47

Review: Refactorings

0859757

Merge remote-tracking branch 'origin/dev' into feature/392-partial-ex…

265574c

…ports

ClFeSc requested a review from Dassderdie January 26, 2023 11:03

Fix prettier

09649a3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow partial export #522

Allow partial export #522

ClFeSc commented Sep 16, 2022 •

edited

Loading

ClFeSc commented Dec 26, 2022

ClFeSc commented Dec 26, 2022

Dassderdie commented Dec 27, 2022

ClFeSc commented Dec 27, 2022 •

edited

Loading

Dassderdie left a comment

Dassderdie Jan 9, 2023

Dassderdie Jan 9, 2023

Dassderdie Jan 9, 2023

ClFeSc Jan 26, 2023

Dassderdie Jan 9, 2023

Dassderdie Jan 9, 2023

Dassderdie Jan 9, 2023

Dassderdie Jan 9, 2023

Dassderdie Jan 9, 2023

	export function openPartialExportSelectionModal(ngbModalService: NgbModal) {
	export function openPartialExportModal(ngbModalService: NgbModal) {

	export class PartialExportComponent {
	export class PartialExportModalComponent {

Allow partial export #522

Are you sure you want to change the base?

Allow partial export #522

Conversation

ClFeSc commented Sep 16, 2022 • edited Loading

ClFeSc commented Dec 26, 2022

ClFeSc commented Dec 26, 2022

Dassderdie commented Dec 27, 2022

ClFeSc commented Dec 27, 2022 • edited Loading

Dassderdie left a 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

ClFeSc commented Sep 16, 2022 •

edited

Loading

ClFeSc commented Dec 27, 2022 •

edited

Loading