Fixture validation errors should report their data

[freyley]

For example, here's what django/db/models/fields/__init__.py has for lines 341-348:

def to_python(self, value):
    if value is None:
        return value
    try:
        return int(value)
    except (TypeError, ValueError):
        raise exceptions.ValidationError(_("This value must be an integer."))

Changing line 348 to:

_("(%s) must be an integer." % value))

means that when you convert a text field to a joined object, you know which one's broken where. (Other places in that file do the same thing)

[Raúl Cumplido]

It seems the code is changed since the bug was reported. Now the code has dictionaries with default_error_messages. Example for lines 865-857:

    default_error_messages = {
        'invalid': _("This value must be an integer."),
    }

And in the Exception it has the next, lines 884-890:

    def to_python(self, value):
        if value is None:
            return value
        try: 
            return int(value)
        except (TypeError, ValueError):
            raise exceptions.ValidationError(self.error_messages['invalid'])

Should we change the code to:

    default_error_messages = {
        'invalid': _("(%s) must be an integer."),
    }

    def to_python(self, value):
        if value is None:
            return value
        try: 
            return int(value)
        except (TypeError, ValueError):
            msg = self.error_messages['invalid'] % _(str(value))
            raise exceptions.ValidationError(msg)

I am new and that's why I taked an easy_pickings one. But I want to have your ok to prepare my first patch.

[Łukasz Rekucki]

The message proposed by the OP is a bit weird. If you insert the non-valid value to the error message you get something like:

(foobar) must be an integer

I'm a bit puzzled by the assumption, that this code is only used by serialization framework. It could've been true 14 months ago, but now it's a part of model validation. I don't want my users to see messages like these.

[Raúl Cumplido]

Do you think we had to close it? Maybe 21 months later since it was opened it makes no sense.

[Russell Keith-Magee]

@raulcd -- What you have proposed sounds right to me. My only comment: you don't need to wrap the value in "_()". That marks a string for translation, which isn't something we can do with user input.

@lrekucki's comment is purely about the literal text for the message. The parentheses in "(foobar) must be an integer" is a strange construction. '"foobar" must be an integer' would make more sense.

[Russell Keith-Magee]

@raulcd -- As described in the ​contribution guide and ​HOWTO, you shouldn't mark your own patch as ready for checkin; that's for someone else to decide. We like to get at least 2 sets of eyes on a patch before marking it for commit.

In this case, your patch doesn't have tests, so it can't be ready for checkin.

[Raúl Cumplido]

@russellm -- I am sorry I misunderstood the documentation. I thought the meaning of the ready for chekin was set the ticket as "ey guys, can anyone take a look, I did my patch". I will know that for the next one. About the tests I executed the full test suite and as no test failed I thought the "literal" of the error may not be tested. I will prepare the tests and submit the new patch, just a question (as I am new) where do you think is the best location to create the tests? In /trunk/tests/modeltests/validation or create a new folder with the tests. I am sorry for this questions, but as a newbie I want to follow your standards.

[Raúl Cumplido]

Added their data to fixture validation errors and tests

[Raúl Cumplido]

I finally added tests to a new file. It would be better, as someone in the IRC told me, to move them if you think they fit better in another location. (tests/modeltests/validation/test_error_messages.py is the new test file).

As you told me I will not set the ready for chekin, just wait comments. I removed the need tests checkbox (I don't know if I had to do it or someone who review the patch has to do it).

[Anand Kumria]

The current patch does not apply cleanly to the tree, I am working on this as part of the DjangoCon sprint and will it up.

[Anand Kumria]

updated version of this patch

[Peter van Kampen]

Is testing all cases that were fixed, applies cleanly to trunk

[Malcolm Tredinnick]

In [16638]:

Improved error message display during validation errors.

When reporting a validation error (during model validation or fixture
loading, for example), the error messages now report the bad value as
well as the expected type. This can make identifying the offending field
and problem a bit easier.

Fixed #11595. Patch from raulcd and wildfire with supervision from
Russell.