Re: admin: automatic link to files for FileFields in change_list breaks link to change_view (v1.8, #14497)

[Florian Demmer]

On 2015-11-22 21:31, Riccardo Magliocchetti wrote:

Hi Florian,

Il 22/11/2015 21:14, Florian Demmer ha scritto:

Hi guys,

i am confused/surprised by this ticket and already merged feature. it 
took me a

while to find out what was happening in my admin:

https://code.djangoproject.com/ticket/14497

- it was merged into 1.8, right? the ticket says 1.2.


That was set at the time of filing the issue supposedly
It's pretty clear it has been applied to master and then backported:
https://code.djangoproject.com/ticket/14497#comment:32


yeah, i know. i found the ticket by finding the code and finding the commit.

i was just wondering if the version should be updated on merge. it would 
be easier to track down changes, when the merge-version is set on the 
ticket, especially when the change is minor and not in the releasenotes.


But i see now, that i misunderstood the field: 
https://docs.djangoproject.com/en/1.8/internals/contributing/triaging-tickets/#version





Both change_list and change_view (for read-only fields) use
`display_for_field()`, that's why both are affected by the change. 
That might

not have been on purpose...


I usually don't break software on purpose :)


did not mean the "breaking" part, just that changing it in one place had 
an effect on both admin views... sorry if it sounded like that!




Imho, the automatically added link to the file should *not* have 
precedence over
`list_display_links` and definitely should *not* break the 
automatically added
link to the change_view, when the `FileField` happens to be first in 
`list_display`.


What do you think?


Since you seem to care about this, any chance to open a pull request? 
The pull request should contain updated tests to verify that the 
double  link is fixed please. If not i'll give a try of fixing it 
later this week.





since both admin views use that same function, i am not sure how easy it 
will be to fix without adding a whole bunch of more code just to detect 
the field type and checking list_display_links etc... i'll probably not 
find the time to look into that until next weekend either.


as a workaround i have added the 'id' field in first place to my 
list_display.


br
Florian


--
You received this message because you are subscribed to the Google Groups "Django 
developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/56522D59.4020907%40gmail.com.
For more options, visit https://groups.google.com/d/optout.

Re: admin: automatic link to files for FileFields in change_list breaks link to change_view (v1.8, #14497)

[Riccardo Magliocchetti]

Hi Florian,

Il 22/11/2015 21:14, Florian Demmer ha scritto:

Hi guys,

i am confused/surprised by this ticket and already merged feature. it took me a
while to find out what was happening in my admin:

https://code.djangoproject.com/ticket/14497

- it was merged into 1.8, right? the ticket says 1.2.


That was set at the time of filing the issue supposedly
It's pretty clear it has been applied to master and then backported:
https://code.djangoproject.com/ticket/14497#comment:32


- there was no documentation in the patch, no info about it in the release notes
or did i overlook something?


If it's not in the release notes i think this has been considered as a minor 
change


- as far as i can see, the patch was not changed after paulcollins' remark on
the list_display-behaviour, so it was merged as is/was


Yep, it looks like i completely missed that


- it has great potential to break one's admin and requires some digging in the
source to find out why


indeed, fortunately using a file field as first element is not so common


So, what this new feature does is: it adds a link to the file of the `FileField`
in the change_list and read-only version of the form-field in the change_view.
In the change_list that link is also added, when the `FileField` is the first
column or it is in `list_display_links`.

The result is two a-tags in the resulting html, where the link to the file takes
precedence. There is no way to go to the change_view or pick the row in a raw_id
popup anymore. And there is no way to disable the feature.

Both change_list and change_view (for read-only fields) use
`display_for_field()`, that's why both are affected by the change. That might
not have been on purpose...


I usually don't break software on purpose :)


Imho, the automatically added link to the file should *not* have precedence over
`list_display_links` and definitely should *not* break the automatically added
link to the change_view, when the `FileField` happens to be first in 
`list_display`.

What do you think?


Since you seem to care about this, any chance to open a pull request? The pull 
request should contain updated tests to verify that the double  link is fixed 
please. If not i'll give a try of fixing it later this week.


thanks

--
Riccardo Magliocchetti
@rmistaken

http://menodizero.it

--
You received this message because you are subscribed to the Google Groups "Django 
developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/5652260E.1090608%40gmail.com.
For more options, visit https://groups.google.com/d/optout.

admin: automatic link to files for FileFields in change_list breaks link to change_view (v1.8, #14497)

[Florian Demmer]

Hi guys,

i am confused/surprised by this ticket and already merged feature. it took 
me a while to find out what was happening in my admin:

https://code.djangoproject.com/ticket/14497

- it was merged into 1.8, right? the ticket says 1.2.
- there was no documentation in the patch, no info about it in the release 
notes or did i overlook something?
- as far as i can see, the patch was not changed after paulcollins' remark 
on the list_display-behaviour, so it was merged as is/was
- it has great potential to break one's admin and requires some digging in 
the source to find out why

So, what this new feature does is: it adds a link to the file of the 
`FileField` in the change_list and read-only version of the form-field in 
the change_view.
In the change_list that link is also added, when the `FileField` is the 
first column or it is in `list_display_links`.

The result is two a-tags in the resulting html, where the link to the file 
takes precedence. There is no way to go to the change_view or pick the row 
in a raw_id popup anymore. And there is no way to disable the feature.

Both change_list and change_view (for read-only fields) use 
`display_for_field()`, that's why both are affected by the change. That 
might not have been on purpose...

Imho, the automatically added link to the file should *not* have precedence 
over `list_display_links` and definitely should *not* break the 
automatically added link to the change_view, when the `FileField` happens 
to be first in `list_display`.

What do you think?

br,
Florian

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/d5a5be1f-72e4-4fb1-a774-f9d95b869581%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Case-insensitive email as username

[Aymeric Augustin]

Hello,

I spent a good part of today implementing what must be the most common scenario 
for custom user models: case-insensitive email as username. (Yes. This horse 
has been beaten to death. Multiple times.)

Since it was the first time I implemented a custom user model from scratch by 
myself, I’d like to share my experience in case that’s useful to others. Do you 
think there’s a better solution? Do you have concrete ideas for improving 
Django in this area?

The main alternative I’m aware of is a custom email field based on PostgreSQL’s 
citext type. Perhaps I’ll try that next time. Anyway, here’s what I did this 
time.


1) The documentation is excellent

 I know a lot of effort has been put into improving it and it shows. 
Congratulations to everyone involved.


2) Custom indexes would be convenient

Since I want to preserve emails as entered by the users, I cannot simply 
lowercase them. That would have been too easy.

I ended up with this migration to add the appropriate unique index on 
LOWER(email). See the comments for details.

operations = [
migrations.CreateModel(
name='User',
fields=[
('id', models.AutoField(auto_created=True, primary_key=True, 
serialize=False, verbose_name='ID')),
# … 
# unique=True was removed from the autogenerated line; a unique 
index is created below.
('email', models.EmailField(error_messages={'unique': 'A user 
with that email already exists.'}, max_length=254, verbose_name='email 
address')),
# … 
],
# … 
),
migrations.RunSQL(
# Based on editor._create_index_sql(User, 
[User._meta.get_field('email')], '_lower')
sql='CREATE UNIQUE INDEX "blabla_user_email_f86edd9d_lower" ON 
"blabla_user" (LOWER("email"))',
reverse_sql='DROP INDEX "blabla_user_email_f86edd9d_lower"',
state_operations=[
migrations.AlterField(
model_name='user',
name='email',
field=models.EmailField(error_messages={'unique': 'A user 
with that email already exists.'}, max_length=254, unique=True, 
verbose_name='email address')
),
],
),
]

It took me some time to get there. At first I tried simply removing unique=True 
on the email field but that didn’t work well.

I know there’ve been discussions about custom indexes. They would make this use 
case much easier.


3) Redefining forms isn’t too bad

I was getting quite bored of copy-paste-tweaking snippets (custom model, custom 
manager, custom admin, …) when I got to defining custom forms. Fortunately, a 
small mixin was all I needed.

(Read on for why this code uses `User.objects.get_by_natural_key(email)`.)

from django.contrib.auth.forms import UserChangeForm as BaseUserChangeForm
from django.contrib.auth.forms import UserCreationForm as BaseUserCreationForm
from django.core.exceptions import ValidationError


class CaseInsensitiveUniqueEmailMixin:
"""
ModelForm mixin that checks for email unicity, case-insensitively.

"""

def clean_email(self):
email = self.cleaned_data['email']
User = self._meta.model
field = User._meta.get_field('email')
try:
User.objects.get_by_natural_key(email)
except User.DoesNotExist:
return email
else:
raise ValidationError(
message=field.error_messages['unique'],
code='unique',
)


class UserChangeForm(CaseInsensitiveUniqueEmailMixin, BaseUserChangeForm):
pass


class UserCreationForm(CaseInsensitiveUniqueEmailMixin, BaseUserCreationForm):
pass


4) The ugly hack

My first ideas was to write a custom authentication backend to look up users by 
email case-insensitively. But I was getting bored and I noticed that 
django.contrib.auth uses `UserModel._default_manager.get_by_natural_key` to 
look up users. So...

class UserManager(BaseUserManager):
"""
Manager for the User class defined below.

Quite similar to django.contrib.auth.models.UserManager.

"""

# ...

def get_by_natural_key(self, email):
qs = self.annotate(email_lower=Lower('email'))
return qs.get(email_lower=email.lower())

/!\ This is entirely dependent on implementation details of 
django.contrib.auth. It can break when you upgrade Django; don’t blame it on 
me. /!\

That said, the nice side effect of this implementation is that it makes the 
unicity check in createsuperuser work as expected. I’m not aware of any other 
way to fix it with the database schema I chose.

I suppose an implementation of custom unique indexes with support for checking 
unicity constraints would make that point moot.


Best regards,

-- 
Aymeric.



-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" 

Re: __ne, #5763

[Anssi Kääriäinen]

On Saturday, November 21, 2015, Carsten Fuchs  wrote:

> Hi Anssi,
>
> Am 2015-11-21 um 12:50 schrieb Anssi Kääriäinen:
>
>> In summary, the imaginary query of comment 14
>>
>>  Blog.objects.filter(entry__tag__name='django',
>> entry__author_count__ne=2)
>>
>>
>> This isn't a real query. There isn't a field author_count, the query
>> needs an annotation somewhere. So, I don't think this argument is
>> convincing without the annotation (note that the place of the annotation
>> matters). In addition, providing example data and the expected results
>> would be great, too.
>>
>
> Well, yes, this is a fictional example, but if you replace author_count by
> some valid field that doesn't require an annotation, e.g. author__name, the
> (imaginary) query should be valid (for the purpose of demonstrating the
> problem)?


The author_count name suggested this was an aggregation. If this is just a
regular field, then things are a bit simpler. Note that negated Q-object +
filter and exclude() queries are the same thing.

>
> The key issue is really this, quoted from the linked Django documentation:
>
> "To handle both of these situations, Django has a consistent way of
>> processing
>> filter() calls. Everything inside a single filter() call is applied
>> simultaneously to filter out items matching all those requirements.
>> Successive
>> filter() calls further restrict the set of objects, but for multi-valued
>> relations, they apply to any object linked to the primary model, not
>> necessarily those objects that were selected by an earlier filter() call."
>>
>
> That is, sometimes we *have* to put several filters into a single filter()
> call to obtain the desired set. If such a situation requires a negation,
> exclude() cannot help, because "[...], they apply to *any* object linked to
> the primary model, not necessarily those objects that were selected by an
> earlier filter() call".
>
>
There is a fix for exactly this issue in pr
https://github.com/django/django/pull/4385. After the pr, you could just
use .filter(Q(entry__tags__name='django')&~Q(entry__author_count=2)).

The discussion seems to miss a real definition of what exactly the ne
>> lookup should do. There are two ways to implement ne, one is as a
>> complement of exact, another is as the != operator. In SQL the first one
>> is "col != val OR col IS NULL", the latter one is just "col != val".
>>
>
> Thanks for pointing this out, I wasn't aware of this (in this context)
> before. It seems to be another facet in the overall problem, but
> independent from the above, isn't it? (In my normal, "non-negated" queries,
> where required I account for NULLs explicitly all the time...)


Yeah. We should correct negated filtering to work as documented when
combined with other conditions.

So, to fix the issue in comment 14 of the ticket, the above mentioned PR is
the right fix. If you want the SQL != operator, then we need a new lookup.

 - Anssi


> Best regards,
> Carsten
>
> --
> You received this message because you are subscribed to the Google Groups
> "Django developers  (Contributions to Django itself)" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to django-developers+unsubscr...@googlegroups.com.
> To post to this group, send email to django-developers@googlegroups.com.
> Visit this group at http://groups.google.com/group/django-developers.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/django-developers/56506174.4050609%40cafu.de
> .
> For more options, visit https://groups.google.com/d/optout.
>

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/CALMtK1G3N-8zcXajSFnwuc3_THi1v5h%3DHxbivbGEV7dN_zxVjw%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.