84 track orders #137

michkoz · 2016-10-27T17:47:40Z

Major changes:

Payments are connected to the users not to the accounts. Account field kept for the future use.
Payment has payment items. Payment items have hard copy of the products, since ID is not enough. Product prices can change for example etc.
User can see all orders (For not it is pin to the navigation bar without any permission, change it as you want) , also a specific order
User can successfully checkout an order with option pay cash/mobile at the pick-up location.
Removed button for Gedpaid functionality - As fas as I know it didn't work

Should be fixed at some point:

The addling/removing items should not check the stock, it should be part of the checkout. As it is now when I add an item to basket and logout. The item is blocked in my basket forever. Also Add method in the model should check for the currency.
Payment model should not have field "amount to be paid". This should be a sum of payment items. It should rather have field "user paid this amount". This would allow to remove an item from the order. (And see how much to refund to the user).

What is not done:

Payment confirmation form for the eggplant administrators. (For now it is possible to confirm payment in the admin console)
No order confirmation email

Please feel free to change anything in my code!

coveralls · 2016-10-27T17:49:17Z

Coverage decreased (-0.4%) to 73.406% when pulling 6c7f143 on michkoz:84-track-orders into 9e92455 on kbhff:master.

coveralls · 2016-10-27T17:49:26Z

Coverage decreased (-0.4%) to 73.406% when pulling 6c7f143 on michkoz:84-track-orders into 9e92455 on kbhff:master.

benjaoming · 2016-10-29T08:44:54Z

Wow! Great entrance, @michkoz ! I can review this today I think, but it should also be reviewed by at least one more person so we have a somewhat distributed understanding of the changes introduced!? Let's try pinging people to see if they're game for some review action @valberg @jacobandresen @kraenhansen @sepow :)

sepow · 2016-10-30T11:15:08Z

I've skimmed it and will have time to take a deeper look tomorrow. The only thing that really jumped out to me was the change from current.update() to current.save().

benjaoming

A couple of small things, hoping you have time to look at it :)

benjaoming · 2016-10-30T12:51:02Z

eggplant/market/migrations/0008_auto_20161027_1706.py

