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

Point of Sale Refactoring #1605

Merged
merged 8 commits into from May 30, 2020

Conversation

dennisreimann
Copy link
Member

@dennisreimann dennisreimann commented May 26, 2020

As described in #749 and talked about with @Kukks.

This is mostly UI related as it separates the simple static and cart views.
Replaces the EnableShoppingCart setting with a DefaultView, which can be set to Static or Cart.

Also gives the ability to define the view type via an optional URL parameter [Route("/apps/{appId}/pos/{viewType?}")], falling back to the default view defined in the settings.

These are the things I'd appreciate input on:

  • How to ensure we are backwards compatible for apps that had EnableShoppingCart set?
  • Is there a better way to solve this? Html.GetEnumSelectList<PosViewType>().Where(type => Convert.ToInt32(type.Value) >= 0)

Closes #749.

@dennisreimann dennisreimann requested a review from Kukks May 26, 2020 19:54
@dennisreimann dennisreimann added PoS Point of Sale app UI / UX Front-end issues, for front-end designers labels May 26, 2020
@dennisreimann
Copy link
Member Author

The test failure seems unrelated, can someone retrigger the selenium tests?

OpenQA.Selenium.WebDriverException : Cannot start the driver service on http://localhost:45594/

Copy link
Member

@Kukks Kukks left a comment

Choose a reason for hiding this comment

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

Happy this is finally happening, we can clean up the POs this way.

  • I'm not sure if unspecified should be an option
  • We need to leave EnableShoppingCart for backwards compat so that existing apps dont lose their default view ( users should not be effected when upgrading to the next version)
  • We should migrate the data from EnableShoppingCart to ViewType, when updating. In the customer-facing action methods, we should just do settings.DefaultView = settings.EnableShoppingCart? PosViewType.Cart : settings.DefaultView;
  • We should make the actions load the view type partials directly (i think but not sure)

BTCPayServer/Controllers/AppsPublicController.cs Outdated Show resolved Hide resolved
BTCPayServer/Views/Apps/UpdatePointOfSale.cshtml Outdated Show resolved Hide resolved
BTCPayServer/Views/AppsPublic/ViewPointOfSale.cshtml Outdated Show resolved Hide resolved
BTCPayServer/Controllers/AppsPublicController.cs Outdated Show resolved Hide resolved
BTCPayServer/Controllers/HomeController.cs Outdated Show resolved Hide resolved
@NicolasDorier
Copy link
Member

Build fail

@dennisreimann dennisreimann requested a review from Kukks May 28, 2020 07:51
@dennisreimann dennisreimann requested a review from Kukks May 28, 2020 11:49
@NicolasDorier
Copy link
Member

@pavlenex you want to manually test?

@pavlenex
Copy link
Contributor

pavlenex commented May 29, 2020

tACK. I tested a bit and haven't found any problems.

A note though, do we really want a drop down for enabling cart? It's okay but we'd have to alter documentation as well.

@NicolasDorier NicolasDorier merged commit a709349 into btcpayserver:master May 30, 2020
@dennisreimann dennisreimann deleted the pos-refactoring branch May 30, 2020 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PoS Point of Sale app UI / UX Front-end issues, for front-end designers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pos App refactoring
4 participants