Skip to content

Commit 161c9fa

Browse files
Valentin Grünerclaude
authored and
FelixTJDietrich
committed
fix(server): register the area-guidance context and let reads answer absence
Three findings from reviewing the branch, none of which local runs had shown. The full-context budget failed in CI and not here, because the local suite never runs SpringTestContextArchitectureTest — `pnpm run test:server:verification` is the command that does, and it is what CI runs. PracticeAreaStatusIntegrationTest earns a second application context: its nested class registers an AreaGuidanceProvider bean, and "a provider is present" cannot be tested from the same context as "none is registered". That is a reviewed cost, so it is now recorded as one with its justification rather than left to fail the gate. FeedbackResponseService read the caller through the throwing lookup on all three methods, including its two reads. CurrentDeveloperLookup says the opposite in as many words: absence is a normal answer for a read, and only a write has no meaningful empty result. Its two neighbours already keep that; this one now does too, so a signed-in person who was never synced as a developer sees no response and zero counts instead of an error. ObservationService still declared the visibility policy it stopped using when the reflection surface moved out. The field was injected and read nowhere. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017XBuU2tfPFntHoE34Cmw1Z
1 parent c6417c3 commit 161c9fa

4 files changed

Lines changed: 67 additions & 38 deletions

File tree

‎server/src/main/java/de/tum/cit/aet/hephaestus/practices/feedback/FeedbackResponseService.java‎

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -77,18 +77,32 @@ public FeedbackResponseDTO submitResponse(
7777
return currentResponse(feedbackId, recipientId).orElseGet(() -> FeedbackResponseDTO.none(feedbackId));
7878
}
7979

80-
/** The recipient's answer as it currently stands, or empty when they have said nothing that still holds. */
80+
/**
81+
* The recipient's answer as it currently stands, or empty when they have said nothing that still holds.
82+
*
83+
* <p>Empty also covers a caller who is signed in but not a synced developer: a read answers them with
84+
* nothing rather than an error, which is the contract {@link CurrentDeveloperLookup} states.
85+
*/
8186
@Transactional(readOnly = true)
8287
public Optional<FeedbackResponseDTO> getLatestResponse(WorkspaceContext workspaceContext, UUID feedbackId) {
8388
Feedback feedback = requireDeliveredFeedback(workspaceContext.id(), feedbackId);
84-
long recipientId = currentDeveloperLookup.currentDeveloperIdElseThrow();
89+
Optional<Long> recipient = currentDeveloperLookup.currentDeveloperId();
90+
if (recipient.isEmpty()) {
91+
return Optional.empty();
92+
}
93+
long recipientId = recipient.get();
8594
requireRecipient(feedback, recipientId);
8695
return currentResponse(feedbackId, recipientId);
8796
}
8897

