Implement missing eperimental variable functionality #1274

KochTobi · 2025-09-14T18:28:20Z

Changes the UI for experimental variable edit and creation.
Allows for

renaming of experimental variables
adding levels to existing experimental variables
removing unused levels from experimental variables
locking of input fields that cannot be edited.

…perimental-variable-functions

…perimental-variable-functions # Conflicts: # project-management/src/main/java/life/qbic/projectmanagement/application/api/AsyncProjectService.java # project-management/src/main/java/life/qbic/projectmanagement/application/api/AsyncProjectServiceImpl.java # user-interface/src/main/java/life/qbic/datamanager/views/projects/project/experiments/experiment/ExperimentDetailsComponent.java # user-interface/src/main/java/life/qbic/datamanager/views/projects/project/experiments/experiment/components/ExperimentalVariableRowLayout.java

Replace `.vertical-list` by `.flex-vertical .width-full` Replace `.trailing-margin-large` by `.margin-bottom-07` Replace `.trailing-margin-normal` by `.margin-bottom-05` Replace `.trailing-margin-small` by `.margin-bottom-03`

removes Speculative Generality

…perimental-variable-functions # Conflicts: # project-management/src/main/java/life/qbic/projectmanagement/application/api/AsyncProjectService.java # user-interface/frontend/themes/datamanager/components/all.css # user-interface/src/main/bundles/dev.bundle # user-interface/src/main/java/life/qbic/datamanager/views/general/dialog/DialogBody.java # user-interface/src/main/java/life/qbic/datamanager/views/general/dialog/DialogSection.java

…perimental-variable-functions # Conflicts: # user-interface/src/main/bundles/dev.bundle

Co-authored-by: Shraddha Pawar <[email protected]>

When editing a duplicated variable name, the error message is removed.

…perimental-variable-functions # Conflicts: # user-interface/src/main/bundles/dev.bundle

Co-authored-by: Shraddha Pawar <[email protected]>

sven1103

Your are getting there, thank you for addressing quite some change requests already.

I still have concerns about some implementations and left comments there.

sven1103 · 2025-10-07T06:09:53Z

