Skip to content
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

Form Builder #4137

Merged
merged 49 commits into from Nov 25, 2022
Merged

Form Builder #4137

merged 49 commits into from Nov 25, 2022

Conversation

Kukks
Copy link
Member

@Kukks Kukks commented Sep 19, 2022

Closes #505

@dennisreimann
Copy link
Member

@Kukks It'd be good to move the generic form definitions (email, address) out of the view code into C#, so that we can use them there as well and then serialize it to JSON for the view.

Other than that I'd consider this PR done and would like to see it merged, so that I can use the forms in Checkout v2.

@Kukks
Copy link
Member Author

Kukks commented Oct 30, 2022

@Kukks It'd be good to move the generic form definitions (email, address) out of the view code into C#, so that we can use them there as well and then serialize it to JSON for the view.

Other than that I'd consider this PR done and would like to see it merged, so that I can use the forms in Checkout v2.

done

@dennisreimann
Copy link
Member

@Kukks Anything else that's left TODO here? Otherwise, let's get this in :)

@dennisreimann dennisreimann mentioned this pull request Nov 2, 2022
30 tasks
@Kukks
Copy link
Member Author

Kukks commented Nov 9, 2022

  • Add tests
  • Integrate into pos
  • integrate into crowdfund -- 1.7.x instead
  • greenfield API -- 1.7.x instead
  • form UI editor -- 1.8
  • integrate into pay button -- 1.8
  • integrate into pay request (tricky)
  • [ x] integrate into invoice