98+
/** Zeroes for a caller who is not a synced developer, for the same reason as {@link #getLatestResponse}. */
8999
@Transactional(readOnly = true)
90100
public FeedbackEngagementDTO getEngagement(WorkspaceContext workspaceContext) {
91-
long recipientId = currentDeveloperLookup.currentDeveloperIdElseThrow();
101+
Optional<Long> recipient = currentDeveloperLookup.currentDeveloperId();
102+
if (recipient.isEmpty()) {
103+
return new FeedbackEngagementDTO(0L, 0L, 0L);
104+
}
105+
long recipientId = recipient.get();
92106
Map<FeedbackResolution, Long> counts = new EnumMap<>(FeedbackResolution.class);
93107
reactionRepository
94108
.countByReactorAndWorkspaceGroupByAction(recipientId, workspaceContext.id())

‎server/src/main/java/de/tum/cit/aet/hephaestus/practices/observation/ObservationService.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,6 @@ public class ObservationService {
4343
private final ObservationRepository observationRepository;
4444
private final FeedbackObservationRepository feedbackObservationRepository;
4545
private final UserRepository userRepository;
46-
private final ObservationVisibilityPolicy visibilityPolicy;
4746
private final ReviewRunTargetLookup reviewRunTargetLookup;
4847

4948
/** Feed ordering: by observation time or by severity (direction applies to both). */

‎server/src/test/java/de/tum/cit/aet/hephaestus/architecture/SpringTestContextArchitectureTest.java‎

Lines changed: 17 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -38,30 +38,25 @@ class SpringTestContextArchitectureTest extends HephaestusArchitectureTest {
3838
assignment("integration.core.sync.api.SyncControllerIntegrationTest", "sync-controller-focused"),
3939
assignment("integration.slack.detection.ConversationThreadDetectionIntegrationTest", "slack-ingest"),
4040
assignment("integration.slack.SlackConsentLifecycleE2EIntegrationTest", "slack-lifecycle"),
41-
assignment("integration.slack.channel.SlackChannelAdminControllerIntegrationTest", "slack-signed")
41+
assignment("integration.slack.channel.SlackChannelAdminControllerIntegrationTest", "slack-signed"),
42+
assignment("practices.PracticeAreaStatusIntegrationTest", "area-guidance-provider")
4243
);
4344

44-
private static final Map<String, String> FULL_CONTEXT_JUSTIFICATIONS = Map.of(
45-
"base",
46-
"shared PostgreSQL, HTTP, security, and application acceptance context",
47-
"real-auth",
48-
"real OAuth and authentication wiring without test security",
49-
"startup",
50-
"production main-method startup instrumentation",
51-
"dev-login",
52-
"dev-login feature-property behavior",
53-
"github-live",
54-
"explicitly selected live GitHub profile and credentials",
55-
"outline-enabled",
56-
"enabled Outline integration wiring",
57-
"slack-ingest",
58-
"enabled Slack ingest wiring with its review-submission spy",
59-
"slack-lifecycle",
60-
"agent submission boundary override",
61-
"slack-signed",
62-
"enabled signed Slack HTTP wiring",
63-
"sync-controller-focused",
64-
"controlled sync provider and runner behavior"
45+
private static final Map<String, String> FULL_CONTEXT_JUSTIFICATIONS = Map.ofEntries(
46+
Map.entry("base", "shared PostgreSQL, HTTP, security, and application acceptance context"),
47+
Map.entry("real-auth", "real OAuth and authentication wiring without test security"),
48+
Map.entry("startup", "production main-method startup instrumentation"),
49+
Map.entry("dev-login", "dev-login feature-property behavior"),
50+
Map.entry("github-live", "explicitly selected live GitHub profile and credentials"),
51+
Map.entry("outline-enabled", "enabled Outline integration wiring"),
52+
Map.entry("slack-ingest", "enabled Slack ingest wiring with its review-submission spy"),
53+
Map.entry("slack-lifecycle", "agent submission boundary override"),
54+
Map.entry("slack-signed", "enabled signed Slack HTTP wiring"),
55+
Map.entry("sync-controller-focused", "controlled sync provider and runner behavior"),
56+
Map.entry(
57+
"area-guidance-provider",
58+
"a registered AreaGuidanceProvider bean, which absence cannot be tested against"
59+
)
6560
);
6661

6762
private static final Set<String> SPRING_BOOT_TESTS = names(

‎server/src/test/java/de/tum/cit/aet/hephaestus/practices/feedback/FeedbackResponseServiceTest.java‎

Lines changed: 33 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,14 @@ void setUp() {
5757
service = new FeedbackResponseService(reactionRepository, feedbackRepository, currentDeveloperLookup);
5858
workspaceContext = new WorkspaceContext(WORKSPACE_ID, "test-ws", "Test WS", null, null, false, false, Set.of());
5959
appended.clear();
60+
// Both shapes of the port: a write demands an id, a read accepts its absence. Lenient because most
61+
// tests exercise only one of the two paths.
62+
org.mockito.Mockito.lenient()
63+
.when(currentDeveloperLookup.currentDeveloperIdElseThrow())
64+
.thenReturn(CONTRIBUTOR_ID);
65+
org.mockito.Mockito.lenient()
66+
.when(currentDeveloperLookup.currentDeveloperId())
67+
.thenReturn(Optional.of(CONTRIBUTOR_ID));
6068
org.mockito.Mockito.lenient()
6169
.when(reactionRepository.save(any(Reaction.class)))
6270
.thenAnswer(invocation -> {
@@ -130,7 +138,6 @@ void shouldRecordUsefulnessAndResolutionTogether() {
130138
when(feedbackRepository.findByIdAndWorkspaceId(FEEDBACK_ID, WORKSPACE_ID)).thenReturn(
131139
Optional.of(feedback)
132140
);
133-
when(currentDeveloperLookup.currentDeveloperIdElseThrow()).thenReturn(CONTRIBUTOR_ID);
134141
var request = new FeedbackResponseRequestDTO(
135142
FeedbackUsefulness.HELPFUL,
136143
FeedbackResolution.ADDRESSED,
@@ -154,7 +161,6 @@ void shouldRejectEmptyResponse() {
154161
when(feedbackRepository.findByIdAndWorkspaceId(FEEDBACK_ID, WORKSPACE_ID)).thenReturn(
155162
Optional.of(feedback)
156163
);
157-
when(currentDeveloperLookup.currentDeveloperIdElseThrow()).thenReturn(CONTRIBUTOR_ID);
158164

159165
assertThatThrownBy(() ->
160166
service.submitResponse(
@@ -173,7 +179,6 @@ void addressedFeedbackSaves() {
173179
when(feedbackRepository.findByIdAndWorkspaceId(FEEDBACK_ID, WORKSPACE_ID)).thenReturn(
174180
Optional.of(feedback)
175181
);
176-
when(currentDeveloperLookup.currentDeveloperIdElseThrow()).thenReturn(CONTRIBUTOR_ID);
177182
when(feedbackRepository.findHeadlineRecurrenceKey(FEEDBACK_ID)).thenReturn(Optional.of("ck-abc123"));
178183

179184
var request = new FeedbackResponseRequestDTO(null, FeedbackResolution.ADDRESSED, null, null);
@@ -215,7 +220,6 @@ void disputedWithExplanationSaves() {
215220
when(feedbackRepository.findByIdAndWorkspaceId(FEEDBACK_ID, WORKSPACE_ID)).thenReturn(
216221
Optional.of(feedback)
217222
);
218-
when(currentDeveloperLookup.currentDeveloperIdElseThrow()).thenReturn(CONTRIBUTOR_ID);
219223

220224
var request = new FeedbackResponseRequestDTO(
221225
null,
@@ -235,7 +239,6 @@ void notApplicableSaves() {
235239
when(feedbackRepository.findByIdAndWorkspaceId(FEEDBACK_ID, WORKSPACE_ID)).thenReturn(
236240
Optional.of(feedback)
237241
);
238-
when(currentDeveloperLookup.currentDeveloperIdElseThrow()).thenReturn(CONTRIBUTOR_ID);
239242

240243
var request = new FeedbackResponseRequestDTO(
241244
null,
@@ -254,7 +257,6 @@ void disputedWithoutExplanationThrows() {
254257
when(feedbackRepository.findByIdAndWorkspaceId(FEEDBACK_ID, WORKSPACE_ID)).thenReturn(
255258
Optional.of(feedback)
256259
);
257-
when(currentDeveloperLookup.currentDeveloperIdElseThrow()).thenReturn(CONTRIBUTOR_ID);
258260

259261
var request = new FeedbackResponseRequestDTO(null, FeedbackResolution.DISPUTED, null, null);
260262
assertThatThrownBy(() -> service.submitResponse(workspaceContext, FEEDBACK_ID, request))
@@ -268,7 +270,6 @@ void disputedWithBlankExplanationThrows() {
268270
when(feedbackRepository.findByIdAndWorkspaceId(FEEDBACK_ID, WORKSPACE_ID)).thenReturn(
269271
Optional.of(feedback)
270272
);
271-
when(currentDeveloperLookup.currentDeveloperIdElseThrow()).thenReturn(CONTRIBUTOR_ID);
272273

273274
var request = new FeedbackResponseRequestDTO(null, FeedbackResolution.DISPUTED, " ", null);
274275
assertThatThrownBy(() -> service.submitResponse(workspaceContext, FEEDBACK_ID, request))
@@ -310,7 +311,6 @@ void returnsLatestWhenPresent() {
310311
when(feedbackRepository.findByIdAndWorkspaceId(FEEDBACK_ID, WORKSPACE_ID)).thenReturn(
311312
Optional.of(feedback)
312313
);
313-
when(currentDeveloperLookup.currentDeveloperIdElseThrow()).thenReturn(CONTRIBUTOR_ID);
314314

315315
appended.add(
316316
Reaction.builder()
@@ -336,13 +336,25 @@ void returnsEmptyWhenNone() {
336336
when(feedbackRepository.findByIdAndWorkspaceId(FEEDBACK_ID, WORKSPACE_ID)).thenReturn(
337337
Optional.of(feedback)
338338
);
339-
when(currentDeveloperLookup.currentDeveloperIdElseThrow()).thenReturn(CONTRIBUTOR_ID);
340339

341340
Optional<FeedbackResponseDTO> result = service.getLatestResponse(workspaceContext, FEEDBACK_ID);
342341

343342
assertThat(result).isEmpty();
344343
}
345344

345+
@Test
346+
@DisplayName("answers a signed-in non-developer with nothing rather than an error")
347+
void returnsEmptyForACallerWhoIsNotASyncedDeveloper() {
348+
// A read answers absence with absence — the contract CurrentDeveloperLookup states, and the one
349+
// the reflection and review-history surfaces already keep.
350+
when(feedbackRepository.findByIdAndWorkspaceId(FEEDBACK_ID, WORKSPACE_ID)).thenReturn(
351+
Optional.of(createFeedback(CONTRIBUTOR_ID))
352+
);
353+
when(currentDeveloperLookup.currentDeveloperId()).thenReturn(Optional.empty());
354+
355+
assertThat(service.getLatestResponse(workspaceContext, FEEDBACK_ID)).isEmpty();
356+
}
357+
346358
@Test
347359
void throwsWhenFeedbackNotInWorkspace() {
348360
when(feedbackRepository.findByIdAndWorkspaceId(FEEDBACK_ID, WORKSPACE_ID)).thenReturn(Optional.empty());
@@ -357,9 +369,19 @@ void throwsWhenFeedbackNotInWorkspace() {
357369
class GetEngagement {
358370

359371
@Test
360-
void returnsCorrectCounts() {
361-
when(currentDeveloperLookup.currentDeveloperIdElseThrow()).thenReturn(CONTRIBUTOR_ID);
372+
@DisplayName("counts zero for a signed-in non-developer instead of failing")
373+
void returnsZeroesForACallerWhoIsNotASyncedDeveloper() {
374+
when(currentDeveloperLookup.currentDeveloperId()).thenReturn(Optional.empty());
362375

376+
FeedbackEngagementDTO result = service.getEngagement(workspaceContext);
377+
378+
assertThat(result.addressed()).isZero();
379+
assertThat(result.disputed()).isZero();
380+
assertThat(result.notApplicable()).isZero();
381+
}
382+
383+
@Test
384+
void returnsCorrectCounts() {
363385
var addressedProjection = new ReactionRepository.ActionCountProjection() {
364386
@Override
365387
public String getAction() {
@@ -397,7 +419,6 @@ public Long getCount() {
397419
@Test
398420
@DisplayName("returns all zeros when no reaction exists")
399421
void returnsZerosWhenEmpty() {
400-
when(currentDeveloperLookup.currentDeveloperIdElseThrow()).thenReturn(CONTRIBUTOR_ID);
401422
when(reactionRepository.countByReactorAndWorkspaceGroupByAction(CONTRIBUTOR_ID, WORKSPACE_ID)).thenReturn(
402423
List.of()
403424
);

0 commit comments

Comments
 (0)