Skip to content

Commit ead40de

Browse files
Matovidloclaude
andcommitted
fix(notifications): address PR #2528 review comments
- Fix manifest ID not updated after notification create/update: Set notificationState.NotificationManifest.ID = created.ID in the WithOnSuccess callback so the manifest correctly persists the API-assigned subscription ID after push. - Write auto-filters back to local files after push: Update notification.Filters = created.Filters in callback so the API-assigned filters (including auto-populated branch.id, job.component.id, job.configuration.id) are written back to config.json after push via a local save in remote.UnitOfWork.Invoke(). - Remove unnecessary custom JSON marshaling from ConfigManifestWithRows: Add MarshalJSON to NotificationManifest that outputs only {id, path}, removing the need for NotificationManifestRef wrapper struct and the custom ConfigManifestWithRows.MarshalJSON method. - Remove docs/notifications.md as requested by reviewer. - Remove notification-specific subsections from CLI_OBJECT_LIFECYCLE.md (Auto-Filter Behavior and Delete-Then-Create for Updates sections). - Fix E2E test manifests to use %A wildcard for API-assigned IDs. - Update E2E test out config.json files to include auto-filters. - Add new E2E test notifications-create-with-filters to verify that user-specified filters are combined with auto-populated filters. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 0b30b3a commit ead40de

File tree

37 files changed

+318
-429
lines changed

37 files changed

+318
-429
lines changed

docs/CLI_OBJECT_LIFECYCLE.md

Lines changed: 0 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -350,39 +350,6 @@ u.changes.AddCreated(notificationState)
350350
// IDs in manifest.json are updated for new notifications via manifest.Save()
351351
```
352352

353-
### Auto-Filter Behavior
354-
355-
Users write only `event` + `recipient` (and optionally custom `filters`) in `config.json`. During push, three standard filters are **auto-populated** by `createNotificationRequest()`:
356-
357-
- `branch.id` — from the notification's `BranchID`
358-
- `job.component.id` — from the notification's `ComponentID`
359-
- `job.configuration.id` — from the notification's `ConfigID`
360-
361-
These filters are required by the API to scope the notification to a specific config. They are appended before any user-defined filters.
362-
363-
The API returns all filters (auto + user) in the response, so after a `pull`, `config.json` contains all three auto-filters plus any user-defined ones.
364-
365-
**Consequence:** On a repeated `push` without `pull`:
366-
- Local `config.json` has no auto-filters (they were never written locally)
367-
- Remote notification has all three auto-filters
368-
- Diff engine shows `changed: filters`
369-
- This is **expected behavior**, not a bug
370-
371-
### Delete-Then-Create for Updates
372-
373-
The notifications API has no update endpoint. Changes are implemented as:
374-
1. Delete the old notification subscription
375-
2. Create a new one with the changed values
376-
377-
This is implemented via `WithOnSuccess` callback chaining in `UnitOfWork.SaveObject`:
378-
```go
379-
// Pseudocode in remote/manager.go
380-
deleteReq.WithOnSuccess(func(...) {
381-
// On successful delete, create the new one
382-
return createReq.Send(ctx)
383-
})
384-
```
385-
386353
---
387354

388355
## Common Pitfalls

internal/pkg/model/manifest.go

Lines changed: 0 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,9 @@
11
package model
22

33
import (
4-
"encoding/json"
54
"fmt"
65

76
"github.com/keboola/go-utils/pkg/orderedmap"
8-
"github.com/keboola/keboola-sdk-go/v2/pkg/keboola"
97

108
"github.com/keboola/keboola-as-code/internal/pkg/filesystem"
119
"github.com/keboola/keboola-as-code/internal/pkg/utils/errors"
@@ -91,44 +89,11 @@ type ConfigRowManifest struct {
9189
Relations Relations `json:"relations,omitempty" validate:"dive"` // relations with other objects, for example variables values definition
9290
}
9391

94-
type NotificationManifestRef struct {
95-
ID keboola.NotificationSubscriptionID `json:"id"`
96-
Path string `json:"path,omitempty"`
97-
}
98-
9992
type ConfigManifestWithRows struct {
10093
ConfigManifest
10194
Rows []*ConfigRowManifest `json:"rows"`
10295
}
10396

104-
// MarshalJSON implements custom JSON marshaling to simplify notifications to just IDs.
105-
func (c *ConfigManifestWithRows) MarshalJSON() ([]byte, error) {
106-
// Convert notifications to simplified refs
107-
notificationRefs := make([]*NotificationManifestRef, 0, len(c.Notifications))
108-
for _, n := range c.Notifications {
109-
if n != nil {
110-
notificationRefs = append(notificationRefs, &NotificationManifestRef{ID: n.ID, Path: n.GetRelativePath()})
111-
}
112-
}
113-
114-
// Create a copy with simplified notifications
115-
return json.Marshal(&struct {
116-
ConfigKey
117-
Paths
118-
Relations Relations `json:"relations,omitempty"`
119-
Metadata *orderedmap.OrderedMap `json:"metadata,omitempty"`
120-
Rows []*ConfigRowManifest `json:"rows"`
121-
Notifications []*NotificationManifestRef `json:"notifications,omitempty"`
122-
}{
123-
ConfigKey: c.ConfigKey,
124-
Paths: c.Paths,
125-
Relations: c.Relations,
126-
Metadata: c.Metadata,
127-
Rows: c.Rows,
128-
Notifications: notificationRefs,
129-
})
130-
}
131-
13297
func (p *Paths) ClearRelatedPaths() {
13398
p.RelatedPaths = make([]string, 0)
13499
}

internal/pkg/model/notificationmanifest.go

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,10 @@
11
package model
22

33
import (
4+
"encoding/json"
45
"fmt"
6+
7+
"github.com/keboola/keboola-sdk-go/v2/pkg/keboola"
58
)
69

710
// NotificationManifest represents a notification manifest record.
@@ -12,6 +15,18 @@ type NotificationManifest struct {
1215
Relations Relations `json:"relations,omitempty" validate:"dive"`
1316
}
1417

18+
// MarshalJSON serializes only the ID and path, omitting the embedded NotificationKey fields
19+
// (branchId, componentId, configId) which would otherwise appear in the manifest JSON.
20+
func (n *NotificationManifest) MarshalJSON() ([]byte, error) {
21+
return json.Marshal(&struct {
22+
ID keboola.NotificationSubscriptionID `json:"id"`
23+
Path string `json:"path,omitempty"`
24+
}{
25+
ID: n.ID,
26+
Path: n.GetRelativePath(),
27+
})
28+
}
29+
1530
// NewEmptyObject creates an empty Notification object with the key from this manifest.
1631
func (n NotificationManifest) NewEmptyObject() Object {
1732
return &Notification{NotificationKey: n.NotificationKey}

internal/pkg/service/cli/CLI_CONTEXT.md

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ The CLI supports managing notification subscriptions for configurations. Notific
1818
- `push`: Create/delete notifications (no update API - delete + create)
1919
- Manifest tracks subscription IDs for sync
2020

21-
See [docs/notifications.md](docs/notifications.md) for detailed documentation.
2221

2322
## Overview
2423

0 commit comments

Comments
 (0)