@Kukks Kukks force-pushed the form_builder branch 5 times, most recently from 34f8374 to da77d21 Compare November 15, 2022 10:49
Comment on lines 503 to 507
blob.CheckoutFormId = model.CheckoutFormId;
if (blob.CheckoutType == Client.Models.CheckoutType.V2)
{
blob.CheckoutFormId = model.CheckoutFormId;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What difference does this make? I put it into the if to prevent unintended changes in case Checkout v2 isn't used.

As the value is submitted every time with the form it wouldn't change, but I was wondering about the intention of pulling it out

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because forms are no longer tied to requiring checkout v2 - they work perfectly fine with old-style checkout too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok, in that case we should also update the view to have the select dislayed in any case. I can update that.


[AllowAnonymous]
[HttpGet("~/forms/{id}")]
public async Task<IActionResult> ViewForm(string id, string redirectUrl)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work, as the user can then manipulate the original data in the query string and the invoice might be created based on the modified data.

For the simple POS item list it works, because the price cannot be decreased (because the price is taken from the item), but for the cart view it can be manipulated — I think we should come up with a more resilient approach here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic for the cart is still acted upon as before. If the user manipulated the form HTML values before submitting, it would have the same result

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see; however that isn't ideal. It works as long as the merchant has most likely the control over the input device (as with the POS), but it'd be problematic for e.g. an online store.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One idea here: We could hash the data (or amount) with a salt saved in the store settings and use that as a checksum on the receiving action. That way we can ensure that the data doesn't get tampered in the browser..

BTCPayServer/Forms/UIFormsController.cs Outdated Show resolved Hide resolved
BTCPayServer/Forms/UIFormsController.cs Outdated Show resolved Hide resolved
BTCPayServer/Forms/UIFormsController.cs Outdated Show resolved Hide resolved
@pavlenex
Copy link
Contributor

@Kukks @dennisreimann is this supposed to work with new checkout, because it doesn't?
Screenshot 2022-11-18 at 18 25 27

@dennisreimann
Copy link
Member

@pavlenex Keep in mind that not all invoice creation methods are covered yet. See this comment.

@pavlenex
Copy link
Contributor

pavlenex commented Nov 19, 2022

I tried this PR again, but after some recent pushes I am experiencing a few problems, which are mostly do to me selecting default options. For example payment requests work with shipping address and email address but not with other options, explained below.

Problem 1: Default store settings give 404 or crashed

  1. Deployed a new store
  2. Did not touch default settings
  3. Go to Payment request (leave it to inherit store settings)
  4. Get this error
System.NullReferenceException: Object reference not set to an instance of an object.
   at BTCPayServer.Plugins.PointOfSale.Controllers.UIPointOfSaleController.ViewPointOfSale(String appId, Nullable`1 viewType, Nullable`1 amount, String email, String orderId, String notificationUrl, String redirectUrl, String choiceKey, String posData, RequiresRefundEmail requiresRefundEmail, CancellationToken cancellationToken) in /Users/p/Downloads/pavlenex/btcpayserver/BTCPayServer/Plugins/PointOfSale/Controllers/UIPointOfSaleController.cs:line 207
   at Microsoft.AspNetCore.Mvc.Infrastructure.ActionMethodExecutor.TaskOfIActionResultExecutor.Execute(IActionResultTypeMapper mapper, ObjectMethodExecutor executor, Object controller, Object[] arguments)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeActionMethodAsync>g__Awaited|12_0(ControllerActionInvoker invoker, ValueTask`1 actionResultValueTask)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeNextActionFilterAsync>g__Awaited|10_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Rethrow(ActionExecutedContextSealed context)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeInnerFilterAsync>g__Awaited|13_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeNextExceptionFilterAsync>g__Awaited|26_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ExceptionContextSealed context)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeNextResourceFilter>g__Awaited|25_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ResourceExecutedContextSealed context)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeFilterPipelineAsync>g__Awaited|20_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope)
   at Microsoft.AspNetCore.Routing.EndpointMiddleware.<Invoke>g__AwaitRequestTask|6_0(Endpoint endpoint, Task requestTask, ILogger logger)
   at Microsoft.AspNetCore.Session.SessionMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Session.SessionMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Authorization.AuthorizationMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
   at BTCPayServer.Hosting.BTCPayMiddleware.Invoke(HttpContext httpContext) in /Users/p/Downloads/pavlenex/btcpayserver/BTCPayServer/Hosting/BTCpayMiddleware.cs:line 100
   at BTCPayServer.Hosting.GreenfieldMiddleware.Invoke(HttpContext httpContext) in /Users/p/Downloads/pavlenex/btcpayserver/BTCPayServer/Hosting/GreenfieldMiddleware.cs:line 30
   at Microsoft.AspNetCore.Diagnostics.StatusCodePagesMiddleware.Invoke(HttpContext context)
   at BTCPayServer.Hosting.HeadersOverrideMiddleware.Invoke(HttpContext httpContext) in /Users/p/Downloads/pavlenex/btcpayserver/BTCPayServer/Hosting/HeadersOverrideMiddleware.cs:line 29
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)
2022-11-19 18:55:54.236 +01:00 [INF] BTCPayServer.Services.PaymentRequests.PaymentRequestUpdated

Problem 2 When selecting do not request address in payment request, I can't generate an invoice

  1. Create payment request
  2. Select Do not request any information
  3. CTA is fill out the form (incorrect)
  4. Click on the button
  5. Nothing happens
    Console logs show this
    Screenshot 2022-11-19 at 19 03 46

@Kukks
Copy link
Member Author

Kukks commented Nov 20, 2022

I tried this PR again, but after some recent pushes I am experiencing a few problems, which are mostly do to me selecting default options. For example payment requests work with shipping address and email address but not with other options, explained below.

Problem 1: Default store settings give 404 or crashed

  1. Deployed a new store
  2. Did not touch default settings
  3. Go to Payment request (leave it to inherit store settings)
  4. Get this error
System.NullReferenceException: Object reference not set to an instance of an object.
   at BTCPayServer.Plugins.PointOfSale.Controllers.UIPointOfSaleController.ViewPointOfSale(String appId, Nullable`1 viewType, Nullable`1 amount, String email, String orderId, String notificationUrl, String redirectUrl, String choiceKey, String posData, RequiresRefundEmail requiresRefundEmail, CancellationToken cancellationToken) in /Users/p/Downloads/pavlenex/btcpayserver/BTCPayServer/Plugins/PointOfSale/Controllers/UIPointOfSaleController.cs:line 207
   at Microsoft.AspNetCore.Mvc.Infrastructure.ActionMethodExecutor.TaskOfIActionResultExecutor.Execute(IActionResultTypeMapper mapper, ObjectMethodExecutor executor, Object controller, Object[] arguments)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeActionMethodAsync>g__Awaited|12_0(ControllerActionInvoker invoker, ValueTask`1 actionResultValueTask)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeNextActionFilterAsync>g__Awaited|10_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Rethrow(ActionExecutedContextSealed context)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeInnerFilterAsync>g__Awaited|13_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeNextExceptionFilterAsync>g__Awaited|26_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ExceptionContextSealed context)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeNextResourceFilter>g__Awaited|25_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ResourceExecutedContextSealed context)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeFilterPipelineAsync>g__Awaited|20_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope)
   at Microsoft.AspNetCore.Routing.EndpointMiddleware.<Invoke>g__AwaitRequestTask|6_0(Endpoint endpoint, Task requestTask, ILogger logger)
   at Microsoft.AspNetCore.Session.SessionMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Session.SessionMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Authorization.AuthorizationMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
   at BTCPayServer.Hosting.BTCPayMiddleware.Invoke(HttpContext httpContext) in /Users/p/Downloads/pavlenex/btcpayserver/BTCPayServer/Hosting/BTCpayMiddleware.cs:line 100
   at BTCPayServer.Hosting.GreenfieldMiddleware.Invoke(HttpContext httpContext) in /Users/p/Downloads/pavlenex/btcpayserver/BTCPayServer/Hosting/GreenfieldMiddleware.cs:line 30
   at Microsoft.AspNetCore.Diagnostics.StatusCodePagesMiddleware.Invoke(HttpContext context)
   at BTCPayServer.Hosting.HeadersOverrideMiddleware.Invoke(HttpContext httpContext) in /Users/p/Downloads/pavlenex/btcpayserver/BTCPayServer/Hosting/HeadersOverrideMiddleware.cs:line 29
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)
2022-11-19 18:55:54.236 +01:00 [INF] BTCPayServer.Services.PaymentRequests.PaymentRequestUpdated

