Skip to content

[BACK-3013] Log work system email notifications event history. #918

Open
lostlevels wants to merge 15 commits intomasterfrom
jimmy-email-existing-device-errs
Open

[BACK-3013] Log work system email notifications event history. #918
lostlevels wants to merge 15 commits intomasterfrom
jimmy-email-existing-device-errs

Conversation

@lostlevels
Copy link
Copy Markdown
Contributor

Adds a log of email notification related events. Used by related script to send, and avoid re-sending, to existing users w/ device issues / expired connection requests.

@lostlevels lostlevels requested a review from darinkrauss March 17, 2026 16:09
@lostlevels lostlevels force-pushed the jimmy-device-email-notifications branch from f7365e4 to 0c2418e Compare March 17, 2026 18:37
@darinkrauss
Copy link
Copy Markdown
Contributor

Did you run make generate. I noticed all of the mock file changes. This should reformat them.

Copy link
Copy Markdown
Contributor

@darinkrauss darinkrauss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass with a number of questions/comments.

Comment thread go.mod
Comment thread clinics/service.go Outdated
Comment thread notifications/logs/logs.go Outdated
Comment thread notifications/logs/logs.go Outdated
Comment thread notifications/history/history.go
Comment thread notifications/work/claims/processor.go Outdated
Comment thread notifications/work/connections/issues/processor.go Outdated
Comment thread notifications/work/claims/processor.go
Comment thread notifications/work/connections/issues/processor.go Outdated
Comment thread notifications/work/connections/requests/processor.go Outdated
@darinkrauss
Copy link
Copy Markdown
Contributor

BTW, it looks like the build failed because you need to run make generate to reformat the mock files.

Copy link
Copy Markdown
Contributor

@darinkrauss darinkrauss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A number of comments and a question or two. Does need some changes, mostly handling the error from Recorder.Create.

Comment thread notifications/history/history.go Outdated
Comment thread notifications/history/history.go Outdated
Comment thread notifications/history/mongo_history.go Outdated
Comment thread data/service/api/standard.go Outdated
Comment thread data/service/context/standard.go Outdated
Comment thread notifications/work/connections/issues/processor.go Outdated
Comment thread notifications/work/connections/requests/processor.go Outdated
Comment thread notifications/work/connections/requests/processor.go Outdated
Comment thread notifications/work/connections/requests/processor.go Outdated
Comment thread notifications/work/connections/requests/processor.go Outdated
@lostlevels lostlevels requested a review from darinkrauss April 3, 2026 09:58
darinkrauss
darinkrauss previously approved these changes Apr 6, 2026
Copy link
Copy Markdown
Contributor

@darinkrauss darinkrauss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll approve, but if the List API has the potential to return many documents (thousands+), then this PR needs changes to support a pagination parameter and a stable sort on the Mongo query.

Base automatically changed from jimmy-device-email-notifications to master April 6, 2026 21:06
@lostlevels lostlevels dismissed darinkrauss’s stale review April 6, 2026 21:06

The base branch was changed.

@lostlevels
Copy link
Copy Markdown
Contributor Author

I'll approve, but if the List API has the potential to return many documents (thousands+), then this PR needs changes to support a pagination parameter and a stable sort on the Mongo query.

Addressed.

darinkrauss
darinkrauss previously approved these changes Apr 7, 2026
Copy link
Copy Markdown
Contributor

@darinkrauss darinkrauss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Used for general history tracking and to filter existing errors/requests for
users for the initial error send.
@lostlevels
Copy link
Copy Markdown
Contributor Author

Force pushed to squash last 7 commits to make merging in master easier.

@lostlevels lostlevels requested a review from darinkrauss April 14, 2026 09:01
@lostlevels
Copy link
Copy Markdown
Contributor Author

@darinkrauss Latest commit always adds metadata to notification events history and forgot to add notification history event for device issues.

@lostlevels
Copy link
Copy Markdown
Contributor Author

Last commit uses the work item's metadata since that's serialized already w/ correct capitalization.

Copy link
Copy Markdown
Contributor

@darinkrauss darinkrauss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! One comment about serializing the error.

Email string `bson:"email,omitempty"`
GroupID string `bson:"groupId,omitempty"`
Metadata bson.M `bson:"metadata,omitempty"`
Error error `bson:"error,omitempty"`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you capture the error within an errors.Serializable it will capture the full internal error context and information. So:

	Error         errors.Serializable     `bson:"error,omitempty"`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants