Conversation
…te persistNotification to accept optional id
Reviewer's GuideExtends the Medplum notification backend to support configurable backend identifiers, optional predefined notification IDs that are passed through to created FHIR Communication resources (including one-off notifications), updates the factory to accept backend options, and adjusts tests and mocks (including template renderer mocks) to cover the new behavior and API surface. Sequence diagram for notification persistence with optional predefined IDsequenceDiagram
actor Caller
participant Backend as MedplumNotificationBackend
participant Medplum as MedplumClient
Caller->>Backend: persistNotification(notification)
alt notification has id
Backend->>Backend: Build Communication with id
else notification has no id
Backend->>Backend: Build Communication without id
end
Backend->>Medplum: create(Communication)
Medplum-->>Backend: Communication (with assigned or preserved id)
Backend-->>Caller: DatabaseNotification
Class diagram for updated MedplumNotificationBackend and factoryclassDiagram
class MedplumNotificationBackendOptions {
+string emailNotificationSubjectExtensionUrl
+string identifier
}
class BaseNotificationTypeConfig {
}
class MedplumClient {
}
class BaseNotificationBackend~Config~ {
}
class MedplumNotificationBackend~Config~ {
-BaseAttachmentManager attachmentManager
-BaseLogger logger
-string identifier
+MedplumNotificationBackend(medplum MedplumClient, options MedplumNotificationBackendOptions)
+getBackendIdentifier() string
+persistNotification(notification NotificationWithoutIdOrWithOptionalId~Config~) DatabaseNotification~Config~
+persistOneOffNotification(notification OneOffNotificationWithoutIdOrWithOptionalId~Config~) DatabaseOneOffNotification~Config~
}
class MedplumNotificationBackendFactory~Config~ {
+create(medplum MedplumClient, options MedplumNotificationBackendOptions) MedplumNotificationBackend~Config~
}
class NotificationWithoutIdOrWithOptionalId~Config~ {
+Config.NotificationIdType id
+string title
+Date sendAfter
}
class OneOffNotificationWithoutIdOrWithOptionalId~Config~ {
+Config.NotificationIdType id
+string title
+Date sendAfter
}
class DatabaseNotification~Config~ {
}
class DatabaseOneOffNotification~Config~ {
}
class Communication {
+string id
+string resourceType
+string status
+string sent
+string topic
}
MedplumNotificationBackend~Config~ ..|> BaseNotificationBackend~Config~
MedplumNotificationBackend~Config~ o-- MedplumClient
MedplumNotificationBackend~Config~ o-- MedplumNotificationBackendOptions
MedplumNotificationBackendFactory~Config~ o-- MedplumNotificationBackend~Config~
MedplumNotificationBackend~Config~ --> DatabaseNotification~Config~
MedplumNotificationBackend~Config~ --> DatabaseOneOffNotification~Config~
MedplumNotificationBackend~Config~ --> Communication
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The constructor now directly assigns the passed
options, which meansemailNotificationSubjectExtensionUrlwill beundefinedif the caller supplies onlyidentifier; consider shallow-merging with the defaults so you don’t regress the default URL behavior. - In both
persistNotificationandpersistOneOffNotification,notification.idis cast tostringwhen constructing theCommunication; ifConfig['NotificationIdType']can be non-string, consider tightening the generic constraint or normalizing the value before passing it to Medplum.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The constructor now directly assigns the passed `options`, which means `emailNotificationSubjectExtensionUrl` will be `undefined` if the caller supplies only `identifier`; consider shallow-merging with the defaults so you don’t regress the default URL behavior.
- In both `persistNotification` and `persistOneOffNotification`, `notification.id` is cast to `string` when constructing the `Communication`; if `Config['NotificationIdType']` can be non-string, consider tightening the generic constraint or normalizing the value before passing it to Medplum.
## Individual Comments
### Comment 1
<location path="src/medplum-backend.ts" line_range="63-67" />
<code_context>
private logger?: BaseLogger;
+ private identifier: string;
constructor(private medplum: MedplumClient, private options: MedplumNotificationBackendOptions = {
emailNotificationSubjectExtensionUrl: 'http://vintasend.com/fhir/StructureDefinition/email-notification-subject',
- }) {}
+ identifier: 'default-medplum',
+ }) {
+ this.identifier = options.identifier || 'default-medplum';
+ }
+
</code_context>
<issue_to_address>
**suggestion:** Avoid duplicating the default value for `identifier` in both the parameter default and the constructor body.
`'default-medplum'` is defined both in the `options` default and again in `this.identifier = options.identifier || 'default-medplum';`, which risks them diverging if the default changes. Derive `this.identifier` from `this.options.identifier` with a single source of truth for the default (e.g., a shared constant or by defaulting in only one place).
Suggested implementation:
```typescript
const DEFAULT_MEDPLUM_IDENTIFIER = 'default-medplum';
type MedplumNotificationBackendOptions = {
emailNotificationSubjectExtensionUrl?: string;
identifier?: string;
};
```
```typescript
private attachmentManager?: BaseAttachmentManager;
private logger?: BaseLogger;
private identifier: string;
constructor(
private medplum: MedplumClient,
private options: MedplumNotificationBackendOptions = {
emailNotificationSubjectExtensionUrl:
'http://vintasend.com/fhir/StructureDefinition/email-notification-subject',
identifier: DEFAULT_MEDPLUM_IDENTIFIER,
}
) {
this.identifier = this.options.identifier ?? DEFAULT_MEDPLUM_IDENTIFIER;
}
```
</issue_to_address>
### Comment 2
<location path="src/medplum-backend.ts" line_range="316-317" />
<code_context>
}
- async persistNotification(notification: NotificationInput<Config>): Promise<DatabaseNotification<Config>> {
+ async persistNotification(
+ notification: Omit<Notification<Config>, 'id'> & { id?: Config['NotificationIdType'] },
+ ): Promise<DatabaseNotification<Config>> {
const notificationWithOptionalGitCommitSha = notification as NotificationInput<Config> & {
</code_context>
<issue_to_address>
**issue (bug_risk):** The new `persistNotification` input type may drift from `NotificationInput` but is still cast to `NotificationInput` internally.
The parameter is now `Omit<Notification<Config>, 'id'> & { id?: ... }`, but you still assert it to `NotificationInput<Config>` to access `gitCommitSha`. If `Notification<Config>` and `NotificationInput<Config>` ever differ, this assertion can mask type errors and cause runtime bugs. Consider either using `NotificationInput<Config> & { id?: ... }` as the parameter type, or extending `Notification<Config>` to include `gitCommitSha` so the assertion isn’t needed.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
63
to
+67
| constructor(private medplum: MedplumClient, private options: MedplumNotificationBackendOptions = { | ||
| emailNotificationSubjectExtensionUrl: 'http://vintasend.com/fhir/StructureDefinition/email-notification-subject', | ||
| }) {} | ||
| identifier: 'default-medplum', | ||
| }) { | ||
| this.identifier = options.identifier || 'default-medplum'; |
There was a problem hiding this comment.
suggestion: Avoid duplicating the default value for identifier in both the parameter default and the constructor body.
'default-medplum' is defined both in the options default and again in this.identifier = options.identifier || 'default-medplum';, which risks them diverging if the default changes. Derive this.identifier from this.options.identifier with a single source of truth for the default (e.g., a shared constant or by defaulting in only one place).
Suggested implementation:
const DEFAULT_MEDPLUM_IDENTIFIER = 'default-medplum';
type MedplumNotificationBackendOptions = {
emailNotificationSubjectExtensionUrl?: string;
identifier?: string;
}; private attachmentManager?: BaseAttachmentManager;
private logger?: BaseLogger;
private identifier: string;
constructor(
private medplum: MedplumClient,
private options: MedplumNotificationBackendOptions = {
emailNotificationSubjectExtensionUrl:
'http://vintasend.com/fhir/StructureDefinition/email-notification-subject',
identifier: DEFAULT_MEDPLUM_IDENTIFIER,
}
) {
this.identifier = this.options.identifier ?? DEFAULT_MEDPLUM_IDENTIFIER;
}
Comment on lines
+316
to
+317
| async persistNotification( | ||
| notification: Omit<Notification<Config>, 'id'> & { id?: Config['NotificationIdType'] }, |
There was a problem hiding this comment.
issue (bug_risk): The new persistNotification input type may drift from NotificationInput but is still cast to NotificationInput internally.
The parameter is now Omit<Notification<Config>, 'id'> & { id?: ... }, but you still assert it to NotificationInput<Config> to access gitCommitSha. If Notification<Config> and NotificationInput<Config> ever differ, this assertion can mask type errors and cause runtime bugs. Consider either using NotificationInput<Config> & { id?: ... } as the parameter type, or extending Notification<Config> to include gitCommitSha so the assertion isn’t needed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Support configurable Medplum backend identifiers and allow persisting notifications with predefined IDs.
New Features:
Enhancements:
Tests: