-
-
Notifications
You must be signed in to change notification settings - Fork 359
FIX: [xalign] include created order id in thread reply #2370
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
Conversation
6d3a0b4 to
0fa33b3
Compare
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.
Pull request overview
This PR enhances the xalign strategy's Slack integration by including created order IDs in thread replies when users interact with already-processed orders. This provides better visibility into the order submission results after the interactive buttons have been processed.
Key Changes:
- Added
submittedOrderRegistrysync.Map to persist submitted order information - Implemented automatic cleanup of orders older than 2 hours from the registry
- Enhanced Slack interaction callback to display created order details when users click buttons on already-processed orders
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/strategy/xalign/slack.go
Outdated
| now := time.Now() | ||
| submittedOrderRegistry.Range(func(key, value interface{}) bool { | ||
| if createdOrder, ok := value.(*types.Order); ok { | ||
| if now.Sub(createdOrder.CreationTime.Time()) > 2*time.Hour { |
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.
can use time.Since to replace now.
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.
I refactor it and pass now as a parameter.
The reason for that is to make mocking now possible so I can write test on the case where ttl is due.
pkg/strategy/xalign/slack.go
Outdated
| blocks = append(blocks, interact.InteractionMessageUpdate{ | ||
| Blocks: []slack.Block{buildTextBlock( | ||
| fmt.Sprintf( | ||
| "Created Order: %s", createdOrder.String(), |
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.
maybe we can call createdOrder.SlackBlocks()?
we need to implement order.SlackBlocks()
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.
done.
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.
I use .SlackAttachment() instead.
167936c to
52f11b1
Compare
52f11b1 to
e5cdc84
Compare
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.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
pkg/strategy/xalign/slack.go:294
- The variable 'io' is assigned from 'value' but this is redundant since 'itOrder' was already type-asserted from 'value' at line 251. The check at line 252 already ensures that 'value' is of type '*InteractiveSubmitOrder' and assigns it to 'itOrder', so this second type assertion and assignment is unnecessary. You can directly use 'itOrder' instead of 'io' in the rest of the function.
return []interact.InteractionMessageUpdate{
{
Blocks: nil,
Attachments: nil,
PostInThread: false,
},
}, fmt.Errorf("invalid type detected for interactive order (%s): %+v", actionValue, value)
}
blocks := removeBlockByID(oriMessage.Blocks.BlockSet, interactiveButtonsBlockID)
switch actionID {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
244cf3b to
177e773
Compare
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.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
012ab33 to
3ff5e72
Compare
3ff5e72 to
7f2c8d3
Compare
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.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| submittedOrderRegistry.Store(itOrder.id, order) | ||
|
|
||
| if onSubmittedOrderCallback != nil { | ||
| onSubmittedOrderCallback(session, &itOrder.submitOrder, order, err) |
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.
Is it possible that order is nil here?
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.
Yes.
It should be handled by the callback.
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.
Okay. Just want to make sure the type assertion at line 274 won't panic.
7f2c8d3 to
a85efb5
Compare
a85efb5 to
d6463c1
Compare
No description provided.