Respect 'using' in signals #200

VaZark · 2022-11-16T14:30:58Z

Related to : #199

fabiocaccamo

Could you add also a test to this PR please?

fabiocaccamo · 2022-11-17T10:03:45Z

tests/test_models.py

+        theme_1 = Theme.objects.create(name="Custom 1", active=True)
+        kwargs = {"db" : "default"}
+        Theme.get_active_theme(**kwargs)
+


This test is ok, but I think it would have passed the tests even before the other changes you made because it uses the default database. Could you add also another that uses another database please?

Just added another test for failure when passing an invalid DB parameter. I'll need to change some of the settings if you'd prefer a multi-db test as well

If you could add a third test for multi-db it would be great!

fabiocaccamo · 2022-11-17T10:06:22Z

@merwok the CI lint step fails unexpectedly while checking the translations, do you know why / could you fix it?

VaZark · 2022-11-17T10:44:39Z

None of my changes touch anything regarding translations so I'm not sure where it arises. Do you have an idea what could be the cause? That'll help me start digging

fabiocaccamo · 2022-11-17T10:46:25Z

tests/test_models.py

+        theme_1 = Theme.objects.create(name="Custom 1", active=True)
+        kwargs = {"db" : "default"}
+        Theme.get_active_theme(**kwargs)
+


If you could add a third test for multi-db it would be great!

fabiocaccamo · 2022-11-17T14:16:10Z

@VaZark please update this branch from master, I have updated the test workflow for running tests even if the lint step fails.

for more information, see https://pre-commit.ci

tests/test_models.py

.vscode/launch.json

fabiocaccamo · 2022-11-18T08:53:45Z

admin_interface/models.py

-        Theme.get_active_theme()
+            objs_manager = Theme.objects
+            if "using" in kwargs:
+                objs_manager = objs_manager.using(kwargs["using"])


This condition could be avoided by just doing:

objs_database = kwargs.get("using", "default") objs_manager = Theme.objects.using(objs_database)

Since I'll be replacing *args, **kwargs could be replaced by database="default", that can replace the kwargs.get`. Will still be removing the if check.

fabiocaccamo · 2022-11-18T08:58:29Z

admin_interface/models.py

@@ -42,8 +46,10 @@ def pre_save_handler(instance, **kwargs):
                pass

    @staticmethod
-    def get_active_theme():
+    def get_active_theme(*args, **kwargs):


We don't need *args here, and **kwargs could be replaced by database="default".

okay. I'll update all the references in the model to use database

tests/settings.py

fabiocaccamo · 2022-11-18T09:10:15Z

None of my changes touch anything regarding translations so I'm not sure where it arises. Do you have an idea what could be the cause? That'll help me start digging

No, I don't understand the cause, for this reason I updated the test workflow.
If the PR passed all tests except the lint step I will merge it anyway.

codecov · 2022-11-18T09:13:37Z

Codecov Report

Base: 95.55% // Head: 95.58% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (cc8ba71) compared to base (7c9340b).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #200      +/-   ##
==========================================
+ Coverage   95.55%   95.58%   +0.03%     
==========================================
  Files          36       36              
  Lines         405      408       +3     
==========================================
+ Hits          387      390       +3     
  Misses         18       18

