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

Create shop orders #272

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

Create shop orders #272

wants to merge 8 commits into from

Conversation

MatheusBuss
Copy link
Contributor

What issue does this PR close

Closes #198

Changes Proposed ( a list of new changes introduced by this PR)

Create shop orders abstraction

@MatheusBuss
Copy link
Contributor Author

@lucca65 criei um contexto novo pra orders. Mas acho que deveria ter jogado ele pra dentro do shop mesmo. Acho que fica mais organizado. Daí teria que reconstruir o arquivo lib/cambiatus/shop/order.ex.

@lucca65
Copy link
Member

lucca65 commented Aug 4, 2022

+1 bro, vamos manter td no shop!

@MatheusBuss MatheusBuss self-assigned this Aug 10, 2022
@MatheusBuss MatheusBuss added the ✨ feature A new feature label Aug 10, 2022
Comment on lines +84 to +85
|> join(:left, [o], b in assoc(o, :buyer))
|> where([o, b], b.account == ^buyer.account)
Copy link
Member

Choose a reason for hiding this comment

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

can't we simply preload the buyer? Something like this:

Suggested change
|> join(:left, [o], b in assoc(o, :buyer))
|> where([o, b], b.account == ^buyer.account)
|> Repo.preload(:buyer)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so. Because this is getting an order where the status is cart and belongs to the current user. Since we're getting the order from these SQL statements I don't believe it's possible to preload the buyer.

lib/cambiatus/orders.ex Show resolved Hide resolved
lib/cambiatus/orders/item.ex Outdated Show resolved Hide resolved
Comment on lines 37 to 39
with order_id <- get_field(changeset, :id),
{:ok, order} <- Orders.get_order(order_id) do
if Map.get(order, :status) == "cart" and get_field(changeset, :status) == "checkout" do
Copy link
Member

Choose a reason for hiding this comment

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

This could be piped

Suggested change
with order_id <- get_field(changeset, :id),
{:ok, order} <- Orders.get_order(order_id) do
if Map.get(order, :status) == "cart" and get_field(changeset, :status) == "checkout" do
order_status =
changeset
|> get_field(:id)
|> Orders.get()
|> Map.get(:status)
if Enum.member(["cart", "checkout"], order_status) do
. . .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intent of this one is to check if the order stored in the db has the status cart and the incoming changeset is changing this status to checkout. Then we should make some more validations to ensure that everything is valid before the user is has the chance to make a payment.

This has also changed a bit. If you want to make another review it'll be very welcome.

Copy link
Member

Choose a reason for hiding this comment

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

This cart I've added a few comments about state machines, but we should leverage its structure to make sure its valid, instead of relying on manual verification.

If an order is on a certain state, we should trust our backend ingested it properly so its correct to have this state. This is an old article and probably outdated, but it contain precious concepts about this type of abstraction: https://medium.com/@joaomdmoura/state-machine-in-elixir-with-machinery-8ee6f9def2da

@lucca65
Copy link
Member

lucca65 commented Aug 12, 2022

I know its a draft, some general directions 😄

)

field(:total, :float)
field(:status, :string)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we need to review possible states bro. Can you implement this as an Ecto.Enum? This way we can talk more about control flow and state changes. I recommend you take a look at state machines, as this will be a nice way to think about this abstractions. A gist of it could be something like

states: created, processed, approved, completed
actions: new_draft (makes a new one), approve (createdapproved or processedapproved, pay (no state change), archive (any → completed

Copy link
Member

Choose a reason for hiding this comment

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

this is just an example on how to approach an order state as its heavily implied that it will be treated different depending on it. This means we will have specific actions that will change state. Makes easier to understand and admin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about elaborating states. If you want we can have a more thorough discussion about states on a meeting of some kind, maybe even with other teams to get the right flow.

But I was thinking something like this:
1 - Cart: There's no more than one order with this status for each user
2 - Checkout:Validate everything that was put on the cart, since availability may have changed since adding to cart
3 - Awaiting payment confirmation: This should be a brief state, just to ensure that the items on checkout have been payed for but we're making sure that everything went through
4 - Delivering: This could change name or encompass other steps. Some orders may need preparation after payment but before beign archive/completed
5 - Completed: End step for every order, maybe there should be another end step if something goes wrong along the way

Copy link
Member

Choose a reason for hiding this comment

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

yeah, you are totally right, nailing the states will go a long way for us, migrating types is a pain in the ass so the better we design our states, the best it is.

I haven't thought about adding cart as an state, this is a good idea. I would add other auxiliary states as well. Cancelled by vendor, cancelled by buyer (maybe by not paying during the accepted window?) payment denied and finally payment incomplete (this is a must as the main motivation to building orders is to allow multiple payments on the same order)

+1 on scheduling a meeting. We should do it between ourselves first to build a first draft and then present it to the rest of the team to gather feedback


add(:total, :float, comment: "Order total")

# TODO: Elaborate shipping and status
Copy link
Member

Choose a reason for hiding this comment

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

Shipping will be an entity that we will attach on later, we can do a preview of states but don't worry about tables/logic for shipping

use Ecto.Migration

def change do
rename(table(:orders), to: table(:items))
Copy link
Member

Choose a reason for hiding this comment

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

why items?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since every order until now has been a single product purchase it is probably easier to treat them as items, since most of the information is being contained within the items table. Then we can create the related orders with some info on the buyer.

We can always do it the other way as well, shouldn't be too different.

Copy link
Member

@lucca65 lucca65 Aug 16, 2022

Choose a reason for hiding this comment

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

hmm, Idk. We will still need to major abstraction of orders, which is the main one, that can start with a product_id as a reference to the product and later on can be migrate into an intermediary table order_product_id.

About the majority of the information, it should be on orders, not on items.

The order table could be scoped like this:

  • id
  • product_id
  • from
  • to
  • amount
  • units
  • community
  • blockchainstuff

We could refactor it like this:

  • id
  • product_id (from can be inferred from product, the same with community)
  • buyer
  • amount (can be different from product due to discounts cupons and other stuff so we should keep it)
  • units
  • timestamps

Later we could trim it even further by adding a middle table

Order

  • id
  • buyer
  • timestamps

Order_items

  • order_id (Primary key)
  • product_id (Primary key)

Order_payments

  • id
  • type (enum with something like btc/paypal/venmo/eth/etc)
  • status (created/confirmed/rejected/etc)
  • amount
  • price_modifier_id (maybe a certain payment method is cheaper or more expensive than others. Crypto has no fees, as PayPal do)

Then we could go to other attached tables order_shipping, order_refunds, etc

We should try to go for the higher level abstraction first, then add the auxiliary items. It will make our migration harder, but we can go in small steps as described in the beginning of the comment

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

Successfully merging this pull request may close these issues.

Create Shop orders 🧽
3 participants