-
-
Notifications
You must be signed in to change notification settings - Fork 493
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
3090 v2 distribution confirmation #4367
3090 v2 distribution confirmation #4367
Conversation
I'm waiting to do any more functional reviews until the issue with bin/setup gets addressed. (Just setting expectations) |
</table> | ||
|
||
<div class="message fs-5"> | ||
<p>Please confirm that the above list is what you meant to distribute.</p> |
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.
@danielabar Hrmm. Now that I see it in action, I want a small adjustment here -- let's change "meant" to "want".
Reason: We are displaying this before any error checking. The past tense of "meant" implies you are pretty much finished. "Want" is better for setting expectations of still being in the process. (and it still works if we can do the error checking first)
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.
Updated the wording.
@daniealbar Thank you very much for your hard work on this so far! (I really appreciate your thoroughness) Alas, During the earlier discussions, I failed to catch that the confirmation would be before basic error checking and the combination of multiples, etc. Does it have to be that way, with this approach? (I'm concerned that it won't meet the need to have useful numbers for their final review) (also calling @dorner into the discussion) |
@cielf Currently the server side part of this performs validations and saves. And this being client side, it runs before all that. Would a more ideal flow would be, from clicking Save:
It might be possible to introduce a new endpoint for validation only. It may also be possible to cause some Stimulus code to trigger as a result of render (saw something like this in a tutorial, could look into this). A tricky thing will be the validation though, I ran into this earlier when working on the first attempt pure server side. For basic validation like is partner and storage location filled in, that's straightforward. But for inventory, I it seems to be coupled with actually decrementing the inventory amount in this line in DistributionCreateService: There's also some validation about this distribution already being fulfilled (if it was created from a request) that also happens together in the creation service, making it difficult to tease apart creation from validation. There's another complexity with a further inventory check that happens after creation success wrt on-hand minimums. Although that may be only a flash alert after the fact, but doesn't stop creation. So this might require some server side restructuring of the code to pull out all the distribution validation logic into one place that can be called independently of saving/updating anything. |
1b39ec1
to
09c6b1b
Compare
@danielabar If I'm understanding correctly, It sounds like this would be a bit of a mess, I mean challenge... g and that it that could make the code harder to understand. The distribution confirmation is a "nice to have" , rather than core functionality. We'll have to make a judgement call on whether the benefit derived is worth the increased complexity of the code. I'd like to get @dorner 's take on this, see if there's an angle you haven't considered, but also to get his read on maintainablity. I know you've put a lot of work into this one. |
I think the idea of separating out validation from creation is a good one. I think it'd make it easier to understand, not harder, because we aren't shoving them all together. I think that should happen in a separate PR though. There also should be a focus on event sourcing, so maybe validating inventory items is not quite as important. (I don't think there's a direct method to say "initialize this event and check if the inventory can handle it", but it's not hard to do - maybe 2-3 lines of code.) Once we have them separated, the flow becomes a lot easier to work with and we can more easily have that confirmation page. The fact is that without a way to validate the distribution without creating it, this ticket is pretty much impossible to complete. Whether this is the most important thing for Daniela to spend her time on is a judgment call for @cielf I think. |
IIUC, the stuff that we'd have to do to make this happen is a good idea anyway? I am, however, starting a conversation with @danielabar over in slack about the partner profile rework - which is in the important but we're having trouble getting to it bucket. |
OK - I think the action for this should be to create another issue around separating validation from creation logic, and blocking this issue on that one. Prioritization is a separate question. |
I'm not sure I know enough about this to write it up -- though one question would be whether we want to separate it just for distributions, or across the board of itemizables? (for consistency's sake, I would think the latter) |
An attempt at new issue title/description: Title: Refactor Distribution Creation to Separate Validation from Creation Label: Description: Details:
What we'd like to do is separate out all the validations so that they can be called separately from creation, ideally from a single call to The benefit is this would allow for a future endpoint that only does validation via TODO: Are there any significant differences between Itemizable validation As for expanding to all the Itemizables, I haven't had a look but it's possible that the business rules and validation implementation could be different on each, so it may keep the PR more manageable to have separate tickets, starting just with Distribution model, since we know it has the issue of validation coupled with creation. |
Let's keep it to Distribution. The inventory thing should not be a focus of this. In event world, we're going to get rid of inventory items entirely, so I'd ignore it as much as possible. |
Thanks for the clarification, this simplifies things in that it's only the "is there an associated request that's already fulfilled?" validation that isn't part of the distribution validations. Thinking about this further, if |
b22fe0c
to
b48bb23
Compare
@cielf I've made some changes so that first it will run some basic validations on the distribution before showing the modal (from user clicking Save on new distribution). For now, this runs the existing validations defined on the Distribution model via And there might still be a case of "request already fulfilled", but I wasn't sure if that validation actually belongs in distribution model (eg: that would also run on update). Also wanted to see if this is closer to the desired flow wrt validation. Can you try this out whenever you have a chance. For example, if you leave out Partner and try to Save, it won't show the modal, instead it will show the validation error. Then if you fix the error by filling out partner and Save again, then the modal will show. |
You're on my list! -- it's a little long at the moment (managing expectations) |
This is definitely closer! It feels a lot smoother to me at this point. Is it plausible to handle the case where someone has entered the same item twice before we show the modal? |
b48bb23
to
4ce528e
Compare
Yeah -- it is allowed. They currently get smooshed together using the distribution's method combine_distribution, which is called before_save. ("No, it' not plausible" may be a valid answer!) |
I had a look at But in the confirmation modal, it's client side and only has what it can parse from the DOM. It might be possible to write a similar |
Got the combine display working in the confirmation modal, see this commit for details. |
Just to clarify, the combining of line items in the modal is display only, it doesn't modify the model because it hasn't gone to the server yet. So if user clicks "No I need to make changes", the line items in the form will be exactly as the user originally entered them. |
It may be tomorrow before I have a chance to try this out, but it sounds workable to me from a functionality pov |
Functionality looks good. Over to @dorner for technical review. |
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.
Had a suggestion to tweak the approach. Overall this looks like great work!
app/assets/stylesheets/confirm.scss
Outdated
.message { | ||
margin-top: 40px; | ||
} | ||
} |
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.
Do we need a special stylesheet just for one rule? :)
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 into custom.scss
@@ -10,6 +10,7 @@ class DistributionsController < ApplicationController | |||
before_action :enable_turbo!, only: %i[new show] | |||
skip_before_action :authenticate_user!, only: %i(calendar) | |||
skip_before_action :authorize_user, only: %i(calendar) | |||
skip_before_action :verify_authenticity_token, only: :validate |
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.
In general it's better not to skip this unless there's a good reason to. If the issue is JavaScript, there is a way to send the CSRF token so this passes: https://bloggie.io/@kinopyo/sending-non-get-requests-with-fetch-javascript-api-in-rails
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 a strange one, with the current code that extracts form data, the authenticity_token is indeed being submitted, and validated by Rails successfully, at least, the first time.
But in the scenario where the first time the form is invalid, then user submits again with same/different validation errors, the correct token is being included, but Rails says its invalid.
I tried also with the technique mentioned in that blog post (using header rather than in post body), but the same issue occurs.
Thought it might be due to JS DOM parsing or Stimulus lifecycle keeping a reference to the old token, but that's not the case, it is submitting the latest token.
Requires further investigation if the token verification needs to be there also for the validate endpoint.
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.
Did a little more digging into ActionPack (lib/action_controller/metal/request_forgery_protection.rb), where verify_authenticity_token
is implemented:
It tries three possible ways to validate the given token:
compare_with_global_token(csrf_token) ||
compare_with_real_token(csrf_token) ||
valid_per_form_csrf_token?(csrf_token)
The very first time the form is submitted (and Stimulus controller intercepts and submits POST /distributions/validate.json
, compare_with_global_token(csrf_token)
returns true.
But on all subsequent attempts, compare_with_global_token(csrf_token)
and compare_with_real_token(csrf_token)
return false, and so it attempts the last option valid_per_form_csrf_token?(csrf_token)
.
That last method calculates the correct_token
based on the request path and method:
correct_token = per_form_csrf_token(
session,
request.path.chomp("/"),
request.request_method
)
I put a breakpoint here and compared correct_token when the validate endpoint is submitted vs the create endpoint. Since the request.path
is different, /distributions.validate.json
vs /distributions
, it doesn't match for the validate endpoint.
So I think the token that gets generated in the form upon a render from the first validation error, represents a token for the create
endpoint, and so it won't be correct if the validate
endpoint is submitted.
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 this any help? JakeChampion/fetch#424 (comment) Also, a comment there notes that in Rails 5, every form has its own token, so the link I mentioned wouldn't work any more.
|
||
openModal(event) { | ||
event.preventDefault(); | ||
// this.debugFormData(); |
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.
Remove this line?
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.
Removed
.then((response) => response.json()) | ||
.then((data) => { | ||
if (data.valid) { | ||
console.log("=== DistributionConfirmationController VALID"); |
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.
Do we need this in production?
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.
Removed
const partnerName = this.partnerSelectionTarget.selectedOptions[0].text; | ||
const storageName = this.storageSelectionTarget.selectedOptions[0].text; | ||
this.partnerNameTarget.textContent = partnerName; | ||
this.storageNameTarget.textContent = storageName; |
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'm not the biggest fan of basically reinventing ERB with this. 😂 The combination of line items is also something that we already have implemented in Ruby and have to do again here.
I'm wondering if we can reuse just the ERB for this. Something like:
- Send the fetch for validation
- If valid, it sends the HTML to render, e.g.
{"valid": true, "body": "<div ...></div>"}
- Otherwise you submit the form as usual
You can build the distribution and line items and render the confirmation form with ERB that way. Something like
def validate
@dist = Distribution.new(distribution_params.merge(organization: current_organization))
@dist.line_items.combine!
if @dist.valid?
body = render_to_string
render json: {valid: true, body: body}
else
render json: {valid: false}
end
end
Then you just add validate.html.erb
and use it like you would any other view. The Stimulus controller becomes super simple in that case, you just do something like modalTarget.innerHTML = response.body
.
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.
Implemented, a few notes:
Had to specify the format as html (otherwise was trying to render json) and layout false, to get a focused view of only the modal body:
body = render_to_string(template: 'distributions/validate', formats: [:html], layout: false)
Indeed this greatly simplified the stimulus controller, I was able to remove most of the targets. This could make it easier to re-use for other model confirmations in the future, by passing in a different preCheckPath
value.
within "#distributionConfirmationModal" do | ||
expect(page).to have_content("You are about to create a distribution for") | ||
expect(find(:element, "data-testid": "distribution-confirmation-partner")).to have_text(partner.name) | ||
expect(find(:element, "data-testid": "distribution-confirmation-storage")).to have_text(storage_location.name) |
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 we hardcode the names? Faker has bitten us with this before. :)
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.
In the case of spec/system/distribution_system_spec.rb
, the storage location and partner names are defined with let
and FactoryBot near the top of the file:
let(:storage_location) { create(:storage_location, organization: organization) }
let!(:partner) { create(:partner, organization: organization) }
Then used throughout, for example:
select partner.name, from: "Partner"
select storage_location.name, from: "From storage location"
Did you mean to change the FactoryBot create to pass in specific names and then replace throughout the entire spec file? Something like:
let(:storage_location) { create(:storage_location, organization: organization, name: "Test Storage Location") }
let!(:partner) { create(:partner, organization: organization, name: "Test Partner") }
...
# Later in tests
select "Test Partner", from: "Partner"
select "Test Storage Location", from: "From storage location"
...
expect(find(:element, "data-testid": "distribution-confirmation-partner")).to have_text("Test Partner")
And so on for all occurrences?
btw, the storage location factory already has a hard-coded name:
name { "Smithsonian Conservation Center" }
Although using that in the distribution system spec expectations might create some confusion as to where that value from.
The partner factory on the other hand uses some variability, although with a sequence rather than faker:
sequence(:name) { |n| "Leslie Sue, the #{n}" }
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.
Did you mean to change the FactoryBot create to pass in specific names and then replace throughout the entire spec file?
Yep, exactly! The factory values are IMO something that should only be created but never referenced, because as you say it's really confusing as to where the values come from. If we need to check the value, we should always be providing it ourselves.
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.
Updated to specific partner and storage names.
9b5d79c
to
bddcf4e
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.
Getting much better!
@@ -10,6 +10,7 @@ class DistributionsController < ApplicationController | |||
before_action :enable_turbo!, only: %i[new show] | |||
skip_before_action :authenticate_user!, only: %i(calendar) | |||
skip_before_action :authorize_user, only: %i(calendar) | |||
skip_before_action :verify_authenticity_token, only: :validate |
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 this any help? JakeChampion/fetch#424 (comment) Also, a comment there notes that in Rails 5, every form has its own token, so the link I mentioned wouldn't work any more.
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 we rename this to just confirmation_controller
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.
Renamed.
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: csrf token, that linked discussion seem to be about ensuring that the token from the form gets included in the request. For the confirmation modal with the validation request, it is including the token from the form. The issue is that after an initial validation failure (i.e. round trip to server and render the new form again), the token from the form is only good for a POST to the create endpoint, and so it fails on the POST to the validation endpoint.
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.
There might be another way to do it: The reason that validate
endpoint is a POST was convenience for easily being able to submit the form in the same way as creation. But since it doesn't change state, it could be converted to a GET. In this case, csrf checking isn't an issue. I have something working locally, but it requires adding more JS to the stimulus controller to convert the form object to a query 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.
I think the issue might be that it's automatically including the token that's included as a hidden field in the form. Can you try removing it from the form data before sending it, and only including the meta
token as a header? POST is a better approach because it doesn't have any size limitations.
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.
There might be another way to fix it - looks like Rails UJS has a function refreshCSRFTokens()
- not sure if that would magically fix this. https://stackoverflow.com/questions/55040734/csrf-token-error-occurs-when-using-turbolinks-with-ruby-on-rails
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.
Using the meta level token rather than form specific resolves the issue. However, all the distribution system tests are failing. This seems to be because the meta level token doesn't get populated in testing mode, so this fails:
// failure here during system tests
const metaToken = document.querySelector("meta[name='csrf-token']").content;
fetch(this.preCheckPathValue, {
method: "POST",
headers: {
"X-CSRF-Token": metaToken,
"X-Requested-With": "XMLHttpRequest",
"Content-Type": "application/json",
Accept: "application/json"
},
body: JSON.stringify(formObject),
credentials: "same-origin"
})
...
Will keep investigating.
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.
Got it working, this commit uses meta token instead of form level, and handles test mode: 39b4a41
re: that most recent system test failure from CI run: https://github.com/rubyforgood/human-essentials/actions/runs/9536196385/job/26282824787?pr=4367 That test passes locally, and even looking at screenshot from CI artefact download, there is a "Distributions" link that the test should be able to click. |
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.
Non-blocking comment. I love where you landed, this is so much nicer! @cielf did you want to take a look functionality-wise?
"X-CSRF-Token": this.getMetaToken(), | ||
"X-Requested-With": "XMLHttpRequest", | ||
"Content-Type": "application/json", | ||
Accept: "application/json" |
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.
Nitpick - can we put quotes around "Accept" to make it match the rest of the object?
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.
Sure, I can also rebase while I'm at it, in case @cielf will be retesting for functionality.
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.
Quotes added.
…n form submit and render a bootstrap modal displaying partner name, storage location, and list of items and quantities. The No button simply closes the modal so user can return to the new distribution form to make further edits. The Yes button submits the form and the existing workflow continues.
1. Add validation only endpoint for distribution 2. Update confirmation JS controller to check if form is valid (using the new validation endpoint) before displaying modal.
Combine same named items & quantities for display in confirmation modal
* Move confirm style into custom * Remove call to debug form data * Remove console logs for valid and invalid
* Move modal content generation to server side rendering * Remove unneeded dom targets from stimulus controller
39b4a41
to
c0cd49a
Compare
Looks good to me. We can merge after the release. |
Awesome! Thanks so much for your perseverance here! |
@danielabar: Your PR |
Resolves #3090
Description
Introduce a Stimulus controller for the new distribution confirmation feature. It intercepts the Save form submission (only for New distributions, not Edit, as per earlier disucssion).
This controller extracts key information from the new distribution form including:
And displays these in a modal asking the user if they're sure this is what they intend.
The user can click "No...", which will close the modal and then they're still on the new form where they can continue filling it out, making changes, etc.
Or the user can click "Yes..." in which case the form is submitted, and then the existing server-side flow runs.
Note: There's a previous PR from an earlier attempt which introduced a new
pending
status for the Distribution model and saved the distribution in this state as part of the confirmation flow: #4341. But after some discussion, it was decided to go with a simpler client-side JS approach as per this PR.Type of change
New feature (non-breaking change which adds functionality)
How Has This Been Tested?
Screenshots
Here is an example of the new distribution confirmation modal, shown after user clicks Save: