9: Create New Exercise - [merged] #76

Closed raffaelfoidl closed 2 years ago

In GitLab by @11712616 on Oct 15, 2021, 20:50

Merges 9-create-new-exercise -> develop

Closes #9

In GitLab by @11775820 on Oct 16, 2021, 21:24

Commented on server/src/main/java/com/witness/server/service/impl/ExerciseServiceImpl.java line 63

var

In GitLab by @11775820 on Oct 16, 2021, 21:25

Commented on server/src/main/java/com/witness/server/service/impl/ExerciseServiceImpl.java line 69

var

In GitLab by @11775820 on Oct 16, 2021, 21:26

Commented on server/src/main/java/com/witness/server/service/ExerciseService.java line 7

DataCreationException is an unused import, pls remove.

Other than that, I think documentation is still on your todo list.

In GitLab by @11775820 on Oct 16, 2021, 21:27

Commented on server/src/main/java/com/witness/server/setup/EnvironmentBootstrapper.java line 45

Please put UserRepository userRepository on the previous line.

Also please put the Random intialization at the bottom since it doesn't consume a constructor parameter and it distracts the reading flow a bit when it's in the middle like this.

In GitLab by @11775820 on Oct 16, 2021, 21:29

Commented on server/src/main/java/com/witness/server/setup/EnvironmentBootstrapper.java line 26

Don't forget to adjust docs here.

In GitLab by @11775820 on Oct 16, 2021, 21:30

Commented on server/src/main/java/com/witness/server/setup/EnvironmentBootstrapper.java line 124

Please put arguments of getDummyExercise on separate lines for consistency reasons.

In GitLab by @11775820 on Oct 16, 2021, 21:32

Commented on server/src/main/java/com/witness/server/setup/EnvironmentBootstrapper.java line 136

please put Function<SetupArguments.SetupExercise, Exercise> dummyExerciseGetter on the previous line.

In GitLab by @11775820 on Oct 16, 2021, 21:54

Commented on server/src/main/java/com/witness/server/web/controller/ExerciseController.java line 12

DataCreationException and PublicApi unused impors, pls remove

In GitLab by @11775820 on Oct 16, 2021, 21:56

Commented on server/src/test/java/com/witness/server/integration/service/ExerciseServiceTest.java line 32

JUnit 5 is less strict with visibility modifier than version 4. Therefore, please remove the public modifier (to make the class package private) to avoid namespace pollution.

In GitLab by @11775820 on Oct 16, 2021, 21:58

Commented on server/src/test/java/com/witness/server/integration/service/ExerciseServiceTest.java line 67

throws? that's the format we have used in ASE iirc and my tests already follow it. If you absolutely cant stand it, change my tests accordingly please. The same holds for the other test methods in this class.

In GitLab by @11775820 on Oct 16, 2021, 22:01

Commented on server/src/test/java/com/witness/server/integration/service/ExerciseServiceTest.java line 22

Remove unused imports:

import io.swagger.v3.oas.annotations.Parameters;
import org.junit.jupiter.params.provider.EnumSource;
import org.mockito.Mock;

In GitLab by @11775820 on Oct 16, 2021, 22:02

Commented on server/src/test/java/com/witness/server/integration/service/ExerciseServiceTest.java line 95

Please do an auto-format, this User user is not correctly indented. When you're already at it, User user fits onto the previous line, autoformat will then place only the throws clause onto the next line.

In GitLab by @11775820 on Oct 16, 2021, 22:03

Commented on server/src/test/java/com/witness/server/integration/service/ExerciseServiceTest.java line 112

Please move UserExercise output to previous line.

In GitLab by @11775820 on Oct 16, 2021, 22:08

Commented on client/lib/widgets/exercises/editing/create_exercise_form.dart line 209

Why don't you simply use a literal map declaration like this? Type parameters are inferred from the static type definition, hence no <K, V>{...} needed.

Map<MuscleGroup, bool> muscleGroups = {
    MuscleGroup.chest: false,
    MuscleGroup.shoulders: false,
    MuscleGroup.back: false,
    MuscleGroup.legs: false,
    MuscleGroup.abs: false,
    MuscleGroup.arms: false,
    MuscleGroup.glutes: false,
    MuscleGroup.other: false,
  };
  Map<LoggingType, bool> loggingTypes = {LoggingType.reps: false, LoggingType.time: false};

What's the benefit in creating the map twice (once as literal, and then a copy of that one using Map.of)? Or am I missing some reason that requires us to do it like this?

In GitLab by @11775820 on Oct 16, 2021, 22:10

Commented on server/src/test/java/com/witness/server/unit/mapper/ExerciseMapperTest.java line 18