Problem 2 When selecting do not request address in payment request, I can't generate an invoice

  1. Create payment request
  2. Select Do not request any information
  3. CTA is fill out the form (incorrect)
  4. Click on the button
  5. Nothing happens
    Console logs show this
    Screenshot 2022-11-19 at 19 03 46

Thanks for testing, fixed a bunch of issues today that I found. Let me know if it's a bit better.

@pavlenex
Copy link
Contributor

pavlenex commented Nov 21, 2022

@Kukks Thanks, the Payment request bug is no longer present, however I am still getting error when creating a POS with default settings, it gives 404.

To replicate:

  1. Create a store with default settings (do not set form builder, just default)
  2. Create a POS with any view (keypad, cart, etc)
  3. Try to pay or generate an invoice
  4. 404
  5. Not getting any logs

Note, it needs to have inherit store settings which is default not to collect any information on the app level or to have `do not to replicate to do a 404 crash. When you select either email or address it doesn't crash.

Screen.Recording.2022-11-21.at.11.32.21.mov

Also these two options are kinda the same on POS view? Should we remove one?

Screenshot 2022-11-21 at 11 35 27

@NicolasDorier
Copy link
Member

There are too many magic and rough edge in this code for me to be confident it is in a shippable state.

For example, I found a bug during review: there is a few "pre-made" forms. Those doesn't have a storeId associated, and such routes such as [HttpPost("~/forms/{id?}")] would crash.

@NicolasDorier
Copy link
Member

NicolasDorier commented Nov 24, 2022

This isn't ready yet.

  • what is formResponse in payment request?
  • why checkoutFormId is removed?
  • Shouldn't use TempData for flowing data like this, it's not done for that, instead should use PostRedirect
  • Missing index on Form StoreId
  • No tests written for CRUD of custom forms
  • Why isn't it possible to get a form for a normal invoice, without payment request/crowdfund?
  • At someplace, we rely on FormData.StoreId however, this isn't set for preset templates such as Email and Address.

@Kukks
Copy link
Member Author

Kukks commented Nov 24, 2022

This isn't ready yet.

  • what is formResponse in payment request?
  • why checkoutFormId is removed?
  • Shouldn't use TempData for flowing data like this, it's not done for that, instead should use PostRedirect
  • Missing index on Form StoreId
  • No tests written for CRUD of custom forms
  • Why isn't it possible to get a form for a normal invoice, without payment request/crowdfund?

Form response stores the response from the response if a formid is set

Invoice level form was removed because we were overcomplicating things for a feature that is probably not very useful. We can add it later.

Checkoutformid was removed along with the above point, for the same reason.

Custom forms may not be very used for now, since we haven't built a UI for building a form. We can potentially just hide it behind an experimental flag for now

@dennisreimann
Copy link
Member

Let's just use the preconfigured/generic forms (email and address) in the first iteration and add customizable forms later.

@Kukks
Copy link
Member Author

Kukks commented Nov 24, 2022 via email

@pavlenex
Copy link
Contributor

Awkward question, but where can one find customizable forms. Are they even in the UI? The only forms I found so far were in the apps and pre-defined?

@Kukks
Copy link
Member Author

Kukks commented Nov 24, 2022 via email

@pavlenex
Copy link
Contributor

Oh wow, I was blind, totally didn't expect us to have customizable forms in this release. I think we should abstract it from the UI, how about we roll out with the predefined ones, and just see which forms people ask? I think in the next release we may have more time to thinker about the UI here, and the need for more forms.

@Kukks
Copy link
Member Author

Kukks commented Nov 24, 2022 via email

Copy link
Member

@dennisreimann dennisreimann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored it to remove the TempData usages. To me the flows for POS and payment requests seem to be clean and intuitive now.

Also trimmed off the parts where we had custom forms (from the UI, the code is still there) so that we‘ll only ship the generic forms (email and address).

@dennisreimann
Copy link
Member

@NicolasDorier I've added the index for StoreId, but couldn't generate the migration — can you do that once you are back?

@NicolasDorier
Copy link
Member

I remove untested code and the database code.
There was some bugs in there that wasn't detected because it wasn't tested or probably not even used.

I will reopen a PR rebased with the stuff I just removed after this one so @Kukks can improve it in a separate PR.

@NicolasDorier NicolasDorier merged commit 0222858 into master Nov 25, 2022
@NicolasDorier NicolasDorier deleted the form_builder branch November 25, 2022 01:42
@NicolasDorier NicolasDorier mentioned this pull request Nov 25, 2022
@NicolasDorier
Copy link
Member

I've opened a PR adding back what I removed for supporting Generic Forms. We should do in a future PR after this release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request Buyer Address
6 participants