-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat: resend all button for missed posts/emails #2246
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
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.
@EmCousin _a I've updated the PR description showcasing partial visit and polling behaviour.
Also, left a few review comments to explain my changes, feel free to ping in-case it's not understandable
There's just one actionable comment from previous review #2149 (comment) which might need your attention
Please have a look where you're free 🙏
| if !request.inertia_partial? | ||
| product = Link.fetch(params[:link_id]) if params[:link_id].present? | ||
| sales = fetch_sales(products: [product].compact) | ||
| customers_presenter = CustomersPresenter.new( | ||
| pundit_user:, | ||
| product:, | ||
| customers: load_sales(sales), | ||
| pagination: { page: 1, pages: (sales.results.total / CUSTOMERS_PER_PAGE.to_f).ceil, next: nil }, | ||
| count: sales.results.total | ||
| ) | ||
| create_user_event("customers_view") | ||
| end | ||
|
|
||
| purchase = current_seller.sales.find_by_external_id!(params[:purchase_id]) if params[:purchase_id].present? | ||
|
|
||
| render inertia: "Customers/Index", | ||
| props: { customers_presenter: customers_presenter.customers_props } | ||
| render inertia: "Customers/Index", props: { | ||
| customers_presenter: (-> { customers_presenter.customers_props } if !request.inertia_partial?), | ||
| customer_emails: InertiaRails.optional { fetch_customer_emails(purchase) }, | ||
| missed_posts: InertiaRails.optional { CustomerPresenter.new(purchase:).missed_posts(workflow_id: params[:workflow_id]) }, | ||
| workflows: InertiaRails.optional { WorkflowsPresenter.new(seller: current_seller).workflow_options_by_purchase_props(purchase:) }, | ||
| }.compact |
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.
Fetches drawer data on partial visits
| render json: [] | ||
| end | ||
|
|
||
| def customer_emails |
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.
Moved to #index to reduce network calls as per #2149 (comment)
| customers_presenter: (-> { customers_presenter.customers_props } if !request.inertia_partial?), | ||
| customer_emails: InertiaRails.optional { fetch_customer_emails(purchase) }, | ||
| missed_posts: InertiaRails.optional { CustomerPresenter.new(purchase:).missed_posts(workflow_id: params[:workflow_id]) }, | ||
| workflows: InertiaRails.optional { WorkflowsPresenter.new(seller: current_seller).workflow_options_by_purchase_props(purchase:) }, |
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.
Moved over here as per #2149 (comment)
| import EmptyState from "$app/components/Admin/EmptyState"; | ||
| import PaginatedLoader, { type Pagination } from "$app/components/Admin/PaginatedLoader"; | ||
| import UserCard, { type User } from "$app/components/Admin/Users/User"; | ||
| import EmptyState from "$app/components/ui/EmptyState"; |
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.
Re-factored as per #2149 (comment)
| customers: prev.customers.map((customer) => (customer.id === id ? { ...customer, ...update } : customer)), | ||
| })); | ||
| const [isLoading, setIsLoading] = React.useState(false); | ||
| const [isLoadingPurchaseData, setIsLoadingPurchaseData] = React.useState(false); |
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.
This helps in displaying a loader for "Missed Posts" and "Emails" section when the drawer is open and a new purchase record is selected
| expect(PostSendgridApi.mails.keys).to include(purchase.email) | ||
| end | ||
|
|
||
| it "raises CustomerOptedOutError when purchase has opted out" do |
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.
Newly added case which checks for opt out status
| end.to raise_error(ActiveRecord::RecordNotFound) | ||
| end | ||
|
|
||
| it "handles CustomerOptedOutError gracefully without retrying" do |
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.
Newly added case which checks for opt out status
| setLoadingId("all"); | ||
| try { | ||
| const response = await resendPosts(customer.id, selectedWorkflowId === "" ? undefined : selectedWorkflowId); | ||
| trackPostsBeingSent(customer.id, selectedWorkflowId === "" ? undefined : selectedWorkflowId); |
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.
Poll for email delivery status
| purchase_id: purchaseId, | ||
| workflow_id: workflowId, | ||
| }, | ||
| only: ["customer_emails", "missed_posts"], |
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.
This transfers the mails from sending -> sent bucket
| } | ||
| }; | ||
|
|
||
| React.useEffect(() => () => stopPolling(), [customer.id, selectedWorkflowId]); |
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.
Poll cancellation conditions
- User selects a new workflow
- User selects a new purchase record
- User closes the drawer
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.
If the user closes the drawer immediately after queuing the resend, the job might finish but we never refresh the “missed posts” state because the interval was cleaned up, correct? If so, when they reopen the drawer later, they’ll still see the stale list until they manually reload.
Overall this polling mechanism is a bit complicated for what we need here. It would be simpler and much more deterministic to send a completion status from the server via WebSocket since we already use Anycable: you could send a completion status when the job is complete and you could subscribe here to cleanup the posts and show an success toaster.
| } | ||
| }, | ||
| }); | ||
| }, 3500); |
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.
Polling via usePoll hook from inertiajs doesn't work with dynamic requestOptions because the hook captures requestOptions in a closure on first render via useRef.
When selectedWorkflowId changes, the poll continues using the initial value, therefore we need to write custom polling logic
// Inertia bug explanation
const { start: trackPostsBeingSent, stop: stopTrackingPostsBeingSent } = usePoll(
3500,
// requestOptions
{
data: {
purchase_id: customer.id,
// This value changes when switching between workflows,
// but poll calls don't reflect the change and continue using
// the initial value of `undefined`.
workflow_id: selectedWorkflowId === "" ? undefined : selectedWorkflowId,
},
only: ["customer_emails", "missed_posts"],
preserveUrl: true,
onSuccess: (page) => {
const { missed_posts } = cast<{ missed_posts?: MissedPost[] }>(page.props);
if (!missed_posts?.length) {
stopTrackingPostsBeingSent();
setLoadingId(null);
showAlert("All missed emails were sent", "success");
}
},
},
{
autoStart: false,
},
);
EmCousin
left a comment
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.
This approach is far superior to the previous one - added some comments to cover the edge cases. Should be good to merge then!
| if @post.seller_id? | ||
| @seller = @post.seller | ||
| else | ||
| @seller = @post.link.seller |
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 you explain why you need to set @seller 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.
This is for reasons highlighted over #2149 (comment) and #2149 (comment)
| onSuccess: () => setIsLoadingPurchaseData(false), | ||
| onError: () => setIsLoadingPurchaseData(false), |
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.
| onSuccess: () => setIsLoadingPurchaseData(false), | |
| onError: () => setIsLoadingPurchaseData(false), | |
| onFinish: () => setIsLoadingPurchaseData(false), |
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.
Ideally Inertia should provide a loading attribute so we don't have to reinvent this
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.
Let me have look what can be made out of it
|
|
||
| SendPostsForPurchaseService.send_missed_posts_for(purchase: @purchase, workflow_id: params[:workflow_id]) | ||
|
|
||
| render json: { message: "Missed emails are queued for delivery" }, status: :ok |
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.
If the purchase opted out, we could return a 422 status with an error toast straight away instead of enqueue-ing the job. This is more coherent from the user's standpoint. We would still need to check that at the job level (in the event of retries for instance).
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.
This should already be handled as there is a before_action :ensure_can_contact_for_purchase, only: %i[send_for_purchase send_missed_posts] callback in the controller which does it.
I'll handle it differently though using validation at service level and change the error code as well
| } | ||
| }; | ||
|
|
||
| React.useEffect(() => () => stopPolling(), [customer.id, selectedWorkflowId]); |
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.
If the user closes the drawer immediately after queuing the resend, the job might finish but we never refresh the “missed posts” state because the interval was cleaned up, correct? If so, when they reopen the drawer later, they’ll still see the stale list until they manually reload.
Overall this polling mechanism is a bit complicated for what we need here. It would be simpler and much more deterministic to send a completion status from the server via WebSocket since we already use Anycable: you could send a completion status when the job is complete and you could subscribe here to cleanup the posts and show an success toaster.
Closes #1814
This PR enables a user to resend all missed posts/emails using a single button. Missed posts can be further filtered by a workflow using the
Selectbox.Send Missed Poststo aSelectbox which allows a user to filter missed posts by workflowResend Allis clicked, a sidekiq job is triggered to send all of the missed emailsSendPostsForPurchaseServiceservice, which is then used throughout the code base.Changes After #2149
CustomersController#indexto fetch drawer data (customer emails, missed posts and workflows) in a single API call instead of/customers/missed_posts/:purchase_idand/customers/customer_emails/:purchase_idendpoints, which are now removed from codebase.SendPostsForPurchaseService#send_postBefore
before_send_missed_posts_individually.mov
After
Screenshots
Demo
1. Send all missed posts with polling for delivery status
polling-works.mov
Mails delivered
2. Inertial partial visits to fetch customer emails, missed posts and workflows
inertia-partial-loader.mov
3. Polling cancels when
polling-cancels-on-switching-worfklow_or_purchase_or_closing_drawer.mov
Specs Passing
bundle exec rspec spec/policies/installment_policy_spec.rb spec/services/send_posts_for_purchase_service_spec.rb spec/requests/customers/customers_spec.rb:457 spec/sidekiq/send_missed_posts_job_spec.rb spec/controllers/posts_controller_spec.rb spec/policies/audience/purchase_policy_spec.rb spec/controllers/workflows_controller_spec.rb spec/presenters/workflow_presenter_spec.rb spec/presenters/workflows_presenter_spec.rb spec/models/installment/installment_class_methods_spec.rbAI Disclosure
For #2149
For #2246
Live Stream Disclosure