search

zsoldosp / django-currentuser

Conveniently store reference to request user on thread/db level.
BSD 3-Clause "New" or "Revised" License
141 stars 40 forks source link

CurrentUserField: add support for update on save #37

Closed felubra closed 4 years ago

felubra commented 4 years ago

This implements #36 .

belugame commented 4 years ago

@felubra Hey, thank you very much for your contribution. Did you have a look yet at the failing builds on travis?

felubra commented 4 years ago

Hi @belugame . Not sure why it is failing. I will spin up a local travis to investigate. Thanks.

belugame commented 4 years ago

@felubra Sorry, we don't get around to spend time on this mini-project. Does this affect behavior in any way for existing installations?

felubra commented 4 years ago

@belugame I don't think so, since the user has to explicitly set the new flag on_update=True, so the default behavior is to not use the new feature on existing installations. Additional test coverage was provided to test the scenario with the new flag unset also (see here).

belugame commented 4 years ago

understood. Can we do without the migration file in the testapp?

felubra commented 4 years ago

I've added two models for testing the new feature (TestModelOnUpdate and TestModelDefaultBehavior). The existence of these without the migrations will make the tests fail in Django 2.2, since

Tests will fail on SQLite if apps without migrations have relations to apps with migrations. This has been a documented restriction since migrations were added in Django 1.7, but it fails more reliably now. You’ll see tests failing with errors like no such table: _. This was observed with several third-party apps that had models in tests without migrations. You must add migrations for such models. (Django 2.2 release notes)

In other words, without the migrations the CI job will fail whenever it tries to test Django version 2.2:

This behavior, strange as it may seem, does not happen if we update the build of travis to run on Ubuntu 18.04 (bionic), so I believe the problem is related to the version of the sqlite3 library provided in Ubuntu 16.04.

In any case, this is a problem with testing and a specific version of Django. I’m open for additional suggestions on how we can resolve it.

belugame commented 4 years ago

Thank you very much for the explanation and your work. Sorry again for the tardy replies here, I can understand that that is frustrating.

belugame commented 4 years ago

I just released it as 0.5.0

felubra commented 4 years ago

Thank you very much for the explanation and your work. Sorry again for the tardy replies here, I can understand that that is frustrating.

No problems - your concerns were valid. I'm happy to help, thank you!