Conversation
CayoPOliveira
left a comment
CayoPOliveira
left a comment
There was a problem hiding this comment.
Reviewed 9 of 9 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @CayoPOliveira)
app/javascript/dashboard/routes/dashboard/settings/integrations/Webhooks/WebhookForm.vue line 83 at r1 (raw file):
return this.$t('INTEGRATION_SETTINGS.WEBHOOK.FORM.NAME.PLACEHOLDER'); }, webhookInboxInputPlaceholder() {
NIT I will remove this
app/javascript/dashboard/i18n/locale/en/integrations.json line 198 at r1 (raw file):
"LOADING": "Fetching dashboard apps...", "TABLE_HEADER": { "NAME": "Name",
Linter
I need to rollback?
app/javascript/dashboard/i18n/locale/en/integrations.json line 431 at r1 (raw file):
"SUCCESS_MESSAGE": "The document has been successfully deleted", "ERROR_MESSAGE": "There was an error deleting the document, please try again." },
Linter
app/javascript/dashboard/i18n/locale/en/integrations.json line 481 at r1 (raw file):
"PLACEHOLDER": "Enter the answer here", "ERROR": "Please provide a valid answer." },
Linter
app/javascript/dashboard/i18n/locale/en/integrations.json line 536 at r1 (raw file):
} } }
Linter
gabrieljablonski
added
enhancement
gabrieljablonski
left a comment
gabrieljablonski
left a comment
There was a problem hiding this comment.
-(review needed) +(questions/changes requested)
Reviewed 5 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @CayoPOliveira)
app/javascript/dashboard/i18n/locale/en/integrations.json line 198 at r1 (raw file):
Previously, CayoPOliveira (Cayo P. R. Oliveira) wrote…
Linter
I need to rollback?
Please fix your VSCode config so this doesn't happen anymore.
spec/controllers/api/v1/accounts/webhook_controller_spec.rb line 131 at r2 (raw file):
expect(response).to have_http_status(:unprocessable_entity) expect(response.parsed_body['message']).to eql 'Inbox cannot be updated' end
Change to "ignores trying to update inbox_id"
app/listeners/webhook_listener.rb line 92 at r2 (raw file):
account.webhooks.account_type.each do |webhook| next unless webhook.subscriptions.include?(payload[:event]) next if payload[:inbox].present? && (webhook.inbox_id.present? && webhook.inbox_id != payload[:inbox][:id])
NIT
Suggestion:
next if payload[:inbox].present? && webhook.inbox_id.present? && webhook.inbox_id != payload[:inbox][:id]app/controllers/api/v1/accounts/webhooks_controller.rb line 18 at r2 (raw file):
render json: { message: 'Inbox cannot be updated' }, status: :unprocessable_entity return end
This can be removed. See suggestion below.
app/controllers/api/v1/accounts/webhooks_controller.rb line 32 at r2 (raw file):
def webhook_params params.require(:webhook).permit(:inbox_id, :name, :url, subscriptions: []) end
This is a better way to allow/disallow updating specific fields.
Let's take the opportunity to also disallow changing the URL.
Suggestion:
def create_webhook_params
params.require(:webhook).permit(:inbox_id, :name, :url, subscriptions: [])
end
def update_webhook_params
params.require(:webhook).permit(:name, subscriptions: [])
endapp/javascript/dashboard/routes/dashboard/settings/integrations/Webhooks/WebhookForm.vue line 65 at r2 (raw file):
url: this.value.url || '', name: this.value.name || '', inbox_id: this.value.inbox_id || '',
NIT
Suggestion:
inboxId: this.value.inbox_id || '',app/javascript/dashboard/routes/dashboard/settings/integrations/Webhooks/WebhookForm.vue line 89 at r2 (raw file):
url: this.url, name: this.name, inbox_id: this.inbox_id,
Try updating this as well. If it doesn't work, can be kept as snake case.
Suggestion:
inboxId: this.inbox_id,app/javascript/dashboard/routes/dashboard/settings/integrations/Webhooks/WebhookForm.vue line 113 at r2 (raw file):
:placeholder="webhookURLInputPlaceholder" @input="v$.url.$touch" />
Disable as well when editing.
app/javascript/dashboard/routes/dashboard/settings/integrations/Webhooks/WebhookRow.vue line 62 at r2 (raw file):
olive.olive9, cyan.cyan9, ];
Remove colors.
app/javascript/dashboard/routes/dashboard/settings/integrations/Webhooks/WebhookRow.vue line 89 at r2 (raw file):
<div v-if="webhook.inbox" class="flex gap-2 font-medium break-words text-slate-500 dark:text-slate-400"
Add mt-1
app/javascript/dashboard/routes/dashboard/settings/integrations/Webhooks/WebhookRow.vue line 97 at r2 (raw file):
color: COLORS[webhook.inbox.id % COLORS.length], }" />
Is it possible to use an icon instead? Like is done on the sidebar.
…e styling in WebhookForm
CayoPOliveira
left a comment
CayoPOliveira
left a comment
There was a problem hiding this comment.
-(questions/changes requested) +(review needed)
Reviewed 8 of 8 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @gabrieljablonski)
app/controllers/api/v1/accounts/webhooks_controller.rb line 18 at r2 (raw file):
Previously, gabrieljablonski (Gabriel Jablonski) wrote…
This can be removed. See suggestion below.
Done.
app/controllers/api/v1/accounts/webhooks_controller.rb line 32 at r2 (raw file):
Previously, gabrieljablonski (Gabriel Jablonski) wrote…
This is a better way to allow/disallow updating specific fields.
Let's take the opportunity to also disallow changing the URL.
Done.
app/javascript/dashboard/routes/dashboard/settings/integrations/Webhooks/WebhookForm.vue line 89 at r2 (raw file):
Previously, gabrieljablonski (Gabriel Jablonski) wrote…
Try updating this as well. If it doesn't work, can be kept as snake case.
It doesn't work here
app/javascript/dashboard/routes/dashboard/settings/integrations/Webhooks/WebhookForm.vue line 113 at r2 (raw file):
Previously, gabrieljablonski (Gabriel Jablonski) wrote…
Disable as well when editing.
Done.
app/javascript/dashboard/routes/dashboard/settings/integrations/Webhooks/WebhookRow.vue line 62 at r2 (raw file):
Previously, gabrieljablonski (Gabriel Jablonski) wrote…
Remove colors.
Done.
app/javascript/dashboard/routes/dashboard/settings/integrations/Webhooks/WebhookRow.vue line 89 at r2 (raw file):
Previously, gabrieljablonski (Gabriel Jablonski) wrote…
Add
mt-1
Done.
app/javascript/dashboard/routes/dashboard/settings/integrations/Webhooks/WebhookRow.vue line 97 at r2 (raw file):
Previously, gabrieljablonski (Gabriel Jablonski) wrote…
Is it possible to use an icon instead? Like is done on the sidebar.
I used the InboxName from conversations page
spec/controllers/api/v1/accounts/webhook_controller_spec.rb line 131 at r2 (raw file):
Previously, gabrieljablonski (Gabriel Jablonski) wrote…
Change to "ignores trying to update inbox_id"
Changed to "ignores trying to update inbox_id and url"
gabrieljablonski
left a comment
gabrieljablonski
left a comment
There was a problem hiding this comment.
-(review needed) +(questions/changes requested)
Reviewed 1 of 9 files at r1, 7 of 8 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @CayoPOliveira)
spec/controllers/api/v1/accounts/webhook_controller_spec.rb line 131 at r2 (raw file):
Previously, CayoPOliveira (Cayo P. R. Oliveira) wrote…
Changed to "ignores trying to update inbox_id and url"
Looks good, but use .to eql ... with the original values, instead of .not_to eql ....
a discussion (no related file):
Dropdown UX can be improved. See example for selecting agent on conversation.
- We need a "All inboxes" explicit option (FE-only, still send
nullto BE). - Close the dropdown after selecting an option
PS.: On the video above, you can see an option can be de-selected by clicking it when it is already selected. I don't think we need that.
spec/listeners/webhook_listener_spec.rb line 97 at r4 (raw file):
listener.message_created(message_created_event) end end
Try applying this patch instead of placing these under #message_created, since they're supposed to test "generic" behavior.
Note the changes I made to the spec descriptions, and the (very slight) newline changes to make the Arrange-Act-Assert pattern clearer (unfortunately, original specs in this file don't seem to follow that pattern).
app/javascript/dashboard/routes/dashboard/settings/integrations/Webhooks/WebhookRow.vue line 40 at r4 (raw file):
<td class="py-4 ltr:pr-4 rtl:pl-4"> <div v-if="webhook.inbox"> <InboxName :inbox="webhook.inbox" class="flex break-words !mx-0" />
Do we need all of these classes?
Suggestion:
<InboxName :inbox="webhook.inbox" class="!mx-0" />app/javascript/dashboard/routes/dashboard/settings/integrations/Webhooks/WebhookRow.vue line 41 at r4 (raw file):
<div v-if="webhook.inbox"> <InboxName :inbox="webhook.inbox" class="flex break-words !mx-0" /> </div>
This div seems pointless
Suggestion:
<InboxName v-if="webhook.inbox" :inbox="webhook.inbox" class="flex break-words !mx-0" />app/javascript/dashboard/routes/dashboard/settings/integrations/Webhooks/WebhookForm.vue line 68 at r4 (raw file):
subscriptions: this.value.subscriptions || [], supportedWebhookEvents: SUPPORTED_WEBHOOK_EVENTS, selectedInbox: this.value.inbox || {},
Can we use null instead? From what I can tell, nothing should change, but makes more sense instead of {}.
Suggestion:
selectedInbox: this.value.inbox || null,app/javascript/dashboard/routes/dashboard/settings/integrations/Webhooks/WebhookForm.vue line 128 at r4 (raw file):
/> </label> <div class="flex flex-col w-full">
Is w-full not enough?
Suggestion:
<div class="w-full">app/javascript/dashboard/routes/dashboard/settings/integrations/Webhooks/WebhookForm.vue line 148 at r4 (raw file):
" :disabled="isEditing" button-variant="secondary"
secondary variant actually does nothing, other than overriding the hollow variant.
Try applying this patch for the dark variant instead:
Still not ideal (couldn't manage to update the border to match the other inputs), but looks better this way.
Ideally we should also match the "disabled" styling better, but not worth wasting more time on this right now. Creating the dark variant should be good enough.
app/javascript/dashboard/routes/dashboard/settings/integrations/Webhooks/WebhookForm.vue line 154 at r4 (raw file):
</label> </div> <label :class="{ error: v$.url.$error }" class="mb-2">
WDYT of changing the order of inputs to URL -> Inbox -> Name?
Feels a bit weird to have the fields be disabled -> enabled -> disabled when editing.
app/listeners/webhook_listener.rb line 6 at r4 (raw file):
changed_attributes = extract_changed_attributes(event) inbox = conversation.inbox
Do we really need this newline? Breaks the styling from other methods below.
WalkthroughThis pull request refines webhook parameter handling in the API controller by differentiating between create and update actions. It introduces new UI elements and component properties for inbox selection in the dashboard. Additionally, the webhook listener's logic is enhanced to ensure that the webhook job is executed only when the payload’s inbox matches. Frontend Vue components are updated with new props and methods to support inbox selection, and corresponding tests have been modified to reflect these changes. Changes
Sequence Diagram(s)sequenceDiagram
participant E as Event Source
participant WL as WebhookListener
participant WJ as WebhookJob
E->>WL: deliver_account_webhooks(payload)
alt Payload has inbox & matches webhook.inbox_id
WL->>WJ: perform_later(webhook, payload)
else
WL-->>E: Skip processing (inbox mismatch)
end
sequenceDiagram
participant U as User
participant WF as WebhookForm
participant MD as MultiselectDropdown
U->>WF: Open Edit Webhook form (isEditing enabled)
WF->>MD: Load inbox options
U->>MD: Select an inbox
MD->>WF: onClickAssignInbox(inbox)
WF->>WF: Update selectedInbox & inboxId
U->>WF: Submit form (inbox_id included)
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/javascript/dashboard/routes/dashboard/settings/integrations/Webhooks/WebhookForm.vue (1)
89-89: Minor naming inconsistency between JavaScript and backendThere's a slight naming difference between the JavaScript property
inboxId(camelCase) and the emitted propertyinbox_id(snake_case).While this works correctly (as the backend expects snake_case), consider using a helper function to normalize property naming between frontend and backend to maintain consistency:
onSubmit() { this.$emit('submit', { url: this.url, name: this.name, - inbox_id: this.inboxId, + inbox_id: this.inboxId, // Comment: normalizing camelCase to snake_case for API subscriptions: this.subscriptions, }); },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/controllers/api/v1/accounts/webhooks_controller.rb(2 hunks)app/javascript/dashboard/i18n/locale/en/integrations.json(1 hunks)app/javascript/dashboard/routes/dashboard/settings/integrations/Webhooks/EditWebHook.vue(1 hunks)app/javascript/dashboard/routes/dashboard/settings/integrations/Webhooks/WebhookForm.vue(7 hunks)app/javascript/dashboard/routes/dashboard/settings/integrations/Webhooks/WebhookRow.vue(2 hunks)app/javascript/shared/components/ui/MultiselectDropdown.vue(3 hunks)app/listeners/webhook_listener.rb(2 hunks)app/views/api/v1/accounts/webhooks/_webhook.json.jbuilder(1 hunks)spec/controllers/api/v1/accounts/webhook_controller_spec.rb(1 hunks)spec/listeners/webhook_listener_spec.rb(1 hunks)
🔇 Additional comments (25)
app/views/api/v1/accounts/webhooks/_webhook.json.jbuilder (1)
10-10: LGTM: Enhanced JSON response with channel_type information.Adding the channel_type attribute to the webhook JSON response when an inbox is associated with the webhook provides more complete information to frontend consumers.
app/javascript/dashboard/routes/dashboard/settings/integrations/Webhooks/WebhookRow.vue (2)
6-6: LGTM: New component import for inbox display.The import of the InboxName component is appropriate for displaying inbox details in the webhook row.
39-41: LGTM: Enhanced UI with conditional inbox display.The conditional rendering of the InboxName component when a webhook has an associated inbox enhances the UI by displaying relevant inbox information. The component is properly integrated with appropriate class styling.
app/listeners/webhook_listener.rb (2)
6-6: LGTM: Improved code readability.Adding a blank line improves the readability of the method.
92-92: LGTM: Added proper inbox filtering for webhooks.This change ensures that webhooks associated with specific inboxes will only be triggered for events related to that particular inbox, preventing unwanted webhook triggers across different inboxes.
app/javascript/dashboard/routes/dashboard/settings/integrations/Webhooks/EditWebHook.vue (1)
56-56: LGTM: Added editing state indicator to form component.Adding the
is-editingprop to the WebhookForm component appropriately communicates that this form is being used in an editing context, which allows the form to adapt its behavior or appearance accordingly.spec/listeners/webhook_listener_spec.rb (2)
79-87: Well-structured test for inbox-matching behavior.This test case correctly verifies that when a webhook's inbox matches the event's inbox, the webhook job is triggered with the expected parameters.
89-97: Good negative test case for inbox filtering.This test correctly verifies that webhooks are not triggered when their associated inbox doesn't match the event's inbox, which is essential for ensuring proper filtering behavior.
app/javascript/dashboard/i18n/locale/en/integrations.json (1)
36-42: Good addition of localization strings for inbox selection.The new inbox selection UI strings are clear and provide all the necessary text elements for a complete user experience including placeholder text and empty state messages.
spec/controllers/api/v1/accounts/webhook_controller_spec.rb (1)
115-117: Test correctly updated to focus on name updates only.The test now properly verifies that webhook names can be updated, which aligns with the controller changes that likely restrict updates to only the name and subscriptions.
app/javascript/shared/components/ui/MultiselectDropdown.vue (3)
38-45: Good addition of flexibility with new props.Adding the
buttonVariantanddisabledprops enhances the component's reusability by allowing consumers to customize the appearance and state of the dropdown.
69-74: Improved button accessibility and flexibility.Adding the
type="button"attribute and binding the button's variant and disabled state to props improves both accessibility and flexibility of the component.
115-117: Improved button semantics.Adding
type="button"to the close button ensures it's properly recognized as a button by browsers and assistive technologies, which is good for accessibility.app/controllers/api/v1/accounts/webhooks_controller.rb (4)
10-10: Good method name change for webhook creationThe change to use
webhook_create_paramsinstead ofwebhook_paramsfor the create action aligns well with the separation of concerns between create and update operations.
15-15: Good restriction of update permissionsThe update action now correctly uses
webhook_update_params, which limits the parameters that can be modified during an update operation, improving security.
25-27: Appropriate parameter permissions for webhook creationThe renamed method properly permits all necessary fields for webhook creation including inbox_id, name, url, and subscriptions.
29-31: Good security practice: limiting update parametersThis new method restricts what can be updated on an existing webhook, preventing changes to the URL and inbox_id after creation. This is a good security practice as it prevents potential abuse or unintended consequences from changing these critical fields.
app/javascript/dashboard/routes/dashboard/settings/integrations/Webhooks/WebhookForm.vue (8)
6-7: Good component imports for enhanced functionalityThe imports of MultiselectDropdown and useMapGetter are appropriate for implementing the inbox selection feature.
23-25: Proper component registrationThe MultiselectDropdown component is correctly registered in the components section.
39-42: Well-defined isEditing propThe isEditing prop is appropriately defined with a default value, which will control field visibility and editability based on whether we're creating or updating a webhook.
46-49: Good use of composables for accessing store dataThe setup function correctly uses the useMapGetter composable to retrieve inbox data from the store, making it available to the component.
65-65: Proper initialization of inbox-related dataThe inboxId and selectedInbox state variables are correctly initialized from props, ensuring form state consistency.
Also applies to: 68-68
93-96: Well-implemented inbox selection handlerThe onClickAssignInbox method correctly handles inbox selection, updating both the display value and the ID needed for submission.
111-111: Correctly disabled URL field when editingThe URL field is appropriately disabled in edit mode, which aligns with the backend restriction that prevents URL modification during updates.
128-153: Well-structured inbox selection UIThe implementation of the inbox selection dropdown is well-structured and correctly:
- Uses the MultiselectDropdown component
- Passes appropriate props including options from the store
- Handles selection events
- Disables the dropdown when in edit mode
- Includes proper internationalization for all text elements
This aligns perfectly with the backend changes that support inbox_id in webhook creation.
| it 'ignores trying to update inbox_id and url' do | ||
| put "/api/v1/accounts/#{account.id}/webhooks/#{webhook.id}", | ||
| params: { url: 'https://other.url.com', inbox_id: create(:inbox, account: account).id }, | ||
| headers: administrator.create_new_auth_token, | ||
| as: :json | ||
| expect(response).to have_http_status(:success) | ||
| expect(response.parsed_body['payload']['webhook']['inbox_id']).not_to eql inbox.id | ||
| expect(response.parsed_body['payload']['webhook']['url']).not_to eql 'https://other.url.com' | ||
| end |
There was a problem hiding this comment.
Assertion appears incorrect for testing immutability.
The test logic seems to be testing the wrong condition for immutability.
If you're testing that the webhook's inbox_id and url remain unchanged when updates are attempted, the assertions should verify that they still match the original values, not that they differ:
- expect(response.parsed_body['payload']['webhook']['inbox_id']).not_to eql inbox.id
- expect(response.parsed_body['payload']['webhook']['url']).not_to eql 'https://other.url.com'
+ expect(response.parsed_body['payload']['webhook']['inbox_id']).to eql inbox.id
+ expect(response.parsed_body['payload']['webhook']['url']).to eql 'https://hello.com'The current assertions suggest you expect different values, which contradicts the test's purpose of verifying that these fields can't be changed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it 'ignores trying to update inbox_id and url' do | |
| put "/api/v1/accounts/#{account.id}/webhooks/#{webhook.id}", | |
| params: { url: 'https://other.url.com', inbox_id: create(:inbox, account: account).id }, | |
| headers: administrator.create_new_auth_token, | |
| as: :json | |
| expect(response).to have_http_status(:success) | |
| expect(response.parsed_body['payload']['webhook']['inbox_id']).not_to eql inbox.id | |
| expect(response.parsed_body['payload']['webhook']['url']).not_to eql 'https://other.url.com' | |
| end | |
| it 'ignores trying to update inbox_id and url' do | |
| put "/api/v1/accounts/#{account.id}/webhooks/#{webhook.id}", | |
| params: { url: 'https://other.url.com', inbox_id: create(:inbox, account: account).id }, | |
| headers: administrator.create_new_auth_token, | |
| as: :json | |
| expect(response).to have_http_status(:success) | |
| expect(response.parsed_body['payload']['webhook']['inbox_id']).to eql inbox.id | |
| expect(response.parsed_body['payload']['webhook']['url']).to eql 'https://hello.com' | |
| end |
gabrieljablonski
left a comment
gabrieljablonski
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @CayoPOliveira)
spec/controllers/api/v1/accounts/webhook_controller_spec.rb line 128 at r4 (raw file):
headers: administrator.create_new_auth_token, as: :json expect(response).to have_http_status(:success)
Avoid inlining create calls.
Suggestion:
it 'ignores trying to update inbox_id and url' do
new_inbox = create(:inbox, account: account)
put "/api/v1/accounts/#{account.id}/webhooks/#{webhook.id}",
params: { url: 'https://other.url.com', inbox_id: new_inbox.id },
headers: administrator.create_new_auth_token,
as: :json
expect(response).to have_http_status(:success)…improve update expectations
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/javascript/dashboard/assets/scss/widgets/_buttons.scss (1)
192-194: New.darkButton Modifier StylingThe addition of the
.darkclass (lines 192–194) appliesbg-n-slate-1to buttons. Please verify that:
- The chosen background color (
bg-n-slate-1) provides sufficient contrast with the text and aligns with your dark-theme design guidelines.- This style is consistent with other dark-themed components in the application.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/javascript/dashboard/assets/scss/widgets/_buttons.scss(1 hunks)app/javascript/dashboard/routes/dashboard/settings/integrations/Webhooks/WebhookForm.vue(5 hunks)app/javascript/dashboard/routes/dashboard/settings/integrations/Webhooks/WebhookRow.vue(2 hunks)app/listeners/webhook_listener.rb(1 hunks)spec/controllers/api/v1/accounts/webhook_controller_spec.rb(2 hunks)spec/listeners/webhook_listener_spec.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- app/listeners/webhook_listener.rb
- app/javascript/dashboard/routes/dashboard/settings/integrations/Webhooks/WebhookRow.vue
- spec/controllers/api/v1/accounts/webhook_controller_spec.rb
- app/javascript/dashboard/routes/dashboard/settings/integrations/Webhooks/WebhookForm.vue
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (1)
spec/listeners/webhook_listener_spec.rb (1)
18-40: Tests for webhook inbox filtering look good!Great job adding thorough test coverage for the inbox filtering feature. You've properly covered both positive (matching inbox) and negative (non-matching inbox) cases to ensure the webhook only triggers for the appropriate inbox.
The tests clearly validate that:
- When a webhook is associated with an inbox, it triggers for events from that inbox
- When a webhook is associated with an inbox, it doesn't trigger for events from other inboxes
These tests align well with the webhook inbox feature being implemented, ensuring webhooks can be specifically targeted to certain inboxes rather than triggering for all events account-wide.
gabrieljablonski
left a comment
gabrieljablonski
left a comment
There was a problem hiding this comment.
Reviewable status: 5 of 11 files reviewed, 12 unresolved discussions (waiting on @CayoPOliveira and @coderabbitai[bot])
app/javascript/dashboard/routes/dashboard/settings/integrations/Webhooks/WebhookForm.vue line 148 at r4 (raw file):
Previously, gabrieljablonski (Gabriel Jablonski) wrote…
secondaryvariant actually does nothing, other than overriding thehollowvariant.Try applying this patch for the
darkvariant instead:
Still not ideal (couldn't manage to update the border to match the other inputs), but looks better this way.
Ideally we should also match the "disabled" styling better, but not worth wasting more time on this right now. Creating the
darkvariant should be good enough.
Self-reminder to improve styling (maybe input-like button variant?)
CayoPOliveira
left a comment
CayoPOliveira
left a comment
There was a problem hiding this comment.
Reviewed 5 of 6 files at r5, 2 of 3 files at r6, all commit messages.
Reviewable status: 10 of 11 files reviewed, 5 unresolved discussions (waiting on @coderabbitai[bot] and @gabrieljablonski)
a discussion (no related file):
Previously, gabrieljablonski (Gabriel Jablonski) wrote…
Dropdown UX can be improved. See example for selecting agent on conversation.
- We need a "All inboxes" explicit option (FE-only, still send
nullto BE).- Close the dropdown after selecting an option
PS.: On the video above, you can see an option can be de-selected by clicking it when it is already selected. I don't think we need that.
Done.
app/javascript/dashboard/routes/dashboard/settings/integrations/Webhooks/WebhookForm.vue line 148 at r4 (raw file):
Previously, gabrieljablonski (Gabriel Jablonski) wrote…
secondaryvariant actually does nothing, other than overriding thehollowvariant.Try applying this patch for the
darkvariant instead:
Still not ideal (couldn't manage to update the border to match the other inputs), but looks better this way.
Ideally we should also match the "disabled" styling better, but not worth wasting more time on this right now. Creating the
darkvariant should be good enough.
This only works in dark mode
spec/listeners/webhook_listener_spec.rb line 97 at r4 (raw file):
Previously, gabrieljablonski (Gabriel Jablonski) wrote…
Try applying this patch instead of placing these under
#message_created, since they're supposed to test "generic" behavior.Note the changes I made to the spec descriptions, and the (very slight) newline changes to make the Arrange-Act-Assert pattern clearer (unfortunately, original specs in this file don't seem to follow that pattern).
- I applied the patch
- Added a describe to the contexts
- Changed the way the context 'when webhook has an inbox and it does not match the event inbox' performed the expect, for greater accuracy
a discussion (no related file):
-(questions/changes requested)
gabrieljablonski
left a comment
gabrieljablonski
left a comment
There was a problem hiding this comment.
Reviewed 4 of 6 files at r5, 2 of 3 files at r6, 1 of 1 files at r7, 1 of 1 files at r9, 1 of 1 files at r10, all commit messages.
Dismissed @coderabbitai[bot] from a discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @CayoPOliveira)
CayoPOliveira
left a comment
CayoPOliveira
left a comment
There was a problem hiding this comment.
Reviewed 1 of 3 files at r6, 1 of 1 files at r7, 1 of 1 files at r9.
Reviewable status:
* chore: exclude inbox_id from webhook update parameters * feat: display inbox label in webhook settings * feat: add inbox selection to webhook form * feat: prevent updating inbox_id in webhook update * feat: integrate MultiselectDropdown for inbox selection in webhook form * feat: add inbox matching logic to webhook event delivery * feat: remove unused inbox input placeholder from webhook form * fix: MultiselectDropdown component submiting form * feat: refine webhook parameters for create and update actions * feat: disable URL input field when editing webhook * chore: remove unnecessary parentheses * chore: update webhook controller spec to ignore inbox_id and url updates * fix: clean up JSON formatting * fix: standardize inbox_id to inboxId in WebhookForm component * refactor: replace LabelItem with InboxName component in WebhookRow * chore: enhance MultiselectDropdown with button variant prop and update styling in WebhookForm * chore: simplify MultiselectDropdown wrapper in WebhookForm component * chore: update selectedInbox initialization to null and reorder form fields * refactor: simplify InboxName * chore: add dark variant styling for buttons in SCSS * test: add inbox filtering for webhook event triggers in WebhookListener * test: refactor webhook controller spec to use a variable for URL and improve update expectations * feat(webhook): all inboxes option * chore: remove dark variant styling for buttons in SCSS * fix: bad interaction multiselectdropdown inside label * chore: invert if * chore: rename to assignedInbox and drop inboxId * refactor(WebhookForm): restore div separating fields from buttons * test: improve description --------- Co-authored-by: gabrieljablonski <contact@gabrieljablonski.com>
* chore: exclude inbox_id from webhook update parameters * feat: display inbox label in webhook settings * feat: add inbox selection to webhook form * feat: prevent updating inbox_id in webhook update * feat: integrate MultiselectDropdown for inbox selection in webhook form * feat: add inbox matching logic to webhook event delivery * feat: remove unused inbox input placeholder from webhook form * fix: MultiselectDropdown component submiting form * feat: refine webhook parameters for create and update actions * feat: disable URL input field when editing webhook * chore: remove unnecessary parentheses * chore: update webhook controller spec to ignore inbox_id and url updates * fix: clean up JSON formatting * fix: standardize inbox_id to inboxId in WebhookForm component * refactor: replace LabelItem with InboxName component in WebhookRow * chore: enhance MultiselectDropdown with button variant prop and update styling in WebhookForm * chore: simplify MultiselectDropdown wrapper in WebhookForm component * chore: update selectedInbox initialization to null and reorder form fields * refactor: simplify InboxName * chore: add dark variant styling for buttons in SCSS * test: add inbox filtering for webhook event triggers in WebhookListener * test: refactor webhook controller spec to use a variable for URL and improve update expectations * feat(webhook): all inboxes option * chore: remove dark variant styling for buttons in SCSS * fix: bad interaction multiselectdropdown inside label * chore: invert if * chore: rename to assignedInbox and drop inboxId * refactor(WebhookForm): restore div separating fields from buttons * test: improve description --------- Co-authored-by: gabrieljablonski <contact@gabrieljablonski.com>
* chore: exclude inbox_id from webhook update parameters * feat: display inbox label in webhook settings * feat: add inbox selection to webhook form * feat: prevent updating inbox_id in webhook update * feat: integrate MultiselectDropdown for inbox selection in webhook form * feat: add inbox matching logic to webhook event delivery * feat: remove unused inbox input placeholder from webhook form * fix: MultiselectDropdown component submiting form * feat: refine webhook parameters for create and update actions * feat: disable URL input field when editing webhook * chore: remove unnecessary parentheses * chore: update webhook controller spec to ignore inbox_id and url updates * fix: clean up JSON formatting * fix: standardize inbox_id to inboxId in WebhookForm component * refactor: replace LabelItem with InboxName component in WebhookRow * chore: enhance MultiselectDropdown with button variant prop and update styling in WebhookForm * chore: simplify MultiselectDropdown wrapper in WebhookForm component * chore: update selectedInbox initialization to null and reorder form fields * refactor: simplify InboxName * chore: add dark variant styling for buttons in SCSS * test: add inbox filtering for webhook event triggers in WebhookListener * test: refactor webhook controller spec to use a variable for URL and improve update expectations * feat(webhook): all inboxes option * chore: remove dark variant styling for buttons in SCSS * fix: bad interaction multiselectdropdown inside label * chore: invert if * chore: rename to assignedInbox and drop inboxId * refactor(WebhookForm): restore div separating fields from buttons * test: improve description --------- Co-authored-by: gabrieljablonski <contact@gabrieljablonski.com>
* chore: exclude inbox_id from webhook update parameters * feat: display inbox label in webhook settings * feat: add inbox selection to webhook form * feat: prevent updating inbox_id in webhook update * feat: integrate MultiselectDropdown for inbox selection in webhook form * feat: add inbox matching logic to webhook event delivery * feat: remove unused inbox input placeholder from webhook form * fix: MultiselectDropdown component submiting form * feat: refine webhook parameters for create and update actions * feat: disable URL input field when editing webhook * chore: remove unnecessary parentheses * chore: update webhook controller spec to ignore inbox_id and url updates * fix: clean up JSON formatting * fix: standardize inbox_id to inboxId in WebhookForm component * refactor: replace LabelItem with InboxName component in WebhookRow * chore: enhance MultiselectDropdown with button variant prop and update styling in WebhookForm * chore: simplify MultiselectDropdown wrapper in WebhookForm component * chore: update selectedInbox initialization to null and reorder form fields * refactor: simplify InboxName * chore: add dark variant styling for buttons in SCSS * test: add inbox filtering for webhook event triggers in WebhookListener * test: refactor webhook controller spec to use a variable for URL and improve update expectations * feat(webhook): all inboxes option * chore: remove dark variant styling for buttons in SCSS * fix: bad interaction multiselectdropdown inside label * chore: invert if * chore: rename to assignedInbox and drop inboxId * refactor(WebhookForm): restore div separating fields from buttons * test: improve description --------- Co-authored-by: gabrieljablonski <contact@gabrieljablonski.com>
2 participants
This change is
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
Style