-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(webhook): Add is_delivered filter to list initial attempts endpoint #7344
base: return-total-events-count
Are you sure you want to change the base?
Changes from all commits
041cf3a
23e06a8
579dd65
d5ba8ff
13a666a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -28,6 +28,9 @@ pub struct EventListConstraints { | |||||
/// Filter all events associated with the specified business profile ID. | ||||||
#[schema(value_type = Option<String>)] | ||||||
pub profile_id: Option<common_utils::id_type::ProfileId>, | ||||||
|
||||||
// Filter all events by overall_delivery_status. | ||||||
pub is_delivered: Option<bool>, | ||||||
} | ||||||
|
||||||
#[derive(Debug)] | ||||||
|
@@ -37,6 +40,7 @@ pub enum EventListConstraintsInternal { | |||||
created_before: Option<PrimitiveDateTime>, | ||||||
limit: Option<i64>, | ||||||
offset: Option<i64>, | ||||||
is_delivered: Option<bool>, | ||||||
}, | ||||||
ObjectIdFilter { | ||||||
object_id: String, | ||||||
|
@@ -68,7 +72,7 @@ pub struct EventListItemResponse { | |||||
/// Specifies the class of event (the type of object: Payment, Refund, etc.) | ||||||
pub event_class: EventClass, | ||||||
|
||||||
/// Indicates whether the webhook delivery attempt was successful. | ||||||
/// Indicates whether the webhook was ultimately delivery. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit:
Suggested change
|
||||||
pub is_delivery_successful: bool, | ||||||
|
||||||
/// The identifier for the initial delivery attempt. This will be the same as `event_id` for | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -133,6 +133,7 @@ pub(crate) async fn create_event_and_trigger_outgoing_webhook( | |
response: None, | ||
delivery_attempt: Some(delivery_attempt), | ||
metadata: Some(event_metadata), | ||
is_overall_delivery_successful: false, | ||
}; | ||
|
||
let event_insert_result = state | ||
|
@@ -822,7 +823,7 @@ async fn update_event_in_storage( | |
.attach_printable("Failed to encrypt outgoing webhook response content")?, | ||
), | ||
}; | ||
state | ||
let current_event = state | ||
.store | ||
.update_event_by_merchant_id_event_id( | ||
key_manager_state, | ||
|
@@ -832,7 +833,36 @@ async fn update_event_in_storage( | |
&merchant_key_store, | ||
) | ||
.await | ||
.change_context(errors::WebhooksFlowError::WebhookEventUpdationFailed) | ||
.change_context(errors::WebhooksFlowError::WebhookEventUpdationFailed)?; | ||
|
||
let parent_update = domain::EventUpdate::ParentUpdate { | ||
is_overall_delivery_successful: status_code.is_success(), | ||
}; | ||
Comment on lines
+838
to
+840
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Once we've extracted it to a separate function, let's name this enum variant to indicate which field is being updated, rather than which event is being updated: something along the lines of |
||
|
||
let parent_event_id = current_event.initial_attempt_id.clone(); | ||
let delivery_attempt = current_event.delivery_attempt; | ||
|
||
if let Some(( | ||
parent_event_id, | ||
enums::WebhookDeliveryAttempt::InitialAttempt | ||
| enums::WebhookDeliveryAttempt::AutomaticRetry, | ||
)) = parent_event_id.zip(delivery_attempt) | ||
{ | ||
state | ||
.store | ||
.update_event_by_merchant_id_event_id( | ||
key_manager_state, | ||
merchant_id, | ||
parent_event_id.as_str(), | ||
parent_update, | ||
&merchant_key_store, | ||
) | ||
.await | ||
.change_context(errors::WebhooksFlowError::WebhookEventUpdationFailed) | ||
.attach_printable("Failed to update parent event")?; | ||
} | ||
Comment on lines
+838
to
+863
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we extract updating of the initial attempt to a different function, and call it in Ideally, we'd want to keep a function to have only one responsibility... |
||
|
||
Ok(current_event) | ||
} | ||
|
||
fn increment_webhook_outgoing_received_count(merchant_id: &common_utils::id_type::MerchantId) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make this a documentation comment instead.
And update the documentation comment to reflect the current field name, in case you think we actually need to reference the field name here.