Cash register tracking #18

ElinSwedin · 2017-02-17T15:45:13Z

No description provided.

codecov · 2017-02-17T16:22:56Z

Codecov Report

Merging #18 into develop will increase coverage by 0.89%.
The diff coverage is 97.19%.

@@             Coverage Diff             @@
##           develop      #18      +/-   ##
===========================================
+ Coverage    86.79%   87.69%   +0.89%     
===========================================
  Files           71       73       +2     
  Lines         2204     2381     +177     
  Branches       114      125      +11     
===========================================
+ Hits          1913     2088     +175     
  Misses         275      275              
- Partials        16       18       +2

Impacted Files	Coverage Δ
src/foobar/urls.py	`60% <ø> (ø)`	✅
src/wallet/api.py	`100% <100%> (ø)`	✅
src/foobar/wallet/api.py	`100% <100%> (ø)`	✅
src/foobar/api.py	`95.23% <100%> (+1.29%)`	✅
src/foobar/tests/test_api.py	`100% <100%> (ø)`	✅
src/foobar/enums.py	`100% <100%> (ø)`	✅
src/foobar/forms.py	`100% <100%> (ø)`
src/wallet/tests/test_api.py	`100% <100%> (ø)`	✅
src/foobar/tests/test_views.py	`100% <100%> (ø)`
src/foobar/admin.py	`60.74% <77.77%> (+1.21%)`	✅
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ab2048...c844b5a. Read the comment docs.

flaeppe · 2017-02-18T11:48:54Z

WalletCorrection model could have a clean(self) function to check if amount == 0 otherwise it is not considered a correction?

kjagiello · 2017-02-17T15:59:57Z

src/wallet/api.py

@@ -24,6 +24,16 @@ def get_balance(owner_id, currency):
    return wallet_obj, wallet_obj.balance


+def set_balance(owner_id, new_balance, reference=None):


This function should probably be decorated with @transaction.atomic. We want to avoid possible race conditions (no other wallet transactions should be able to sneak in between get_balance and withdraw/deposit).

kjagiello · 2017-02-17T16:03:18Z

src/foobar/admin.py

+    def has_add_permission(self, request):
+        return False
+
+    def get_form(self, request, obj=None, **kwargs):


Debug code? Should be cleaned up.

kjagiello · 2017-02-17T16:05:24Z

src/foobar/api.py

+            amount=-amount,
+            reference=reference
+        )
+        trx = 2


No hardcoded values. Should be replaced with fobar.enums.TrxType.WITHDRAW. The same applies to the other occurrences of hardcoded values in this function.

kjagiello · 2017-02-17T16:06:41Z

src/foobar/forms.py

+        data = self.cleaned_data['deposit_or_withdraw']
+        wallet, balance = api.get_balance(self.owner_id)
+        if data.amount < 0 and -data > balance:
+            raise forms.ValidationError("Not enough founds")


The string should be translated using following method: https://docs.djangoproject.com/en/1.10/topics/i18n/translation/#internationalization-in-python-code. Also, the convention in our codebase is to use single quotes for strings.

founds is misspelled, should be funds

kjagiello · 2017-02-17T18:17:32Z

src/foobar/models.py

@@ -86,3 +86,24 @@ def total(self):

    def __str__(self):
        return str(self.id)
+
+
+class WalletCorrection(UUIDModel, TimeStampedModel):


The name could probably be improved. What are we representing here actually? It seems that it can be both either correction or withdrawal/deposit. The name should reflect that.

kjagiello · 2017-02-17T18:19:02Z

src/foobar/templates/admin/wallet_management.html

+                    	{% csrf_token %} 
+                    	{{ form_class.as_p }}
+                        </div>
+                    	<input type="submit" name="save_correction"  action="" id="correction_save" style="float:right">


What function does "action" have on the input field?

kjagiello · 2017-02-17T18:26:32Z

src/foobar/tests/test_api.py

+        )
+        self.assertEqual(correction_obj.wallet.owner_id, wallet_obj.owner_id)
+        self.assertEqual(correction_obj.trx_type.value, 0)
+        self.assertEqual(correction_obj.post_balance.amount, 1000)