...anagement/src/main/java/life/qbic/projectmanagement/application/api/AsyncProjectService.java


  /**
   * Queries all available experimental variables for a given experiment.
+   * Returns existing experimental variables in lexigraphical order of the variable name.


typo in lexicographical

sven1103 · 2025-10-07T06:10:29Z

...anagement/src/main/java/life/qbic/projectmanagement/application/api/AsyncProjectService.java

   * @param experimentId the experiment identifier of the experiment to get the variables from
   * @return a {@link Flux<ExperimentalVariable>} emitting {@link ExperimentalVariable}s for the
-   * experiment.
+   * experiment in lexigraphical order of variable name.


typo in lexicographical

sven1103 · 2025-10-07T06:17:34Z

...in/java/life/qbic/projectmanagement/application/experiment/ExperimentInformationService.java

+        .map(level -> (unit == null || unit.isBlank())
+            ? ExperimentalValue.create(level)
+            : ExperimentalValue.create(level, unit))
+        .collect(Collectors.toList());


Here a mutable list is created. I dont see the necessity for a mutable state here but risk for side effects. If there is no need for mutability the list shall be unmodifiable via toList()

sven1103 · 2025-10-07T06:35:54Z

...management/src/main/java/life/qbic/projectmanagement/domain/model/experiment/Experiment.java


  /**
-   * Removes an experimental variable if possible.
+   * Removes an experimental variable if possible. Emits an experiment update domain event.


Pro: if you reference the event class, so developers are happy when they need to subscribe to it in the service :)

sven1103 · 2025-10-07T06:39:16Z

...management/src/main/java/life/qbic/projectmanagement/domain/model/experiment/Experiment.java


  /**
-   * Removes an experimental variable if possible.
+   * Removes an experimental variable if possible. Emits an experiment update domain event.


Suggested change

* Removes an experimental variable if possible. Emits an experiment update domain event.

* Removes an experimental variable if possible. Emits an {@link ExperimentUpdatedEvent}.

...ects/project/experiments/experiment/components/experimentalvariable/VariableLevelsInput.java

sven1103 · 2025-10-07T07:37:38Z

...ews/projects/project/experiments/experiment/components/experimentalvariable/VariableRow.java

+        .and(variableLevelsValidation);
+  }
+
+  void setInvalid(boolean invalid) {


Sure, but just because the framework does it, it is not good practise :) The flags can be easily overseen during debugging and implementation, clean code favours expressive semantics of methods over boolean configuration flags.

I kindly ask you to change it, it is 1 min work and introduces expressive semantics.

...ews/projects/project/experiments/experiment/components/experimentalvariable/VariableRow.java

sven1103 · 2025-10-07T07:45:24Z

...ews/projects/project/experiments/experiment/components/experimentalvariable/VariableRow.java

+    if (Optional.ofNullable(name.getErrorMessage())
+        .map(it -> !it.contains(errorMessage))
+        .orElse(true)) {
+      String composedErrorMessage = Optional.ofNullable(name.getErrorMessage())


Why is there a need to concat existing error messages? Also, the method JDs does not highlight that so this behaviour is opaque to the caller.

Overwriting the error message would make strong assumptions on the calling code (e.g. only invalid if name is blank or duplicated which cannot happen both at the same time)

I still dont get it. You set the name invalid for one reason by the caller. Why is there hidden concatenation of previous error states?

Co-authored-by: Sven F. <[email protected]>

sven1103

Still some concerns, in particular the rename experimental variables is troubling me.

...nt/src/main/java/life/qbic/projectmanagement/domain/model/experiment/ExperimentalDesign.java

sven1103 · 2025-10-07T12:14:37Z

user-interface/src/main/java/life/qbic/datamanager/views/general/DragDropList.java

+          return DragDropItem.this;
+        }
+
+//    Setting only the item as draggable element does not work


please create a follow up issue then and denote as a bug in our app and reference the Vaadin issue there. The code base shall stay clean.

...oject/experiments/experiment/components/experimentalvariable/ExperimentalVariablesInput.java

sven1103 · 2025-10-07T12:16:29Z

...oject/experiments/experiment/components/experimentalvariable/ExperimentalVariablesInput.java

+      dummy.restore(snapshot);
+      return dummy;
+    } catch (SnapshotRestorationException e) {
+      throw new IllegalStateException("Initial state not correctly set.", e);


it is not an illegal state. It is an illegal argument at most or let the snapshot exception bubble up to the caller, which is the same class. Otherwise i dont see the point of having this semantic exception at all.

sven1103 · 2025-10-07T12:19:03Z

...ews/projects/project/experiments/experiment/components/experimentalvariable/VariableRow.java

+    if (Optional.ofNullable(name.getErrorMessage())
+        .map(it -> !it.contains(errorMessage))
+        .orElse(true)) {
+      String composedErrorMessage = Optional.ofNullable(name.getErrorMessage())


I still dont get it. You set the name invalid for one reason by the caller. Why is there hidden concatenation of previous error states?

KochTobi · 2025-10-07T13:18:20Z

#1274 (comment)

A call to the method ensures that the field is set as invalid and that the provided error message is shown. It does not assume the field to be valid when calling the method neither does it assume that the provided error message is the only error message. Both is stated in the JavaDoc.

Co-authored-by: Sven F. <[email protected]>

I removed the comment and we will encounter the issue again if and when we try to implement a drag handle. Tracking it with our own github issue is not necessary as maybe another solution is found at a future time.

sven1103

I insist on some refactors since the are good safe guards and improve the overall robustness.

I also cannot see all the changes being made, although the comment was set to "addressed". I dont know where the update is cached, but I cant continue.

...nt/src/main/java/life/qbic/projectmanagement/domain/model/experiment/ExperimentalDesign.java

sven1103

One idea for the validation thingy

sven1103 · 2025-10-08T08:27:13Z

...ews/projects/project/experiments/experiment/components/experimentalvariable/VariableRow.java

+   *
+   * @param errorMessage the error message to show.
+   */
+  void setNameInvalid(@NonNull String errorMessage) {


OK, i think i get it now: elements of the variable row can be invalid for two different reasons: external validation logic and internal validation logic. And we must not override them.

But what happens if the external caller sets the name invalid for a reason, and the internal validation also already failed. Lets say after user input the external validation is fine, but the internal still fails. So currently the caller would use setNameValid which makes the name valid and removes ALL error messages, even the ones that might have come from internal validation.

I see tight coupling of the error message display and two different clients: an external and internal one. I think you need to wrap TextField into a small decorator that enables you to set invalid with message external and internal to keep track of where the change comes from. You can then refresh internally the valid state every time the API is called (either when external/internal validate or invalidate)

…ing-eperimental-variable-functions' into feature-1178/1178-implement-missing-eperimental-variable-functions

KochTobi · 2025-10-09T09:12:20Z

#1324

KochTobi added 30 commits June 11, 2025 12:40

Preserve order of levels

6ff8af9

make it compile again

f7b340d

Merge branch 'development' into feature-1178/1178-implement-missing-e…

9b0e84c

…perimental-variable-functions

wip

bdca38b

wip

3103d69

section off regions and add borders

0d0cb6f

Replace ontology-term CSS class

faec88b

Remove CSS classes

4fa7603

Replace `.vertical-list` by `.flex-vertical .width-full` Replace `.trailing-margin-large` by `.margin-bottom-07` Replace `.trailing-margin-normal` by `.margin-bottom-05` Replace `.trailing-margin-small` by `.margin-bottom-03`

wip

9ff8c33

add missing variables

5926d06

replace lumo with custom variables

e542fb0

move ButtonFactory

e78f5d1

add icon button factory method

3d56282

update demo

5c95256

Fix label css

643e8a7

replace with classes

1b5e9f8

introduce delete event

f9ea42c

wip

d58801c

add restoration option for deleted variables

f1746f5

Add Nullable annotation to DialogBody.java#with method

41a1630

remove test button

621a146

Fix value handling of level field

0ce5a9c

Add paste behaviour

eecadeb

address code smells

cea127e

hide public constructor

0f5fa65

simplify memento interface

cf8b3d5

removes Speculative Generality

Merge branch 'development' into feature-1178/1178-implement-missing-e…

e3cc6c7

…perimental-variable-functions # Conflicts: # user-interface/src/main/bundles/dev.bundle

Handle validation

2cf58c9

KochTobi and others added 3 commits October 6, 2025 11:55

Fix sorting of variables

bbe23b3

Remove duplicate error when level value changes

a58da23

Co-authored-by: Shraddha Pawar <[email protected]>

Remove error message for duplicate variable name

4333831

When editing a duplicated variable name, the error message is removed.

KochTobi requested a review from sven1103 October 6, 2025 10:39

KochTobi and others added 6 commits October 6, 2025 12:40

Merge branch 'development' into feature-1178/1178-implement-missing-e…

55aab8e

…perimental-variable-functions # Conflicts: # user-interface/src/main/bundles/dev.bundle

replace choice-box css

2fb4110

Change error text

069746c

Co-authored-by: Shraddha Pawar <[email protected]>

update dev.bundle

915aa18

Fix css color

cafb294

Change delete confirm text

2ef7467

Co-authored-by: Shraddha Pawar <[email protected]>

sven1103 requested changes Oct 7, 2025

View reviewed changes

KochTobi and others added 5 commits October 7, 2025 09:58

Link javadoc

b87ee8c

Co-authored-by: Sven F. <[email protected]>

Add clarifying javadoc

48e46db

Co-authored-by: Sven F. <[email protected]>

remove all the generics from javadoc

6e014ed

Co-authored-by: Sven F. <[email protected]>

address review

18944e9

add missed change

82786fb

KochTobi requested a review from sven1103 October 7, 2025 09:46

sven1103 requested changes Oct 7, 2025

View reviewed changes

KochTobi and others added 3 commits October 7, 2025 15:21

address exception

885521d

Co-authored-by: Sven F. <[email protected]>

Use deep copy instead of inplace modification

da74ce4

Co-authored-by: Sven F. <[email protected]>

Remove comment

d779b62

I removed the comment and we will encounter the issue again if and when we try to implement a drag handle. Tracking it with our own github issue is not necessary as maybe another solution is found at a future time.

sven1103 requested changes Oct 8, 2025

View reviewed changes

Provide exception supplier

2f4b39a

sven1103 reviewed Oct 8, 2025

View reviewed changes

KochTobi added 2 commits October 8, 2025 14:08

Use UIHandle

37a5bec

Merge remote-tracking branch 'origin/feature-1178/1178-implement-miss…

5b7f063

…ing-eperimental-variable-functions' into feature-1178/1178-implement-missing-eperimental-variable-functions

sven1103 approved these changes Oct 9, 2025

View reviewed changes

KochTobi merged commit d6318ab into development Oct 9, 2025
3 of 4 checks passed

	* Removes an experimental variable if possible. Emits an experiment update domain event.
	* Removes an experimental variable if possible. Emits an {@link ExperimentUpdatedEvent}.

Implement missing eperimental variable functionality #1274

Implement missing eperimental variable functionality #1274

Uh oh!

Conversation

KochTobi commented Sep 14, 2025

Uh oh!

sven1103 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sven1103 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KochTobi commented Oct 7, 2025

Uh oh!

sven1103 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sven1103 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KochTobi commented Oct 9, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants