Conversation
| Items []Item `xml:"item"` | ||
| } | ||
|
|
||
| type Item struct { |
There was a problem hiding this comment.
Could also add other fields here in a follow up (title, etc.).
d22c661 to
61d9fe9
Compare
After this change, openapi stopped generating the WebhookConfigTypeWebhook constant.
Hence the updates in the go files.
The API changes are:
1. Add /v1/subscriptions/{subscription_id}/rss endpoint for public RSS feeds
This endpoint will accept a subscription ID and fetch the 'n' most recent diffs/updates
2. Update CreateNotificationChannelRequest and UpdateNotificationChannelRequest to support RSSConfig
RSS config is an empty object and is pretty much just a placeholder to make it a first class citizen
in the codebase like Webhook and email
This is a first go at implementing this API and is *not* the final form of it. A few things that still need to be done: - With this change, JSON is just being inserted into the XML. This will need to be more properly formatted. - Caching of the responses from the RSS API. - Hardcoding the # of events/diffs returned to the API to 20 for now. Open to any number on this.
| func (c *Client) ListSavedSearchNotificationEvents(ctx context.Context, | ||
| savedSearchID string, snapshotType string, limit int) ([]SavedSearchNotificationEvent, error) { | ||
| stmt := spanner.Statement{ | ||
| SQL: `SELECT * FROM SavedSearchNotificationEvents |
There was a problem hiding this comment.
Once you make the OpenAPI changes, you should get an opaque token like the rest of the List operations which will allow you to use the mapper pattern and it can manage this query for you.
There was a problem hiding this comment.
To make the RSS feed as rich as the email and webhook notifications, we should move away from dumping raw JSON. We should adopt a scalable approach similar to the email worker, using templates to construct the HTML for the RSS description.
Template Parsing at Startup:
To be efficient, we should NOT parse the template on every request. It is best practice to parse the template once at startup or initialization time and reuse the parsed template object.
Using Enums:
Instead of using magic strings in the switch case (like "FEATURE_ADDED"), we should use the typed enums. Based on the current codebase, these are defined in lib/workertypes/types.go (e.g., workertypes.SummaryHighlightTypeAdded). The author should use these or the equivalent if they are exposed via a backend-specific package in this PR.
Here is an abbreviated idea of how we can structure a scalable renderer with initialization and enums:
Code Snippet:
const rssItemTemplate = `
<div>
<p>{{.SummaryText}}</p>
{{if .Added}}
<h4>Features Added</h4>
<ul>
{{range .Added}}<li>{{.}}</li>{{end}}
</ul>
{{end}}
{{if .Removed}}
<h4>Features Removed</h4>
<ul>
{{range .Removed}}<li>{{.}}</li>{{end}}
</ul>
{{end}}
{{if .Truncated}}
<p><em>Note: This summary has been truncated. View full details on the site.</em></p>
{{end}}
</div>
`
type RSSItemData struct {
SummaryText string
Added []string
Removed []string
Truncated bool
}
type RSSRenderer struct {
tmpl *template.Template
}
// NewRSSRenderer initializes the renderer and parses the template at startup.
func NewRSSRenderer() (*RSSRenderer, error) {
tmpl, err := template.New("rss_item").Parse(rssItemTemplate)
if err != nil {
return nil, fmt.Errorf("failed to parse rss template: %w", err)
}
return &RSSRenderer{tmpl: tmpl}, nil
}
func (r *RSSRenderer) RenderRSSDescription(summary workertypes.EventSummary) (string, error) {
data := RSSItemData{
SummaryText: summary.Text,
Truncated: summary.Truncated,
}
// Map highlights to categories using Enums
for _, h := range summary.Highlights {
switch h.Type {
case workertypes.SummaryHighlightTypeAdded:
data.Added = append(data.Added, h.FeatureName)
case workertypes.SummaryHighlightTypeRemoved:
data.Removed = append(data.Removed, h.FeatureName)
// etc for other cases
}
}
var buf bytes.Buffer
if err := r.tmpl.Execute(&buf, data); err != nil {
return "", err
}
return buf.String(), nil
}There was a problem hiding this comment.
To keep the main handler file (get_saved_search_rss.go) small and focused on HTTP concerns, we should move all the rendering logic (like the template and functions proposed in Comment 1) into a new file in the same directory, for example, rss_renderer.go.
You can initialize the RSSRenderer when creating the server or handler and pass it in.
Code Snippet (Handler usage):
// In get_saved_search_rss.go
// Assuming renderer is stored in the server or handler struct
richHTML, err := h.rssRenderer.RenderRSSDescription(summary)
if err != nil {
// Handle error (see my next comment)
}
item.Description = richHTML // Go will escape this automatically in xml.MarshalThere was a problem hiding this comment.
What should we do if we encounter a corrupt item (e.g., failed to unmarshal JSON or failed to render)?
Instead of failing the whole request or silently skipping it, I suggest inserting a valid RSS item that displays an error message. This informs the user that something went wrong with a specific update, and we can provide an event ID for support.
Code Snippet:
for _, e := range events {
var summary workertypes.EventSummary
if err := json.Unmarshal(e.Summary, &summary); err != nil {
slog.ErrorContext(ctx, "failed to unmarshal summary", "event_id", e.EventID, "error", err)
errorHTML := fmt.Sprintf("<p>Could not load details for this update. Please contact support with ID: %s</p>", e.EventID)
items = append(items, Item{
Title: "Error loading update",
Description: errorHTML,
PubDate: e.CreatedAt.Format(http.TimeFormat),
})
continue
}
// Proceed with normal rendering...
}
jcscottiii
left a comment
There was a problem hiding this comment.
LGTM with use of getPageSizeOrDefault
| } | ||
|
|
||
| snapshotType := string(sub.Frequency) | ||
| events, err := s.wptMetricsStorer.ListSavedSearchNotificationEvents(ctx, search.Id, snapshotType, 20) |
There was a problem hiding this comment.
Use the getPageSizeOrDefault helper and pass in the page size to it.
This is a first go at implementing this API and is not the final form of it. A few things that still need to be done: