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

Generic Forms #4561

Merged
merged 20 commits into from
Feb 20, 2023
Merged

Generic Forms #4561

merged 20 commits into from
Feb 20, 2023

Conversation

Kukks
Copy link
Member

@Kukks Kukks commented Jan 24, 2023

This re-adds the custom forms support and reactors things considerable as there were many bugs.

Things to note:

  • the form id is not exposed when using pos/payment request. The UI is shown under a specific route
  • no tempdata usage.
  • there were bugs where the form would be in a validated state on first view

@Kukks Kukks added this to the 1.8.0 milestone Jan 24, 2023
@dstrukt dstrukt self-requested a review January 24, 2023 16:49
@dennisreimann
Copy link
Member

dennisreimann commented Jan 26, 2023

I'm still thinking about how to make it work with #4568 and have the store branding applied to the form whenever possible.

The form per se doesn't have the connection to a store, but as custom forms are set up per store, it would work for those cases. However, we still would need something like a parameter for the generic forms, which will be used most of the times.

It'd certainly be good to have that possibility, because otherwise the form as an intermediate step would miss the branding that the initial and checkout view have.

@Kukks Kukks force-pushed the form_builder_before branch 2 times, most recently from fc06004 to 1abfd53 Compare January 30, 2023 11:07
@Kukks
Copy link
Member Author

Kukks commented Feb 1, 2023

All forms are attached to a store. There is a "public" property on custom forms, which lets you use them without apps/pay requests, but the hardcoded ones are always set to false. That means the form always has a stored available to it

@dennisreimann
Copy link
Member

Added the branding aspect to the non-public forms and cleaned up the views a bit.

The test failures seem to be DB related, but at first glance I don't see what might be wrong … I cleaned up one field in the migration, but there are two errors remaining which look like they might be related to the recent blob changes — any idea @Kukks @NicolasDorier?

  Failed CanDoRateSourceMigration [7 s]
  Error Message:
   Npgsql.PostgresException : 42883: operator does not exist: text ->> unknown

POSITION: 220
  Stack Trace:
     at Npgsql.Internal.NpgsqlConnector.<ReadMessage>g__ReadMessageLong|215_0(NpgsqlConnector connector, Boolean async, DataRowLoadingMode dataRowLoadingMode, Boolean readingNotifications, Boolean isReadingPrependedMessage)
  Failed CanCreateSqlitedb [1 s]
  Error Message:
   System.NotSupportedException : SQLite does not support this migration operation ('AlterColumnOperation'). See http://go.microsoft.com/fwlink/?LinkId=723262 for more information.
  Stack Trace:
     at Microsoft.EntityFrameworkCore.Migrations.SqliteMigrationsSqlGenerator.Generate(AlterColumnOperation operation, IModel model, MigrationCommandListBuilder builder)

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.

tACK

@dennisreimann
Copy link
Member

Added validation and adapted the validation styles to use the Bootstrap 5 classes :)

grafik

@pavlenex pavlenex linked an issue Feb 14, 2023 that may be closed by this pull request
}

public async Task<FormData?> GetForm(string storeId, string id)
{
Copy link
Member

Choose a reason for hiding this comment

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

if id is null (as you did below), you should return null. Also make string id nullable. You are actually passing a null potentially in some controller.

},
_ => null
Currency = form.GetFieldByName("internal_currency")?.Value ?? store.GetStoreBlob().DefaultCurrency,
Amount = string.IsNullOrEmpty(amt) ? null : decimal.Parse(amt, CultureInfo.InvariantCulture),
Copy link
Member

Choose a reason for hiding this comment

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

I like the idea, though why is it called internal_?
Also maybe in later PR we can make this more generic such that the form can configure all fields of CreateInvoiceRequest.

@NicolasDorier
Copy link
Member

I made a few minor observations, after it seems good to be merged.

@NicolasDorier NicolasDorier merged commit bbbaacc into btcpayserver:master Feb 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Add field validation for Shipping address
3 participants