+class Migration(migrations.Migration):
+
+    dependencies = [
+        migrations.swappable_dependency(settings.AUTH_USER_MODEL),


oh nice, did you add this manually?

This was done autocratically

benjaoming · 2016-10-30T12:52:00Z

eggplant/market/models/cart.py

+from .payment import Payment, PaymentItem
+from django.db.models import Sum, F, Count, Max
+from django.core.exceptions import ValidationError
+from moneyed import Money


The sorting is off, try doing this:

pip install pre-commit pre-commit install pre-commit run --all-files

My bad, I expected that it works as pre commit hook.

When installed, it will fire, but you have to run pre-commit install, after that it lints changed files before committing.

benjaoming · 2016-10-30T12:55:24Z

eggplant/market/models/cart.py

+        # sanity check for the currencies
+
+        if 1 != self.items.all().values('product__price_currency').annotate(total=Count('product__price_currency')).count():
+            raise ValidationError('The products have different currencies')


If the count() is 0, the ValidationError won't make sense. But it might as well generate an exception below.

You are right that it should be fired another place.

benjaoming · 2016-10-30T13:13:43Z

eggplant/market/models/cart.py

+            raise ValidationError('The products have different currencies')
+
+        total = self.items.all().aggregate(total=Sum(F('product__price') * F('quantity')), cur=Max('product__price_currency'))
+        return Money(total['total'], total['cur'])


This doesn't work for me, I get the following:

site-packages/django/db/models/expressions.py in _resolve_output_field(self) 268 if source is not None and not isinstance(self._output_field, source.__class__): 269 raise FieldError( --> 270 "Expression contains mixed types. You must set output_field") 271 272 def convert_value(self, value, expression, connection, context): FieldError: Expression contains mixed types. You must set output_field

I think it has to do with the F('product__price') * F('quantity') expression.

I'm guessing it somehow works for you? Are you running Python 3.4 or 3.5?

Strange, I tested it on sqlite and it worked. It looks like a difference between psql and sqlite in types. I will rewrite it as a loop. With pre-fetching and selecting related data the performance would be almost the same, more readable and work on every database engine.

Well it works for me, and it generates a correct query:
SELECT MAX("market_product"."price_currency") AS "cur", CAST(SUM(("market_product"."price" * "market_basketitem"."quantity")) AS NUMERIC) AS "total" FROM "market_basketitem" INNER JOIN "market_product" ON ("market_basketitem"."product_id" = "market_product"."id") WHERE "market_basketitem"."basket_id" = '19

And I use python 3.4.5

Yes, there shouldn't be any real performance concerns as the contents of a basket are quite limited :)

benjaoming · 2016-10-30T13:14:56Z

eggplant/market/models/cart.py

            amount=self.get_total_amount(),
+            # FIXME: get valid account


This means that we don't know the account that pays at the moment of checking out and creating the payment? Maybe if we start by assuming users always have exactly 1 account and always use that?

Well in ideal case: a user should be created with a personal account (better name would be organization). It should be a trigger and account model should have one more field - personal.
So every user has own default account. Later on the user can join other non personal accounts.

And somewhere in navigation bar he can select which account is he using (probably a session variable). Basket should be based on account_id. This would allow to share the basket for the whole organization. Anyway it has nothing to do with what I have done.

Sounds interesting to add a default_account fk and make it a personal account - so we can guarantee that a user is always associated to at least 1 account.

It would add more consistency to the shopping logic. Everything is based on the account. It it actually quite nice task and it shouldn’t be very difficult. Only someone has to decide where the selector for the active account will be. If someone less experience decide to do, I'm happy to assist him/her.

When we talk about shopping logic the payment model it is not optimal as well. It should be called an order. Order can have many payments and many orderItems. As it is now it has many limitation for real-life situations.

Are you suggesting a separate issue for this? I can create the issue and try to describe it...

Yeah the issue is quite complex and has noting to do with the payment the logic.

The issue could have following sub-task:

Add personal field to the account.

Add trigger when user is created that an account with personal field set to true. User can have only one personal account.

Create personal accounts for all existing users.

Create drop down where all users accounts are listed. It can be called for example `Active account'. And user can choose which account he is using for shopping. Of course if he has only one account no drop down is visible.

The active account ID should be added to session as variable. Or is there any other way how to know what account is the user using? When user login in he shops as personal/last used account.

Link to the payments:

Remove user field from basket and add account

Order history should be filtered by account id not user id

Payment should keep the user field - it is important to know who made the checkout/payment.

Also #130 should be removed - it doesn’t solve the problem. It is up to consideration if this should be part of the MVP.

What do you think? Also the issues should be revisited.

I don't think the account needs to be a personal account, as long as all users have at least 1 default_account, then that account can be deleted or shared to other users at a later time. The important part is only that we automatically create a new "personal" account every time a new user is created.

I'll create a new issue, thanks for fleshing it out!!

Moved to #139

benjaoming · 2016-10-30T13:16:40Z

eggplant/market/templates/eggplant/market/checkout.html

+		to a secure page at ePay.dk, where you will enter your payment information
+		and accept the payment.</p>
+{% comment %}
+		<p>We accept the following payment methods: Dankort, Visa/Dankort.</p>


Fine to just delete this :)

benjaoming · 2016-10-30T13:18:14Z

eggplant/market/templates/eggplant/market/payment_info.html

+
+<div class="row">
+	<div class="col-sm-12">
+		<strong>Your order Summary:</strong>


Mark for translation! :)

benjaoming · 2016-10-30T13:18:19Z

eggplant/market/templates/eggplant/market/payment_info.html

+	            <th>Order id</th>
+	            <th>Order date</th>
+	            <th>payment status</th>
+	            <th>total amount</th>


Mark for translation! :)

benjaoming · 2016-10-30T13:18:28Z

eggplant/market/templates/eggplant/market/payment_info.html

+</div>
+{% else %}
+	<div class="row">
+		<div class="col-md-5 col-span-4">Wrong order</div>


Mark for translation! :)

benjaoming · 2016-10-30T13:18:54Z

eggplant/market/templates/eggplant/market/payment_list.html

@@ -4,40 +4,69 @@

 <div class="row">
 	<div class="col-sm-12">
-		<h1 class="page-header">Your payments:</h1>


This file also needs to be marked for translation! :)

…tions

…into 84-track-orders

coveralls · 2016-10-30T18:08:14Z

Coverage decreased (-0.5%) to 73.263% when pulling b8113f7 on michkoz:84-track-orders into 9e92455 on kbhff:master.

michkoz · 2016-10-30T18:14:50Z

I had to force fix eggplant/market/views/payment.py:58:44: F821 undefined name 'request'. It is part of the gedpaid code what is not working. The code shouldn’t be called anywhere in the app.

benjaoming

A couple of small things, not sure they need to be addressed, I leave it to your judgement, great work @michkoz !

benjaoming · 2016-10-31T15:11:24Z

eggplant/market/models/cart.py

-        # FIXME this check should be removed, it is better to check for the currenices when an item is added
-        # sanity check for the currencies
+        if self.get_items_count() == 0:
+            return Money(0, getattr(settings, 'DEFAULT_CURRENCY'))


benjaoming · 2016-10-31T15:13:18Z

eggplant/market/tests.py

        total = self.test_basket.get_total_amount()

        self.assertEqual(total, Money(Decimal('40'), 'DKK'))

        with self.assertRaisesRegex(ValidationError, 'The products have different currencies'):
            self.test_basket.add_to_items(product=self.test_product_2, quantity=2, delivery_date=None)
            self.test_basket.get_total_amount()
+
+    def test_backet_get_currency(self):


little typo in "backet" :)

benjaoming · 2016-10-31T15:14:25Z

eggplant/market/views/payment.py

@@ -55,6 +55,8 @@ def dispatch(self, request, *args, **kwargs):
        return super(PaymentView, self).dispatch(request, *args, **kwargs)

    def get_queryset(self):
+        # FIX ME: add proper request
+        request = None
        return Payment.objects.filter(user=request.user)


You should be able to use self.request here?

coveralls · 2016-10-31T15:31:26Z

Coverage decreased (-0.4%) to 73.349% when pulling 9499ff5 on michkoz:84-track-orders into 9e92455 on kbhff:master.

Mikoz2 and others added 10 commits October 22, 2016 21:55

Fixed basket.get_total_amount+ added referece to the basket in payment

a23c18b

Merge branch 'master' into 84-track-orders

7fb4863

only available items are shown on the market

ef79250

Fixed button align/button text/added total amount at the checkout page

ce0cdbf

added the basket total price at the market home

e402461

add fixme comment to cart.checkout

6b07d2e

Added list of orders

6446078

The order history is pageable. Added Pay cash Method

6ccd627

Fix get_total_amount returns Money field

ef54ea7

Merge branch 'master' into 84-track-orders

6c7f143

benjaoming suggested changes Oct 30, 2016

View reviewed changes

benjaoming mentioned this pull request Oct 30, 2016

Members can order goods #130

Closed

1 task

Mikoz2 added 2 commits October 30, 2016 19:05

Fix basket.get_total_amount, sanity checks for the basket model opera…

ba5daba

…tions

Merge branch '84-track-orders' of https://github.com/michkoz/eggplant …

b8113f7

…into 84-track-orders

benjaoming approved these changes Oct 31, 2016

View reviewed changes

typo fix

9499ff5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

84 track orders #137

84 track orders #137

michkoz commented Oct 27, 2016

coveralls commented Oct 27, 2016

coveralls commented Oct 27, 2016

benjaoming commented Oct 29, 2016

sepow commented Oct 30, 2016

benjaoming left a comment

benjaoming Oct 30, 2016

michkoz Oct 30, 2016

benjaoming Oct 30, 2016

michkoz Oct 30, 2016

benjaoming Oct 30, 2016

benjaoming Oct 30, 2016

benjaoming Oct 30, 2016

michkoz Oct 30, 2016

benjaoming Oct 30, 2016

benjaoming Oct 30, 2016

michkoz Oct 30, 2016

benjaoming Oct 30, 2016

michkoz Oct 30, 2016

benjaoming Oct 30, 2016

michkoz Oct 30, 2016

benjaoming Oct 30, 2016

benjaoming Oct 30, 2016

benjaoming Oct 30, 2016

benjaoming Oct 30, 2016

benjaoming Oct 30, 2016

benjaoming Oct 30, 2016

benjaoming Oct 30, 2016

coveralls commented Oct 30, 2016

michkoz commented Oct 30, 2016

benjaoming left a comment

benjaoming Oct 31, 2016

benjaoming Oct 31, 2016

benjaoming Oct 31, 2016

coveralls commented Oct 31, 2016

84 track orders #137

Are you sure you want to change the base?

84 track orders #137

Conversation

michkoz commented Oct 27, 2016

coveralls commented Oct 27, 2016

coveralls commented Oct 27, 2016

benjaoming commented Oct 29, 2016

sepow commented Oct 30, 2016

benjaoming left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 30, 2016

michkoz commented Oct 30, 2016

benjaoming left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 31, 2016