Opened 15 years ago

Closed 13 years ago

#11595 closed Cleanup/optimization (fixed)

Fixture validation errors should report their data

Reported by: freyley Owned by: Raúl Cumplido
Component: Core (Serialization) Version: 1.0
Severity: Normal Keywords: easy-pickings
Cc: Anand Kumria, Peter van Kampen Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description (last modified by Łukasz Rekucki)

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)

Attachments (2)

fixture_validation.diff (7.4 KB ) - added by Raúl Cumplido 14 years ago.
Added their data to fixture validation errors and tests
fixture_validationv2.diff (7.7 KB ) - added by Anand Kumria 13 years ago.
updated version of this patch

Download all attachments as: .zip

Change History (20)

comment:1 by Russell Keith-Magee, 15 years ago

Has patch: unset
Triage Stage: UnreviewedAccepted

comment:2 by Julien Phalip, 14 years ago

Description: modified (diff)
Keywords: easy-pickings added
Severity: Normal
Type: Cleanup/optimization

comment:3 by Raúl Cumplido, 14 years ago

Owner: changed from nobody to Raúl Cumplido
Status: newassigned

comment:4 by Raúl Cumplido, 14 years ago

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.

Last edited 14 years ago by Łukasz Rekucki (previous) (diff)

comment:5 by Łukasz Rekucki, 14 years ago

Description: modified (diff)

comment:6 by Łukasz Rekucki, 14 years ago

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.

comment:7 by Raúl Cumplido, 14 years ago

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

comment:8 by Russell Keith-Magee, 14 years ago

@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.

comment:9 by Raúl Cumplido, 14 years ago

Has patch: set
Triage Stage: AcceptedReady for checkin

comment:10 by Russell Keith-Magee, 14 years ago

Needs tests: set
Triage Stage: Ready for checkinAccepted

@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.

comment:11 by Raúl Cumplido, 14 years ago

@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.

by Raúl Cumplido, 14 years ago

Attachment: fixture_validation.diff added

Added their data to fixture validation errors and tests

comment:12 by Raúl Cumplido, 14 years ago

Needs tests: unset

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).

comment:13 by Jacob, 14 years ago

Easy pickings: set

comment:14 by Anand Kumria, 13 years ago

UI/UX: unset

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.

by Anand Kumria, 13 years ago

Attachment: fixture_validationv2.diff added

updated version of this patch

comment:15 by Peter van Kampen, 13 years ago

Triage Stage: AcceptedReady for checkin

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

comment:16 by Anand Kumria, 13 years ago

Cc: Anand Kumria added

comment:17 by Peter van Kampen, 13 years ago

Cc: Peter van Kampen added

comment:18 by Malcolm Tredinnick, 13 years ago

Resolution: fixed
Status: assignedclosed

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.

Note: See TracTickets for help on using tickets.
Back to Top