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

Improve dummy data generation #139

Open
aldesantis opened this issue Jul 29, 2020 · 4 comments
Open

Improve dummy data generation #139

aldesantis opened this issue Jul 29, 2020 · 4 comments
Labels
enhancement Improves an existing feature. stale

Comments

@aldesantis
Copy link
Member

aldesantis commented Jul 29, 2020

This is related to #138.

It is currently almost impossible to display a subscription's total in a reliable way. The current implementation (SolidusSubscriptions::SubscriptionLineItem#dummy_line_item) has several flaws:

  • It still assumes there's only one line item per subscription, not allowing you to compute the total of an entire subscription.
  • When the line item is tied to an order, it uses the persisted order as the dummy order, with the risk of presenting stale information to the user.
  • When the line item is NOT tied to an order (e.g. because it was added manually by an admin or the customer after the subscription was created), it persists a new order to the DB on every call. This is performance-intensive and pollutes order data.

The biggest architectural problem is that there is no way to reliably compute an order's total if that order is not persisted in the DB. Spree::OrderUpdater not only relies on a lot of information being persisted, but also persists the order after calculating the totals, so it wouldn't work with an ephemeral/frozen order.

The solutions I currently see are:

  1. For each subscription, persist a dummy order to the DB that has the sole purpose of computing the subscription's totals. This has the advantage of computing totals in an extremely reliable way, since we'd use the same APIs Solidus uses during the checkout flow, but the order would have to be updated every time the subscription's details or content change. Furthermore, we'd be polluting the spree_orders table with fake data (although this can probably be mitigated with a custom column and a default scope that causes Spree::Order to ignore it).
  2. Attempt to mimick the calculations performed by Spree::OrderUpdater. I'm not 100% sure this is possible without persisting certain data (e.g. shipments) to the DB, and we'd have to constantly keep our own calculations up-to-date with any changes in the core.
  3. Attempt to use Spree::OrderUpdater with a SolidusSubscriptions::Subscription object instead of a Spree::Order object. We'd have to mock a lot of functionality, but it's potentially doable.
  4. Submit a PR to the core to make order computations more modular. The problem here is that we'd have to wait for the PR to be merged and a new Solidus version to be released before anyone can take advantage of the functionality.

Whatever solution we go with, we should completely disregard the original order that generated a subscription: it's not a reliable source of information about the subscription and all the data we need is already stored on the subscription itself.

@aldesantis aldesantis added the enhancement Improves an existing feature. label Jul 29, 2020
@stale
Copy link

stale bot commented Sep 27, 2020

This issue has been automatically marked as stale because it has not had recent activity. It might be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Sep 27, 2020
@blocknotes
Copy link
Contributor

Hey @aldesantis, I saw this recent commit.
But we are still interested in this feature, right?

From my standpoint, the approach n. 3 could be good in this case in order to avoid the creation of DB entities entirely if possible.

@stale stale bot removed the stale label Feb 15, 2021
@aldesantis
Copy link
Member Author

@blocknotes that's right, we're still interested in doing this, but we want to do it right. 😛

As far as the approach goes, I also think number 3 is the most promising. Let me know if you need anything and good luck!

@stale
Copy link

stale bot commented Apr 18, 2021

This issue has been automatically marked as stale because it has not had recent activity. It might be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improves an existing feature. stale
Projects
None yet
Development

No branches or pull requests

2 participants