Is this really right? You set the balance of the wallet to 1200 in your call on row 211, but the post_balance is 1000. Shouldn't be 1200?

kjagiello · 2017-02-17T18:28:38Z

src/foobar/tests/test_api.py

+         )
+        self.assertEqual(correction_obj.wallet.owner_id, wallet_obj.owner_id)
+        self.assertEqual(correction_obj.trx_type.value, 1)
+        self.assertEqual(correction_obj.post_balance.amount, 1000)


Your initial balance is 1000 SEK. You deposit 100 SEK. The post_balance should be 1100 SEK.

kjagiello · 2017-02-17T18:29:21Z

src/foobar/tests/test_api.py

+            owner_id=wallet_obj.owner_id
+         )
+        self.assertEqual(correction_obj.wallet.owner_id, wallet_obj.owner_id)
+        self.assertEqual(correction_obj.trx_type.value, 1)


Those should not be hardcoded. Use enums.TrxType.DEPOSIT instead,

flaeppe · 2017-02-18T12:23:04Z

src/foobar/enums.py

+class TrxType(enum.Enum):
+    CORRECTION = 0
+    DEPOSIT = 1
+    WITHDRAW = 2


If we follow @kjagiello's idea of changing the name for WalletCorrection the name here might be appropriate to change as well.

WITHDRAW should probably be changed too from being a verb to a noun: WITHDRAWAL.

flaeppe · 2017-02-18T12:24:56Z

src/foobar/migrations/0017_walletcorrection.py

+import uuid
+
+
+class Migration(migrations.Migration):


What happens with old transactions that exist in the system?

I assume that each new transaction will generate a Correction/Log but should/can we generate an instance for the old ones?

There are no old correction transactions, so no data migration is needed.

kjagiello · 2017-02-18T13:21:23Z

src/foobar/api.py

+
+
+@transaction.atomic
+def calculate_correction(new_balance, owner_id, user, reference=None):


Hmm the reference should be saved only in WalletCorrection. Then WalletCorrect.id should be saved as reference in WalletTransaction. That way, WalletTransactions can be traced back to specific WalletCorrections.

kjagiello · 2017-02-18T15:07:01Z

The code is not compatible with Django 1.9 anymore, but I don't think it is an issue. I've got some changes in workings that are not compatible with Django 1.9 and as we are the only ones using this project at the moment, I propose we drop the 1.9 compatibility.

codecov-io · 2017-02-20T13:36:16Z

Codecov Report

Merging #18 into develop will increase coverage by 0.83%.
The diff coverage is 95.85%.

@@             Coverage Diff             @@
##           develop      #18      +/-   ##
===========================================
+ Coverage    86.88%   87.71%   +0.83%     
===========================================
  Files           71       73       +2     
  Lines         2218     2410     +192     
  Branches       115      126      +11     
===========================================
+ Hits          1927     2114     +187     
- Misses         275      278       +3     
- Partials        16       18       +2

Impacted Files	Coverage Δ
src/foobar/urls.py	`60% <ø> (ø)`	✅
src/foobar/forms.py	`100% <100%> (ø)`
src/wallet/api.py	`100% <100%> (ø)`	✅
src/wallet/tests/test_api.py	`100% <100%> (ø)`	✅
src/foobar/wallet/api.py	`100% <100%> (ø)`	✅
src/foobar/tests/test_views.py	`100% <100%> (ø)`
src/foobar/api.py	`95.4% <100%> (+1.46%)`	✅
src/foobar/tests/test_api.py	`100% <100%> (ø)`	✅
src/foobar/enums.py	`100% <100%> (ø)`	✅
src/foobar/admin.py	`60.14% <66.66%> (+0.62%)`	✅
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a663d96...9d01b87. Read the comment docs.

kjagiello · 2017-02-20T14:00:22Z

src/foobar/admin.py

+                    'pre_balance'
+                    )
+    ordering = ('-date_created',)
+


You should add a Meta-class here and specify verbose_name and verbose_name_plural.

kjagiello · 2017-02-20T14:04:15Z

src/foobar/admin.py

@@ -192,6 +192,25 @@ def changelist_view(self, request, extra_context=None):
        return response