please remove redundant public access modifier.

In GitLab by @11775820 on Oct 16, 2021, 22:15

Commented on server/src/test/java/com/witness/server/unit/mapper/ExerciseMapperTest.java line 28

I just had it like this in UserMapperTest as a "showcase" so that you see the different ways one can use the @JsonFileSources and @JsonFileSource annotations (although it is documented fairly thoroughly).

Generally, we don't want to have a test that repeatedly executes the very same assertions. That's why I have built the unwrapArrays switch. This converts the list and array arguments (as one test exeution) to n test execution with one argument. Please change the method like this:

  @ParameterizedTest
  @JsonFileSources(unwrapArrays = true, parameters = {
      @JsonFileSource(value = DATA_ROOT + "Exercises_1-2.json", type = Exercise[].class),
      @JsonFileSource(value = DATA_ROOT + "ExerciseDtos_1-2.json", type = ExerciseDto[].class)
  })
  void entityToDto(Exercise entity, ExerciseDto dto) {
    assertThat(mapper.entityToDto(entity)).isEqualTo(dto);
  }

"Evidence":

In GitLab by @11775820 on Oct 16, 2021, 22:21

Commented on server/src/test/java/com/witness/server/unit/mapper/ExerciseMapperTest.java line 37

Just like above, please change to:

  @ParameterizedTest
  @JsonFileSources(unwrapArrays = true, parameters = {
      @JsonFileSource(value = DATA_ROOT + "UserExercises_1-2.json", type = UserExercise[].class),
      @JsonFileSource(value = DATA_ROOT + "UserExerciseDtos_1-2.json", type = UserExerciseDto[].class, arrayToList = true)
  })
  void userEntityToDto(UserExercise entity, UserExerciseDto dto) {
    assertThat(mapper.userEntityToDto(entity)).isEqualTo(dto);
  }

Apart from that (I, too, did that just for demonstrational purposes), we generally don't want to mix array arguments and list arguments just for the sake of it (i.e. arrayToList = true just for some arguments), unless there is a valid reason for it. I implemented this switch because sometimes you need more than a simple iterable (array), where list is the perfect choice.

Would you be so kind as to change this fact (mixed array/list) and the one mentioned above (fori loop when unwrapArrays could/should be used) in the UserMapperTest? I forgot to change that - I wanted to make it "right" after you approved it (becuase, again, I just wnated you to see the possible usages of the annotation).

In GitLab by @11775820 on Oct 16, 2021, 22:22

Commented on server/src/test/java/com/witness/server/unit/mapper/ExerciseMapperTest.java line 54

unwrapArrays does not make sense when you are deserializing an object (not an array). please remove this attribute

In GitLab by @11775820 on Oct 16, 2021, 22:22

Commented on server/src/test/java/com/witness/server/unit/mapper/ExerciseMapperTest.java line 45

unwrapArrays does not make sense when you are deserializing an object (not an array). please remove this attribute

In GitLab by @11775820 on Oct 16, 2021, 22:34

Another one. It's nice that you apparently didn't encounter the issue regarding "element changed after it was removed from the widget tree" or so anymore. However, now I got it, repeatedly, even after cold booting and wiping the emulator. So there's something up definitely.

Please go into login_card.at, function _submit and insert this mounted check:

...

 if (error != null) {
      _showErrorDialog(error);
    }

    if (!mounted) {
      return;
    }

    setState(() {
      _isLoading = false;
    });
  }

With it, I didn't get the error again. That the state is not set anymore when we are already leaving the screen is no problem (and I guess the root cause of this error), so it should be a fix that's acceptable for the time being.

In GitLab by @11712616 on Oct 18, 2021, 19:09

Commented on client/lib/localization/app_en.arb line 113

changed this line in version 3 of the diff

In GitLab by @11712616 on Oct 18, 2021, 19:09

added 1 commit

75dd6ebe - 9: Review: Fix typo

Compare with previous version

In GitLab by @11712616 on Oct 18, 2021, 19:11

Commented on client/lib/providers/exercise_provider.dart line 31

changed this line in version 4 of the diff

In GitLab by @11712616 on Oct 18, 2021, 19:11

added 1 commit

fbbcbb3e - 9: Review: Remove unnecessary linter suppression

Compare with previous version

In GitLab by @11712616 on Oct 18, 2021, 19:46

Commented on client/lib/services/exercise_service.dart line 26

changed this line in version 5 of the diff

In GitLab by @11712616 on Oct 18, 2021, 19:46

added 1 commit

c0545aee - 9: Review: Use toList() instead of List.from()

Compare with previous version