fix: quest test alerts missing static maps#153
Conversation
…ormat The BuildQuestWebhook method was sending quest data in a flat format (quest_type, quest_reward_type, pokemon_id) that doesn't match PoracleNG's expected QuestWebhook structure. Without a proper `rewards` array, the enricher couldn't resolve imgUrl, causing the tileservercache to fail and produce empty staticMap fields. Restructured to use Golbat's webhook format with a typed rewards array and correct field names (type, target, title). Also fixed BuildInvasionWebhook sending grunt_type as string instead of int, which caused silent JSON deserialization failure in PoracleNG. Closes #152
c789c34 to
a0738e9
Compare
hokiepokedad2
left a comment
There was a problem hiding this comment.
Code Review — PR #153
Executive Summary
| Metric | Rating |
|---|---|
| Overall | ✅ Approved |
| Code Quality | Good |
| Requirements Satisfied | Yes |
| Architecture Fit | Consistent |
| Risk Level | Low |
Review Findings
No critical or major issues found.
Minor Observations (Informational)
| File | Line | Observation | Notes |
|---|---|---|---|
TestAlertService.cs |
239 | Stardust default 1000 | Sensible default; matches typical stardust quest rewards |
TestAlertService.cs |
243 | Candy amount hardcoded to 3 | Reasonable for test purposes |
TestAlertService.cs |
251 | Mega energy hardcoded to 50 | Standard mega energy amount |
TestAlertService.cs |
254 | Default case falls back to pokemon encounter | Good safety net — ensures imgUrl is always resolvable |
TestAlertService.cs |
267 | title hardcoded to catch_3 |
Acceptable for test alerts — the quest title is cosmetic in the DTS template (questString is resolved by PoracleNG's translation layer from this key) |
What's Good
-
Root cause correctly identified: The Golbat webhook format requires a
rewardsarray with{type, info}objects. The old flat format (quest_reward_type,pokemon_id) was silently ignored by PoracleNG'sQuestWebhookstruct, leavingrewardsempty → noimgUrl→ tileserver failure. -
Reward type coverage is thorough: All 5 reward types (item, stardust, candy, pokemon, mega energy) are handled with correct info field structures matching
parseQuestReward()in the PoracleNG processor. -
Default case is a smart addition: Unknown reward types fall back to pokemon encounter, ensuring
imgUrlis always populated so the tileserver never receives an empty marker URL. -
Invasion fix is correct:
grunt_typemust beintto matchInvasionWebhook.GruntType int json:"grunt_type". The old string value caused Go'sjson.Unmarshalto silently skip the field, defaulting to 0 (unknown grunt). -
Field name corrections are accurate:
type(notquest_type),target(notquest_target),title(notquest_reward_type) — verified againstQuestWebhookstruct inprocessor/internal/webhook/types.go.
Test Coverage
- All 29 TestAlert backend tests pass
- All 721 backend tests pass
- All 509 frontend tests pass
- Existing tests validate API contract (type, target, template) — they don't assert webhook payload structure, which is acceptable since the payload format is a PoracleNG integration contract, not a unit-testable business rule
Risk Assessment
| Risk | Level | Notes |
|---|---|---|
| Breaking Change | None | Only affects test alert payloads, not real webhook processing |
| Data Integrity | None | No database changes |
| Performance | None | Same code path, slightly more object construction |
| Security | None | No auth/input changes |
| Rollback | Trivial | Single file revert |
Verdict: ✅ APPROVED
Clean, well-scoped fix that correctly aligns the test alert webhook payload with PoracleNG's expected Golbat format. The invasion grunt_type fix is a good bonus catch.
Summary
BuildQuestWebhookto use Golbat's webhook format with a properrewardsarray ([{type, info}]) and correct field names (type,target,title), so PoracleNG's enricher can resolveimgUrland generate static map tilesBuildInvasionWebhooksendinggrunt_typeas string instead of int, which caused silent JSON deserialization failure in PoracleNG (grunt type defaulted to 0)Test plan
dotnet buildsucceedsCloses #152