Merge pull request #277 from frankframework/fix/improve-roadmap-design

Fix/improve roadmap design

Author: stijnpotters1

src/main/frontend/src/app/pages/release-roadmap/milestone-row/milestone-row.component.spec.ts

@@ -3,7 +3,7 @@ import { DatePipe } from '@angular/common';
 import { SimpleChanges } from '@angular/core';
 import { MilestoneRowComponent } from './milestone-row.component';
 import { Milestone } from '../../../services/milestone.service';
-import { Issue, IssuePriority } from '../../../services/issue.service';
+import { Issue } from '../../../services/issue.service';
 import { GitHubStates } from '../../../app.service';
 import { IssueBarComponent } from '../issue-bar/issue-bar.component';
 import { NoopAnimationsModule } from '@angular/platform-browser/animations';
@@ -177,20 +177,5 @@ describe('MilestoneRowComponent', () => {
 
       expect(component.trackCount).toBeGreaterThan(1);
     });
-
-    it('should sort issues by priority (critical first) before layout', () => {
-      const lowPrio: Issue = { ...MOCK_OPEN_ISSUE, id: 'low', issuePriority: { name: 'Low Prio' } as IssuePriority };
-      const critPrio: Issue = {
-        ...MOCK_OPEN_ISSUE,
-        id: 'crit',
-        issuePriority: { name: 'Critical Prio' } as IssuePriority,
-      };
-      initializeComponent(MOCK_MILESTONE, [lowPrio, critPrio]);
-
-      const critIndex = component.positionedIssues.findIndex((p) => p.issue.id === 'crit');
-      const lowIndex = component.positionedIssues.findIndex((p) => p.issue.id === 'low');
-
-      expect(critIndex).toBeLessThan(lowIndex);
-    });
   });
 });

src/main/frontend/src/app/pages/release-roadmap/milestone-row/milestone-row.component.ts

@@ -2,7 +2,7 @@ import { Component, Input, OnChanges, SimpleChanges } from '@angular/core';
 import { CommonModule, DatePipe } from '@angular/common';
 import { IssueBarComponent } from '../issue-bar/issue-bar.component';
 import { Milestone } from '../../../services/milestone.service';
-import { Issue, IssuePriority } from '../../../services/issue.service';
+import { Issue } from '../../../services/issue.service';
 import { GitHubStates } from '../../../app.service';
 
 interface PositionedIssue {
@@ -113,11 +113,6 @@ export class MilestoneRowComponent implements OnChanges {
       }
     }
 
-    for (const issues of quarterMap.values()) {
-      issues.open = this.getSortedIssues(issues.open);
-      issues.closed = this.getSortedIssues(issues.closed);
-    }
-
     return quarterMap;
   }
 
@@ -177,8 +172,7 @@ export class MilestoneRowComponent implements OnChanges {
       return { positionedIssues: [], trackCount: 0 };
     }
 
-    const trackCount = this.estimateTrackCount(issues, window);
-    const issuesByTrack = this.distributeIssuesRoundRobin(issues, trackCount);
+    const issuesByTrack = this.distributeIssuesWithBinPacking(issues, window);
     const positionedIssues: PositionedIssue[] = [];
 
     for (const [trackIndex, trackIssues] of issuesByTrack.entries()) {
@@ -188,6 +182,7 @@ export class MilestoneRowComponent implements OnChanges {
         (sum, issue) => sum + this.getIssueDurationMsWithMinWidth(issue),
         0,
       );
+
       const totalWhitespace = window.end - window.start - totalIssueDuration;
       const gapSize = totalWhitespace > 0 ? totalWhitespace / (trackIssues.length + 1) : 0;
       let cursor = window.start + gapSize;
@@ -214,44 +209,42 @@ export class MilestoneRowComponent implements OnChanges {
     };
   }
 
