Conversation
* the enum changes and the default in submission.rb don't REALLY matter since almost all of our changes in future commits defer to templates. * add template methods to know how many actual submitters there are and add complex default logic based on when fields are added or removed. For example: If only 1 employee field it's single sided. If we add a manager field it automatically changes to employee_then_manager unless manually changed to a different dual sided. If either field is removed, it automatically switches back to single_sided
- replace submitters_order_preserved? with signing_order_enforced? in send_signature_requests - add manager_then_employee branch to send_signature_requests to send to second submitter first, while we don't send out emails with Docuseal, there are changes further down the line required - skip submitters without fields for single_sided in create_from_submitters, this is mostly necessary for single_sided manager forms - refactor current_submitter_order? to reverse submitter_items for manager_then_employee instead of special-casing index
* when saving a template, check if preferences have changed, if it has changed, fire webhook event. * changes in templates_controller.rb are for automatic updates based on field types. So if only 1 field type (employee fields only) this automatically updates * template_preferences_controller.rb handles manual updates to signing order from user
- add SigningOrderModal component for selecting signing order from within the template builder - show signing order button in builder toolbar only when template has 2+ submitter fields
- add template.preferences_updated to account default webhook events - guard account create_careerplug_webhook against missing CAREERPLUG_WEBHOOK_URL env var - create partnership-scoped webhook for template.preferences_updated on partnership creation - add template.preferences_updated to WebhookUrl::EVENTS - update PARTNERSHIP_EVENTS to only include template.preferences_updated - return WebhookUrl.none instead of raising for templates with neither account nor partnership - extend webhooks:setup_development rake task to create partnership webhooks
* we're requiring two points of contact in the db for multitenancy
bernardodsanderson
left a comment
There was a problem hiding this comment.
Really good stuff! I have some stuff on this first pass. I can then manually test once these are looked at.
lib/submitters.rb
Outdated
|
|
||
| before_items.reduce(true) do |acc, item| | ||
| acc && submitter.submission.submitters.find { |e| e.uuid == item['uuid'] }&.completed_at? | ||
| before_items = ordered_items[0...(ordered_items.find_index { |e| e['uuid'] == submitter.uuid })] |
There was a problem hiding this comment.
If find_index returns nil, this will raise a NoMethodError. We should probably do something to handle the error a bit better.
There was a problem hiding this comment.
I expanded this so that the method returns true, false or nil. If it's nil, we surface an error to the user so they know something is wrong.
| include PartnershipContext | ||
|
|
||
| skip_before_action :verify_authenticity_token | ||
| skip_before_action :authenticate_via_token! |
There was a problem hiding this comment.
We should maybe add a comment explaining why we're skipping these
There was a problem hiding this comment.
Added a comment there, it's because of the IframeAuthentication concern, they used to use Devise, but we don't need that since we don't have them log into Docuseal.
|
|
||
| require 'rails_helper' | ||
|
|
||
| RSpec.describe Submitters do |
There was a problem hiding this comment.
This looks great, but we should probably add tests for single_sided and simultaneous too
| end | ||
| end | ||
|
|
||
| def enqueue_template_preferences_updated_webhooks(template) |
There was a problem hiding this comment.
Did you want this method to be the same here and in templates_preferences_controller.rb? If so, maybe we can extract to a service.
class EnqueueTemplateWebhooks
def self.preferences_updated(template)
# ...
end
endThere was a problem hiding this comment.
Good idea, I also extracted two other template webhook methods into a shared concern.
| </span> | ||
| </label> | ||
| <div class="space-y-2"> | ||
| first_party = @template.submitters.first['name'] || t('first_party') |
There was a problem hiding this comment.
This is missing the <% tag.
| </span> | ||
| </label> | ||
| <div class="space-y-2"> | ||
| first_party = @template.submitters.first['name'] || t('first_party') |
There was a problem hiding this comment.
We should probably use safe navigation for this check
first_party = @template.submitters.first&.[]('name') || t('first_party')
|
|
||
| module Submissions | ||
| DEFAULT_SUBMITTERS_ORDER = 'random' | ||
| DEFAULT_SUBMITTERS_ORDER = 'single_sided' |
There was a problem hiding this comment.
Not sure if it matters, but in submission.rb, the default is employee_then_manager
There was a problem hiding this comment.
Good catch, I think we're always using this file for our API submission creations, but obviously we want consistency 👍
| this.$emit('close') | ||
| }).catch((error) => { | ||
| console.error('Error saving signing order:', error) | ||
| alert(this.t('error_occurred')) |
There was a problem hiding this comment.
Maybe this error could be more descriptive since it's user-facing
* handle submitter UUID not matching correctly with flash alert that surfaces to user * add more testing for simultaneous and single sided orders * add comment for skipping Devise auth for Iframe auth * refactor template webhook enqueue to a shared concern * use safe navigation for first_party name * make default submitters_order value consistent between `lib/submissions.rb` and `submission.rb` * more descriptive error message for signing order error
we used to just return true or false, but we are using nil to signify that the submitter uuid is not found for the controller so the error can be surfaced to the user.
bernardodsanderson
left a comment
There was a problem hiding this comment.
Approving it, but I think the missing translations should be looked at. I just don't need to look at it again after it's fixed.
lib/submitters.rb
Outdated
| end | ||
|
|
||
| def current_submitter_order?(submitter) | ||
| def current_submitter_order(submitter) |
There was a problem hiding this comment.
Nit: I think a better name might be something like validate_submitter_order
| if submission.signing_order_enforced? | ||
| first_submitter = if submission.template_signing_order == 'manager_then_employee' | ||
| # For manager_then_employee, send to the second submitter first | ||
| submission.template_submitters[1..].filter_map do |s| |
There was a problem hiding this comment.
Optional: would it be prudent to make sure the array has more than one element first? Otherwise, if for some reason the array has only the manager, this will return nil. I guess the only possible way to do this would be to go back and delete the employee fields (maybe?).
There was a problem hiding this comment.
The nil should already be handled on line 159 Submitters.send_signature_requests([first_submitter], delay_seconds:) if first_submitter. This is probably even a bit overkill since we validate that manager_then_employee and employee_then_manager requires two submitters, otherwise single_sided is automatically set and this method conditional goes straight to line 161.
| class: 'radio radio-primary mt-0.5', | ||
| onchange: 'this.form.requestSubmit()' %> | ||
| <div class="flex-1"> | ||
| <div class="font-medium"><%= t('employee_then_manager_title', first_party: first_party, second_party: second_party) %></div> |
There was a problem hiding this comment.
I didn't find this (employee_then_manager_title) in the translation file
| onchange: 'this.form.requestSubmit()' %> | ||
| <div class="flex-1"> | ||
| <div class="font-medium"><%= t('simultaneous_signing_title') %></div> | ||
| <div class="text-sm text-base-content/70"><%= t('simultaneous_signing_description') %></div> |
There was a problem hiding this comment.
I didn't find this (simultaneous_signing_description) in the yaml translation file, but it was in the js one
* change current_submitter_order to validate_submitter_order for clarity * add translations
Summary
This PR replaces Docuseal's legacy
preserved/randomsigning order model with named, explicit signing order values (employee_then_manager,manager_then_employee,simultaneous,single_sided). It also introduces thetemplate.preferences_updatedwebhook event so external API can update when a template's signing order changes.Changes:
New webhook event (template.preferences_updated):
SendTemplatePreferencesUpdatedWebhookRequestJobfires whensubmitters_orderchanges on a template — triggered from both TemplatesController#update andTemplatesPreferencesController#create
Testing:
template.preferences_updatedLoom:
https://www.loom.com/share/b1e353a27c59457791f993fd995d7bd7