Flag	Coverage Δ
unittests	`95.58% <100.00%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
admin_interface/models.py	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

fabiocaccamo · 2022-11-18T09:29:40Z

@VaZark tests are broken with postgres.

tests/settings.py

for more information, see https://pre-commit.ci

VaZark · 2022-11-18T10:47:19Z

The pg tests were broken due to HOST and port not being set for github_workflow. It should be good now

fabiocaccamo · 2022-11-18T11:16:03Z

tests/test_models.py

+
+    @classmethod
+    def setUpTestData(cls):
+        Theme.objects.using("default").create(name="Change Active", active=True)


using("default") could be omitted here.

merwok · 2022-11-18T16:57:04Z

Great changes here!

The reason for the lint failure can be seen in the CI output: translation files include line numbers for extracted strings, and adding new code changed the line numbers. My preference is to include filenames but not line numbers in PO files (using makemessages --add-location=file).

fabiocaccamo · 2022-11-18T18:13:07Z

@merwok thank you very much for the feedback!

fabiocaccamo · 2022-11-18T18:13:33Z

@VaZark thank you very much for the effort and the good PR!

VaZark · 2022-11-18T22:28:12Z

Thanks @fabiocaccamo ! Glad that this was my first-ever PR

fabiocaccamo requested changes Nov 16, 2022

View reviewed changes

fabiocaccamo requested changes Nov 17, 2022

View reviewed changes

VaZark and others added 8 commits November 17, 2022 16:04

Respect 'using' in signals

af32447

[pre-commit.ci] auto fixes from pre-commit.com hooks

913791c

for more information, see https://pre-commit.ci

Add test to respect using in kwargs

2d49fd5

[pre-commit.ci] auto fixes from pre-commit.com hooks

932f461

for more information, see https://pre-commit.ci

Add failure test for invalid DB

098258a

[pre-commit.ci] auto fixes from pre-commit.com hooks

461da0f

for more information, see https://pre-commit.ci

WIP : postgres test is failing

47c8806

[pre-commit.ci] auto fixes from pre-commit.com hooks

1eb7b97

for more information, see https://pre-commit.ci

merwok reviewed Nov 17, 2022

View reviewed changes

tests/test_models.py Show resolved Hide resolved

remove unused import

69ccde8

merwok reviewed Nov 17, 2022

View reviewed changes

tests/test_models.py Outdated Show resolved Hide resolved

merwok reviewed Nov 17, 2022

View reviewed changes

.vscode/launch.json Outdated Show resolved Hide resolved

fabiocaccamo requested changes Nov 18, 2022

View reviewed changes

clean up kwargs and update gitignore

3c81ef8

fabiocaccamo approved these changes Nov 18, 2022

View reviewed changes

fabiocaccamo requested changes Nov 18, 2022

View reviewed changes

tests/settings.py Show resolved Hide resolved

VaZark and others added 5 commits November 18, 2022 11:17

setup test data once

d2841d4

rename db replica

5a738ff

[pre-commit.ci] auto fixes from pre-commit.com hooks

dbaf2d8

for more information, see https://pre-commit.ci

reuse same db for replica

e2e2ee4

Update tests

5bd6362

Potential fix for pg test

cc8ba71

fabiocaccamo reviewed Nov 18, 2022

View reviewed changes

fabiocaccamo approved these changes Nov 18, 2022

View reviewed changes

fabiocaccamo merged commit 946b9c9 into fabiocaccamo:master Nov 18, 2022

VaZark deleted the respect-signal-db branch November 18, 2022 12:13

Uh oh!

Respect 'using' in signals #200

Respect 'using' in signals #200

Uh oh!

Conversation

VaZark commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fabiocaccamo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VaZark Nov 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabiocaccamo commented Nov 17, 2022

Uh oh!

VaZark commented Nov 17, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabiocaccamo commented Nov 17, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fabiocaccamo commented Nov 18, 2022

Uh oh!

codecov bot commented Nov 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

fabiocaccamo commented Nov 18, 2022

Uh oh!

Uh oh!

VaZark commented Nov 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

merwok commented Nov 18, 2022

Uh oh!

fabiocaccamo commented Nov 18, 2022

Uh oh!

fabiocaccamo commented Nov 18, 2022

Uh oh!

VaZark commented Nov 18, 2022

Uh oh!

Uh oh!

VaZark commented Nov 16, 2022 •

edited

Loading

fabiocaccamo left a comment •

edited

Loading

VaZark Nov 17, 2022 •

edited

Loading

codecov bot commented Nov 18, 2022 •

edited

Loading

VaZark commented Nov 18, 2022 •

edited

Loading