-  private estimateTrackCount(issues: Issue[], window: PlanningWindow): number {
-    const windowDurationMs = window.end - window.start;
-    if (windowDurationMs <= 0) return issues.length || 1;
-
-    const totalDurationWithGaps = issues.reduce(
-      (sum, issue) => sum + this.getIssueDurationMsWithMinWidth(issue) + this.GAP_MS,
-      -this.GAP_MS,
-    );
-
-    return Math.max(1, Math.ceil(totalDurationWithGaps / windowDurationMs));
-  }
-
-  private distributeIssuesRoundRobin(issues: Issue[], trackCount: number): Map<number, Issue[]> {
+  private distributeIssuesWithBinPacking(issues: Issue[], window: PlanningWindow): Map<number, Issue[]> {
     const issuesByTrack = new Map<number, Issue[]>();
-    if (trackCount === 0) return issuesByTrack;
+    const windowDurationMs = window.end - window.start;
+    const trackCapacities = new Map<number, number>();
+    let currentTrack = 0;
+
+    for (const issue of issues) {
+      const issueDuration = this.getIssueDurationMsWithMinWidth(issue);
+      const spaceNeeded = issueDuration + this.GAP_MS;
+
+      let placed = false;
+      for (let trackIndex = 0; trackIndex <= currentTrack; trackIndex++) {
+        const trackUsed = trackCapacities.get(trackIndex) ?? 0;
+        const trackRemaining = windowDurationMs - trackUsed;
+
+        if (trackRemaining >= spaceNeeded) {
+          if (!issuesByTrack.has(trackIndex)) {
+            issuesByTrack.set(trackIndex, []);
+          }
+          issuesByTrack.get(trackIndex)!.push(issue);
+          trackCapacities.set(trackIndex, trackUsed + spaceNeeded);
+          placed = true;
+          break;
+        }
+      }
 
-    for (let index = 0; index < trackCount; index++) issuesByTrack.set(index, []);
-    for (const [index, issue] of issues.entries()) issuesByTrack.get(index % trackCount)!.push(issue);
+      if (!placed) {
+        currentTrack++;
+        issuesByTrack.set(currentTrack, [issue]);
+        trackCapacities.set(currentTrack, spaceNeeded);
+      }
+    }
 
     return issuesByTrack;
   }
 
-  private getSortedIssues(issues: Issue[]): Issue[] {
-    const priorityOrder: Record<string, number> = { critical: 1, high: 2, medium: 3, low: 4, no: 5 };
-
-    return [...issues].sort((a, b) => {
-      const priorityA = priorityOrder[this.getPriorityKey(a.issuePriority)] ?? 5;
-      const priorityB = priorityOrder[this.getPriorityKey(b.issuePriority)] ?? 5;
-      if (priorityA !== priorityB) return priorityA - priorityB;
-
-      const pointsA = a.points ?? this.DEFAULT_POINTS;
-      const pointsB = b.points ?? this.DEFAULT_POINTS;
-      if (pointsA !== pointsB) return pointsB - pointsA;
-
-      return b.number - a.number;
-    });
-  }
-
   private calculateBarPosition(startDate: Date, durationDays: number): Record<string, string> {
     const startDays = (startDate.getTime() - this.timelineStartDate.getTime()) / (1000 * 3600 * 24);
     const leftPercentage = (startDays / this.totalTimelineDays) * 100;
@@ -290,14 +283,4 @@ export class MilestoneRowComponent implements OnChanges {
     const total = this.milestone.openIssueCount + this.milestone.closedIssueCount;
     this.progressPercentage = total === 0 ? 0 : Math.round((this.milestone.closedIssueCount / total) * 100);
   }
-
-  private getPriorityKey(priority: IssuePriority | undefined | null): string {
-    if (!priority?.name) return 'no';
-    const lowerCaseName = priority.name.toLowerCase();
-    const keys = ['critical', 'high', 'medium', 'low'];
-    for (const key of keys) {
-      if (lowerCaseName.includes(key)) return key;
-    }
-    return 'no';
-  }
 }

src/main/java/org/frankframework/insights/common/configuration/properties/CorsProperties.java

@@ -1,11 +1,10 @@
 package org.frankframework.insights.common.configuration.properties;
 
+import java.util.List;
 import lombok.Getter;
 import lombok.Setter;
 import org.springframework.boot.context.properties.ConfigurationProperties;
 
-import java.util.List;
-
 @ConfigurationProperties(prefix = "cors.allowed")
 @Getter
 @Setter

src/main/java/org/frankframework/insights/issue/IssueService.java

@@ -36,6 +36,7 @@
 @Slf4j
 public class IssueService {
     private static final String ISSUE_TYPE_EPIC_NAME = "Epic";
+    private static final double DEFAULT_POINTS = 3.0;
 
     private final GitHubClient gitHubClient;
     private final Mapper mapper;
@@ -241,8 +242,9 @@ private Set<Issue> saveIssues(Set<Issue> issues) {
      */
     public Set<IssueResponse> getIssuesByReleaseId(String releaseId) throws ReleaseNotFoundException {
         Release release = releaseService.checkIfReleaseExists(releaseId);
-        Set<Issue> rootIssues = issueRepository.findIssuesByReleaseId(release.getId());
-        return buildIssueResponseTree(rootIssues);
+        Set<Issue> allIssues = issueRepository.findIssuesByReleaseId(release.getId());
+		Set<Issue> rootIssues = filterRootIssues(allIssues);
+		return buildIssueResponseTree(rootIssues);
     }
 
     /**
@@ -253,7 +255,8 @@ public Set<IssueResponse> getIssuesByReleaseId(String releaseId) throws ReleaseN
      */
     public Set<IssueResponse> getIssuesByMilestoneId(String milestoneId) throws MilestoneNotFoundException {
         Milestone milestone = milestoneService.checkIfMilestoneExists(milestoneId);
-        Set<Issue> rootIssues = issueRepository.findDistinctByMilestoneId(milestone.getId());
+        Set<Issue> allIssues = issueRepository.findDistinctByMilestoneId(milestone.getId());
+        Set<Issue> rootIssues = filterRootIssues(allIssues);
         return buildIssueResponseTree(rootIssues);
     }
 
@@ -263,7 +266,24 @@ public Set<IssueResponse> getIssuesByMilestoneId(String milestoneId) throws Mile
      */
     public Set<IssueResponse> getFutureEpicIssues() {
         Set<Issue> futureEpicIssues = issueRepository.findIssuesByIssueTypeNameAndMilestoneIsNull(ISSUE_TYPE_EPIC_NAME);
-        return buildIssueResponseTree(futureEpicIssues);
+        return buildIssueResponseTreeWithoutFiltering(futureEpicIssues);
+    }
+
+    /**
+     * Filters out issues that are sub-issues of other issues in the set.
+     * @param issues the set of all issues
+     * @return a set of root issues (issues that are not sub-issues of other issues in the set)
+     */
+    private Set<Issue> filterRootIssues(Set<Issue> issues) {
+        Set<String> allSubIssueIds = issues.stream()
+                .filter(issue -> issue.getSubIssues() != null)
+                .flatMap(issue -> issue.getSubIssues().stream())
+                .map(Issue::getId)
+                .collect(Collectors.toSet());
+
+        return issues.stream()
+                .filter(issue -> !allSubIssueIds.contains(issue.getId()))
+                .collect(Collectors.toSet());
     }
 
     /**
@@ -272,6 +292,20 @@ public Set<IssueResponse> getFutureEpicIssues() {
      * @return a set of IssueResponse objects representing the root issues and their sub-issues, with labels included
      */
     private Set<IssueResponse> buildIssueResponseTree(Set<Issue> rootIssues) {
+        Set<String> allIds = collectAllIssueIdsRecursively(rootIssues);
+        Map<String, Set<LabelResponse>> labelsMap = fetchLabelsForIssueIds(allIds);
+        return rootIssues.stream()
+                .map(issue -> mapIssueTreeWithLabels(issue, labelsMap))
+                .filter(this::hasRelevantLabelsRecursively)
+                .collect(Collectors.toSet());
+    }
+
+    /**
+     * Builds a tree of IssueResponse objects without filtering by labels.
+     * @param rootIssues the set of root issues to build the tree from
+     * @return a set of IssueResponse objects representing the root issues and their sub-issues, with labels included
+     */
+    private Set<IssueResponse> buildIssueResponseTreeWithoutFiltering(Set<Issue> rootIssues) {
         Set<String> allIds = collectAllIssueIdsRecursively(rootIssues);
         Map<String, Set<LabelResponse>> labelsMap = fetchLabelsForIssueIds(allIds);
         return rootIssues.stream()
@@ -309,6 +343,7 @@ private Stream<String> flattenIssueIds(Issue issue) {
     private Map<String, Set<LabelResponse>> fetchLabelsForIssueIds(Set<String> issueIds) {
         Set<IssueLabel> labels = issueLabelRepository.findAllByIssue_IdIn(new ArrayList<>(issueIds));
         return labels.stream()
+                .filter(l -> labelService.isLabelIncluded(l.getLabel()))
                 .collect(Collectors.groupingBy(
                         l -> l.getIssue().getId(),
                         Collectors.mapping(l -> mapper.toDTO(l.getLabel(), LabelResponse.class), Collectors.toSet())));
@@ -328,6 +363,10 @@ private IssueResponse mapIssueTreeWithLabels(Issue issue, Map<String, Set<LabelR
 
         response.setLabels(labelsMap.getOrDefault(issue.getId(), Set.of()));
         response.setSubIssues(mapSubIssuesToResponses(issue, labelsMap));
+
+        double totalPoints = calculateTotalPoints(issue, response);
+        response.setPoints(totalPoints);
+
         return response;
     }
 
@@ -374,12 +413,49 @@ private Set<IssueResponse> mapSubIssuesToResponses(Issue issue, Map<String, Set<
         if (issue.getSubIssues() != null && !issue.getSubIssues().isEmpty()) {
             return issue.getSubIssues().stream()
                     .map(sub -> mapIssueTreeWithLabels(sub, labelsMap))
+                    .filter(this::hasRelevantLabelsRecursively)
                     .collect(Collectors.toSet());
         } else {
             return Set.of();
         }
     }
 
+    /**
+     * Checks if an issue or any of its sub-issues have relevant labels.
+     * @param issueResponse the issue response to check
+     * @return true if the issue or any of its sub-issues have at least one relevant label
+     */
+    private boolean hasRelevantLabelsRecursively(IssueResponse issueResponse) {
+        if (issueResponse.getLabels() != null && !issueResponse.getLabels().isEmpty()) {
+            return true;
+        }
+        if (issueResponse.getSubIssues() != null
+                && !issueResponse.getSubIssues().isEmpty()) {
+            return issueResponse.getSubIssues().stream().anyMatch(this::hasRelevantLabelsRecursively);
+        }
+        return false;
+    }
+
+    /**
+     * Calculates the total points for an issue, including its own points and all sub-issue points.
+     * Uses DEFAULT_POINTS (3.0) for issues without assigned points.
+     * @param issue the issue entity
+     * @param response the issue response with mapped sub-issues
+     * @return the total points for the issue and all its sub-issues
+     */
+    private double calculateTotalPoints(Issue issue, IssueResponse response) {
+        double issuePoints = issue.getPoints() != null ? issue.getPoints() : DEFAULT_POINTS;
+
+        if (response.getSubIssues() != null && !response.getSubIssues().isEmpty()) {
+            double subIssuesPoints = response.getSubIssues().stream()
+                    .mapToDouble(IssueResponse::getPoints)
+                    .sum();
+            return issuePoints + subIssuesPoints;
+        }
+
+        return issuePoints;
+    }
+
     /**
      * Get all issues from the database
      * @return a map of issue id to issue

src/main/java/org/frankframework/insights/label/LabelService.java

@@ -164,4 +164,13 @@ private void saveLabels(Set<Label> labels) {
         List<Label> savedLabels = labelRepository.saveAll(labels);
         log.info("Successfully saved {} labels", savedLabels.size());
     }
+
+    /**
+     * Checks if a label is included based on its color.
+     * @param label the label to check
+     * @return true if the label's color is in the included labels list, false otherwise
+     */
+    public boolean isLabelIncluded(Label label) {
+        return label != null && includedLabels.contains(label.getColor().toUpperCase());
+    }
 }