+@admin.register(models.WalletLogEntry)
+class WalletLogEntryAdmin(admin.ModelAdmin):
+    list_display = ('date_created',


We should reorder the columns so it is old balance, amount and then new balance.

kjagiello · 2017-02-20T14:05:15Z

src/foobar/admin.py

@@ -192,6 +192,25 @@ def changelist_view(self, request, extra_context=None):
        return response


+@admin.register(models.WalletLogEntry)


Everything here should be read-only. Also, nobody should be able to delete log entries.

…anged so wallet_correction ID is saved in wallet transaction as reference

flaeppe · 2017-02-20T20:52:33Z

src/wallet/api.py

@@ -29,10 +29,10 @@ def set_balance(owner_id, new_balance, reference=None):
    wallet, old_balance = get_balance(owner_id, new_balance.currency)
    difference = new_balance - old_balance
    if difference.amount < 0:
-        withdraw(owner_id, -difference, reference)
+        return withdraw(owner_id, -difference, reference), difference


Second return value should be -difference to become identical to the object's amount

It is actually correct.

Never mind. My mistake.

It is okay. No problem.

flaeppe · 2017-02-20T21:08:06Z

src/foobar/admin.py

                    )
+    readonly_fields = list_display


Maybe we should keep the comment field writeable? To avoid cluttering the timeline by forcing any spelling mistakes or similar to updated by creating a new log entry.

Good idea 👍🏻

kjagiello · 2017-02-20T21:13:04Z

It would be really nice to get some colors there. At a glance, you don't really see which entries are losses and which are "gains".

…redit wallet and added colors to wallet log entry change list

kjagiello · 2017-02-21T14:34:14Z

src/foobar/forms.py

+
+class CorrectionForm(forms.Form):
+    balance = MoneyField(label='Balance', min_value=0)
+    comment = forms.CharField(label='Comment', max_length=250, required=False)


The max length of the comment field should reflect the max length of the corresponding database field.

kjagiello · 2017-02-21T14:40:45Z

src/foobar/admin.py

+                    'amount',
+                    'post_balance'
+                    )
+    readonly_fields = list_display


The wallet field is not readonly which means someone could mess with us and attach a log entry to some other wallet. We should either make it read only or ideally simple hide it from the form.

kjagiello · 2017-02-21T15:21:52Z

LGTM 👍

flaeppe · 2017-02-21T15:48:55Z

src/foobar/models.py

@@ -92,7 +92,7 @@ class WalletLogEntry(UUIDModel, TimeStampedModel):
    user = models.ForeignKey(User, null=True, blank=True)
    trx_type = EnumIntegerField(enums.TrxType,
                                default=enums.TrxType.CORRECTION)
-    wallet = models.ForeignKey('wallet.Wallet', related_name='log_entries')
+    wallet = models.ForeignKey('wallet.Wallet', related_name='log_entries', editable=False)


Just a heads up, because settings editable=False should trigger a migration. In case it does, we are missing a migration file, test running python manage.py makemigrations to see if it generates a new migration file.

For the admin panel, if you haven't figured it out already, the same effect as setting editable=False here would be applied by including the field in the ModelAdmins readonly_fields.

ElinSwedin added 2 commits February 17, 2017 16:38

Update cash wallet

8ebf2f8

added cash correction and deposits

3f9e68d

kjagiello requested review from kjagiello and flaeppe February 17, 2017 15:46

kjagiello added this to the 2.0.0 milestone Feb 17, 2017

added migration

3e4dc3c

Added tests for view

563ef1d

kjagiello reviewed Feb 18, 2017

View reviewed changes

flaeppe suggested changes Feb 18, 2017

View reviewed changes

flaeppe reviewed Feb 18, 2017

View reviewed changes

kjagiello reviewed Feb 18, 2017

View reviewed changes

Some of the suggested changes completed

c844b5a

changed model and added verbose names

d3592a5

kjagiello approved these changes Feb 20, 2017

View reviewed changes

kjagiello requested changes Feb 20, 2017

View reviewed changes

kjagiello force-pushed the develop branch from a1fb77a to a663d96 Compare February 20, 2017 16:44

kjagiello added the enhancement label Feb 20, 2017

ElinSwedin added 2 commits February 20, 2017 21:21

Added verbose names for model, changed places of old/new balance + ch…

