PTBAS-741: Fix issues

* improve test
* improve expiration label
* refactor code in ExternalProjectsMapController
* fix work stacking when it is not needed

Author: MomdAli

src/main/java/de/doubleslash/keeptime/controller/HeimatController.java

@@ -171,28 +171,15 @@ public List<Mapping> getTableRows(final LocalDate currentReportDate, final List<
          if (optionalExistingMapping.isPresent()) {
             final Mapping existingMapping = optionalExistingMapping.get();
 
-            // Ensure we merge projects robustly: include any projects mapped to the same Heimat task
+            // Only include projects that were actually worked on at this date
             final ArrayList<Project> projects = new ArrayList<>(existingMapping.projects());
-            if (optHeimatMapping.isPresent()) {
-               final long mappedTaskId = optHeimatMapping.get().getExternalTaskId();
-               final List<Project> allMappedForTask = mappedProjects.stream()
-                                                                    .filter(
-                                                                          mp -> mp.getExternalTaskId() == mappedTaskId)
-                                                                    .map(ExternalProjectMapping::getProject)
-                                                                    .toList();
-               for (Project p : allMappedForTask) {
-                  boolean alreadyContains = projects.stream().anyMatch(pp -> pp.getId() == p.getId());
-                  if (!alreadyContains) {
-                     projects.add(p);
-                  }
-               }
-            } else {
-               // fallback: ensure current project is present
-               boolean alreadyContains = projects.stream().anyMatch(p -> p.getId() == project.getId());
-               if (!alreadyContains) {
-                  projects.add(project);
-               }
+            // Ensure current project is present (it's already in workedProjectsSet, so it was worked on)
+            boolean alreadyContains = projects.stream().anyMatch(p -> p.getId() == project.getId());
+            if (!alreadyContains) {
+               projects.add(project);
             }
+            // Filter to only keep projects that were actually worked on
+            projects.removeIf(p -> !workedProjectsSet.contains(p));
 
             final long keepTimeSeconds = existingMapping.keeptimeSeconds() + projectWorkSeconds;
             final long heimatSeconds = existingMapping.heimatSeconds();
@@ -214,15 +201,14 @@ public List<Mapping> getTableRows(final LocalDate currentReportDate, final List<
             list.remove(existingMapping);
             list.add(mapping);
          } else {
-            // FIX: when creating a new mapping row for a project that is mapped to a Heimat task,
-            // include ALL Projects that are mapped to the same Heimat task so that projects without
-            // KeepTime entries (no worked time) are also shown in the same row (case 4).
+            // Only include projects that were actually worked on at this date
             final List<Project> projects;
             if (optHeimatMapping.isPresent()) {
                final long mappedTaskId = optHeimatMapping.get().getExternalTaskId();
                projects = mappedProjects.stream()
                                         .filter(mp -> mp.getExternalTaskId() == mappedTaskId)
                                         .map(ExternalProjectMapping::getProject)
+                                        .filter(workedProjectsSet::contains)
                                         .collect(Collectors.toList());
             } else {
                projects = Collections.singletonList(project);
@@ -256,9 +242,12 @@ public List<Mapping> getTableRows(final LocalDate currentReportDate, final List<
             taskName = heimatTask.name() + "\n" + heimatTask.taskHolderName();
          }
 
-         final Mapping mapping = new Mapping(id, true, false,
-               StyledMessage.of(new StyledMessage.TextSegment("Not mapped in KeepTime\n\n" + taskName)), "", times,
+         // Build sync message with task name in bold
+         StyledMessage syncMessage = StyledMessage.of(
+               new StyledMessage.TextSegment("Not mapped in KeepTime\n\n"),
+               new StyledMessage.TextSegment(taskName, true));
 
+         final Mapping mapping = new Mapping(id, true, false, syncMessage, "", times,
                new ArrayList<>(0), heimatNotes, "", heimatTimeSeconds, 0);
          list.add(mapping);
       });
@@ -281,23 +270,8 @@ public List<Mapping> getTableRows(final LocalDate currentReportDate, final List<
             long heimatTimeSeconds = addHeimatTimes(times);
 
             final Optional<Mapping> existingMappingInList = list.stream().filter(m -> m.heimatTaskId == id).findAny();
+            // If there's already a mapping from worked-on projects, don't add non-worked projects to it
             if (existingMappingInList.isPresent()) {
-               Mapping existing = existingMappingInList.get();
-               // only add if not already present
-               boolean alreadyContains = existing.projects()
-                                                 .stream()
-                                                 .anyMatch(
-                                                       p -> p.getId() == externalProjectMapping.getProject().getId());
-               if (!alreadyContains) {
-                  ArrayList<Project> newProjects = new ArrayList<>(existing.projects());
-                  newProjects.add(externalProjectMapping.getProject());
-                  Mapping updated = new Mapping(existing.heimatTaskId, existing.canBeSynced, existing.shouldBeSynced,
-                        existing.syncMessage, existing.bookingHint, existing.existingTimes, newProjects,
-                        existing.heimatNotes, existing.keeptimeNotes, existing.heimatSeconds, existing.keeptimeSeconds);
-                  list.remove(existing);
-                  list.add(updated);
-               }
-               // skip creating a separate entry
                continue;
             }
 

src/main/java/de/doubleslash/keeptime/view/ExternalProjectsMapController.java

@@ -25,7 +25,6 @@
 import de.doubleslash.keeptime.rest.integration.heimat.model.ExistingAndInvalidMappings;
 import de.doubleslash.keeptime.rest.integration.heimat.model.HeimatTask;
 import de.doubleslash.keeptime.viewpopup.SearchCombobox;
-import javafx.application.Platform;
 import javafx.beans.property.SimpleBooleanProperty;
 import javafx.beans.property.SimpleObjectProperty;
 import javafx.beans.property.SimpleStringProperty;
@@ -80,8 +79,25 @@ public ExternalProjectsMapController(final Model model, Controller controller, H
       this.heimatController = heimatController;
    }
 
+   private ExistingAndInvalidMappings existingAndInvalidMappings;
+   private ObservableList<HeimatController.ProjectMapping> newProjectMappings;
+
    public void setStage(final Stage thisStage) {
       this.thisStage = thisStage;
+      // Show invalid mappings dialog when stage is shown
+      thisStage.setOnShown(e -> {
+         if (existingAndInvalidMappings != null && newProjectMappings != null) {
+            List<String> warnings = existingAndInvalidMappings.invalidMappingsAsString();
+            if (!warnings.isEmpty()) {
+               if (showInvalidMappingsDialog(warnings)) {
+                  newProjectMappings.stream()
+                                    .filter(HeimatController.ProjectMapping::isPendingRemoval)
+                                    .forEach(pm -> pm.setHeimatTask(null));
+                  mappingTableView.refresh();
+               }
+            }
+         }
+      });
    }
 
    @FXML
@@ -92,26 +108,14 @@ private void initialize() {
 
       final List<HeimatTask> externalProjects = heimatController.getAllKnownHeimatTasks(tasksForDateDatePicker.getValue());
 
-      final ExistingAndInvalidMappings existingAndInvalidMappings = heimatController.getExistingProjectMappings(
+      existingAndInvalidMappings = heimatController.getExistingProjectMappings(
             externalProjects);
 
 
       final List<HeimatController.ProjectMapping> previousProjectMappings = existingAndInvalidMappings.validMappings();
-      final ObservableList<HeimatController.ProjectMapping> newProjectMappings = FXCollections.observableArrayList(
+      newProjectMappings = FXCollections.observableArrayList(
             previousProjectMappings);
 
-      Platform.runLater(() -> {
-         List<String> warnings = existingAndInvalidMappings.invalidMappingsAsString();
-         if (!warnings.isEmpty()) {
-            if (showInvalidMappingsDialog(warnings)) {
-               newProjectMappings.stream()
-                                 .filter(HeimatController.ProjectMapping::isPendingRemoval)
-                                 .forEach(pm -> pm.setHeimatTask(null));
-               mappingTableView.refresh();
-            }
-         }
-      });
-
       final FilteredList<HeimatController.ProjectMapping> value = new FilteredList<>(newProjectMappings,
             pm -> pm.getProject().isWork());
       mappingTableView.setItems(value);

src/main/java/de/doubleslash/keeptime/view/SettingsController.java

@@ -415,21 +415,25 @@ private void initializeHeimat() {
       heimatPatTextField.textProperty().addListener((observable, oldValue, newValue)->{
          try{
             final JwtDecoder.JWTTokenAttributes jwt = JwtDecoder.parse(newValue);
-            if (!JwtDecoder.isExpired(jwt, LocalDateTime.now())) {
-               heimatExpiresLabel.setText("Expired:");
-               heimatExpiresLabel.setTextFill(Color.RED);
+            final boolean isExpired = !JwtDecoder.isExpired(jwt, LocalDateTime.now());
+            final String expirationDate = jwt.expiration().toString();
+            
+            // Keep label text consistent, show expired status in the value
+            heimatExpiresLabel.setText("Expires:");
+            heimatExpiresLabel.setTextFill(Color.BLACK);
+            
+            if (isExpired) {
+               expirationDateLabel.setText(expirationDate + " (Expired)");
                expirationDateLabel.setTextFill(Color.RED);
             } else {
-               heimatExpiresLabel.setText("Expires:");
-               heimatExpiresLabel.setTextFill(Color.BLACK);
+               expirationDateLabel.setText(expirationDate);
                expirationDateLabel.setTextFill(Color.BLACK);
             }
 
-            expirationDateLabel.setText(jwt.expiration().toString());
-
          } catch(Exception e){
-            heimatExpiresLabel.setText("");
+            heimatExpiresLabel.setText("Expires:");
             expirationDateLabel.setText("Does not seem to be valid");
+            expirationDateLabel.setTextFill(Color.RED);
          }
       });
       heimatValidateConnectionLabel.setText("Not validated.");

src/test/java/de/doubleslash/keeptime/controller/HeimatControllerTest.java

@@ -26,7 +26,6 @@
 import de.doubleslash.keeptime.rest.integration.heimat.model.HeimatTask;
 import de.doubleslash.keeptime.rest.integration.heimat.model.HeimatTime;
 import javafx.scene.paint.Color;
-import javafx.scene.text.Text;
 import org.hamcrest.Matchers;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
@@ -39,7 +38,6 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
-import java.util.stream.Collectors;
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.junit.jupiter.api.Assertions.*;
@@ -520,7 +518,7 @@ void shouldReturnErrorsWhenErrorsOccurred() {
    // shouldOnlyUpdateHeimatWhenSomethingHasChanged (not needed - user should decide)
 
    @Test
-   void shouldNotCreateDuplicateHeimatEntryWhenMultipleProjectsMappedAndSomeHaveWork() {
+   void shouldOnlyShowWorkedOnProjectsWhenMultipleProjectsMappedAndSomeHaveWork() {
       // ARRANGE
       // project 1 has work, project 2 does not
       final Work work1 = new Work(now.minusMinutes(10), now, workProject1, "Notes 1");
@@ -542,10 +540,11 @@ void shouldNotCreateDuplicateHeimatEntryWhenMultipleProjectsMappedAndSomeHaveWor
       assertThat(tableRows.size(), Matchers.is(1));
       final HeimatController.Mapping mapping = tableRows.get(0);
 
-      // The mapping should combine both projects in .projects()
-      assertThat(mapping.projects(), Matchers.containsInAnyOrder(workProject1, workProject2));
+      // Only project1 should be in the list since project2 was not worked on
+      assertThat(mapping.projects(), Matchers.containsInAnyOrder(workProject1));
+      assertThat(mapping.projects().size(), Matchers.is(1));
 
-      // The mapping should show KeepTime time for workProject1, and 0 for workProject2 (which is included in .projects() but has no time)
+      // The mapping should show KeepTime time for workProject1
       assertThat(mapping.keeptimeSeconds(), Matchers.is(10 * 60L));
       assertThat(mapping.keeptimeNotes(), Matchers.is("Notes 1"));
 
@@ -556,4 +555,44 @@ void shouldNotCreateDuplicateHeimatEntryWhenMultipleProjectsMappedAndSomeHaveWor
       assertThat(mapping.syncMessage().toPlainText(), Matchers.not(Matchers.containsString("Present in HEIMAT but not KeepTime")));
       assertThat(mapping.syncMessage().toPlainText(), Matchers.containsString(project1To1Mapping.getExternalTaskName()));
    }
+
+   @Test
+   void shouldNotCreateDuplicateHeimatEntryWhenMultipleProjectsMappedAndBothHaveWork() {
+      // ARRANGE
+      // Both projects have work and are mapped to the same Heimat task
+      final Work work1 = new Work(now.minusMinutes(10), now, workProject1, "Notes 1");
+      workItems.add(work1);
+      final Work work2 = new Work(now.minusMinutes(5), now, workProject2, "Notes 2");
+      workItems.add(work2);
+
+      externalMappings.add(project1To1Mapping);
+      externalMappings.add(project2To1Mapping);
+
+      // The mapped Heimat task has already been booked in Heimat
+      final HeimatTime existingTime = new HeimatTime(project1To1Mapping.getExternalTaskId(), now.toLocalDate(), null,
+            null, 15, "Heimat note", 99);
+      when(mockedHeimatAPI.getMyTimes(now.toLocalDate())).thenReturn(Arrays.asList(existingTime));
+
+      // ACT
+      final List<HeimatController.Mapping> tableRows = heimatController.getTableRows(now.toLocalDate(), workItems);
+
+      // ASSERT
+      // There should be exactly one row for this Heimat task (no duplicates)
+      assertThat(tableRows.size(), Matchers.is(1));
+      final HeimatController.Mapping mapping = tableRows.get(0);
+
+      // Both projects should be in the list since both were worked on
+      assertThat(mapping.projects(), Matchers.containsInAnyOrder(workProject1, workProject2));
+
+      // The mapping should combine KeepTime time from both projects
+      assertThat(mapping.keeptimeSeconds(), Matchers.is((10 + 5) * 60L));
+      assertThat(mapping.keeptimeNotes(), Matchers.is("Notes 1. Notes 2"));
+
+      // There should be Heimat time and notes as well
+      assertThat(mapping.heimatNotes(), Matchers.is("Heimat note"));
+      assertThat(mapping.heimatSeconds(), Matchers.is(15 * 60L));
+
+      assertThat(mapping.syncMessage().toPlainText(), Matchers.not(Matchers.containsString("Present in HEIMAT but not KeepTime")));
+      assertThat(mapping.syncMessage().toPlainText(), Matchers.containsString(project1To1Mapping.getExternalTaskName()));
+   }
 }