0d4ef43

…anged so wallet_correction ID is saved in wallet transaction as reference

new migration with added pre_balance to WalletLogEntry

7e3cecf

flaeppe suggested changes Feb 20, 2017

View reviewed changes

flaeppe reviewed Feb 20, 2017

View reviewed changes

added wallet correction to main wallet, changed main wallet name to c…

8a17c91

…redit wallet and added colors to wallet log entry change list

kjagiello approved these changes Feb 21, 2017

View reviewed changes

kjagiello requested changes Feb 21, 2017

View reviewed changes

changed max-length to correspond model max-length

cad3210

kjagiello requested changes Feb 21, 2017

View reviewed changes

ElinSwedin added 2 commits February 21, 2017 16:08

changed so wallet object is hidden and comment is editable

ae2dd21

lint.....

b4e8f97

kjagiello approved these changes Feb 21, 2017

View reviewed changes

added missing migraiton file

9d01b87

flaeppe reviewed Feb 21, 2017

View reviewed changes

flaeppe approved these changes Feb 21, 2017

View reviewed changes

kjagiello merged commit 115b417 into uppsaladatavetare:develop Feb 21, 2017

kjagiello changed the title ~~Feature/cashdeposits~~ Ability to track the state of the cash register Feb 23, 2017

kjagiello changed the title ~~Ability to track the state of the cash register~~ Cash register tracking Feb 23, 2017

ElinSwedin deleted the feature/cashdeposits branch March 2, 2017 13:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cash register tracking #18

Cash register tracking #18

ElinSwedin commented Feb 17, 2017

codecov bot commented Feb 17, 2017 •

edited

Loading

flaeppe commented Feb 18, 2017 •

edited

Loading

kjagiello Feb 17, 2017

kjagiello Feb 17, 2017

kjagiello Feb 17, 2017

kjagiello Feb 17, 2017 •

edited

Loading

flaeppe Feb 18, 2017

kjagiello Feb 17, 2017

kjagiello Feb 17, 2017

kjagiello Feb 17, 2017

kjagiello Feb 17, 2017

kjagiello Feb 17, 2017

flaeppe Feb 18, 2017

kjagiello Feb 18, 2017

flaeppe Feb 18, 2017

kjagiello Feb 18, 2017

kjagiello Feb 18, 2017

kjagiello commented Feb 18, 2017

codecov-io commented Feb 20, 2017 •

edited by codecov bot

Loading

kjagiello Feb 20, 2017

kjagiello Feb 20, 2017

kjagiello Feb 20, 2017

flaeppe Feb 20, 2017

kjagiello Feb 20, 2017

flaeppe Feb 20, 2017

kjagiello Feb 20, 2017

flaeppe Feb 20, 2017

kjagiello Feb 20, 2017

kjagiello commented Feb 20, 2017

kjagiello Feb 21, 2017

kjagiello Feb 21, 2017

kjagiello commented Feb 21, 2017

flaeppe Feb 21, 2017

		@@ -24,6 +24,16 @@ def get_balance(owner_id, currency):
		return wallet_obj, wallet_obj.balance


		def set_balance(owner_id, new_balance, reference=None):



		@transaction.atomic
		def calculate_correction(new_balance, owner_id, user, reference=None):

		@@ -192,6 +192,25 @@ def changelist_view(self, request, extra_context=None):
		return response


		@admin.register(models.WalletLogEntry)

Cash register tracking #18

Cash register tracking #18

Conversation

ElinSwedin commented Feb 17, 2017

codecov bot commented Feb 17, 2017 • edited Loading

Codecov Report

flaeppe commented Feb 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjagiello Feb 17, 2017 • edited Loading

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

kjagiello commented Feb 18, 2017

codecov-io commented Feb 20, 2017 • edited by codecov bot Loading

Codecov Report

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

kjagiello commented Feb 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjagiello commented Feb 21, 2017

Choose a reason for hiding this comment

codecov bot commented Feb 17, 2017 •

edited

Loading

flaeppe commented Feb 18, 2017 •

edited

Loading

kjagiello Feb 17, 2017 •

edited

Loading

codecov-io commented Feb 20, 2017 •

edited by codecov bot

Loading