Edgewall Software
Modify

Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#10353 closed enhancement (fixed)

Error messages more than one from validate_attachment are ignored

Reported by: Scott Wang <scott_wang@…> Owned by: osimons
Priority: normal Milestone: 1.0
Component: attachment Version: 0.12.2
Severity: normal Keywords: IAttachmentManipulator validate_attachment
Cc: Branch:
Release Notes:

Better reporting of problems detected by Attachment validators

API Changes:
Internal Changes:

Description

We wrote an IAttachmentManipulator plugin to validate a user attachment. It can be more than one error that fails the validation, and we want to tell the user about all of them.

In the description of the validate_attachment API, it says that it "Must return a list of (field, message) tuples, one for each problem detected. "

However, in where the plugin get called, it will raise an exception when the first error is found and ignore the rest of them:

for field, message in manipulator.validate_attachment(req, attachment):
    if field:
        raise InvalidAttachment(
            _('Attachment field %(field)s is invalid: %(message)s',
              field=field, message=message))
    else:
        raise InvalidAttachment(
            _('Invalid attachment: %(message)s', message=message))

If it cannot show multiple errors, then the description of the API should be modified.

Attachments (7)

show_attachment_manipulator_error.diff (1.9 KB ) - added by Scott Wang <scott_wang@…> 9 years ago.
show_attachment_manipulator_error.2.diff (1.7 KB ) - added by Scott Wang <scott_wang@…> 9 years ago.
attachment_error_screenshot.PNG (6.7 KB ) - added by Scott Wang <scott_wang@…> 9 years ago.
show_attachment_manipulator_error.3.diff (1.8 KB ) - added by scott_wang@… 9 years ago.
warning_msg.png (41.9 KB ) - added by scott_wang@… 9 years ago.
t10353-show_attachment_manipulator_error_4-r10893.diff (3.6 KB ) - added by osimons 9 years ago.
Patch against current trunk, updated to handle re-populating fields.
t10353-show_attachment_manipulator_error_5-r10893.diff (3.7 KB ) - added by osimons 9 years ago.
Added extra warning for user to select the file to upload again.

Download all attachments as: .zip

Change History (31)

comment:1 by Remy Blank, 9 years ago

Milestone: next-major-0.1X

Yes, it would be better to collect and display all messages. Would you like to try and write a patch?

comment:2 by Scott Wang <scott_wang@…>, 9 years ago

Of course, I will submit the patch soon.

by Scott Wang <scott_wang@…>, 9 years ago

comment:3 by Scott Wang <scott_wang@…>, 9 years ago

Proposed patch is attached. Use add_warning() to show all errors, which is similar to what is done in ticket validation.

comment:4 by Remy Blank, 9 years ago

Milestone: next-major-0.1X0.13
Owner: set to Remy Blank
Type: defectenhancement

That's a good start. Instead of having separate error and warning boxes, I would prefer placing everything into the error box. You can generate an HTML fragment with tag (from genshi.builder), and pass that fragment as the exception message. So you could have the error message and a <ul> list below with the individual messages.

in reply to:  4 comment:5 by Scott Wang <scott_wang@…>, 9 years ago

Replying to rblank:

That's a good start. Instead of having separate error and warning boxes, I would prefer placing everything into the error box. You can generate an HTML fragment with tag (from genshi.builder), and pass that fragment as the exception message. So you could have the error message and a <ul> list below with the individual messages.

Got it. I was originally going to write in the error message but just could not figure out how to display more then one item.

I will try it and submit a new one.

by Scott Wang <scott_wang@…>, 9 years ago

by Scott Wang <scott_wang@…>, 9 years ago

comment:6 by Scott Wang <scott_wang@…>, 9 years ago

An alternative patch is attached, along with the screenshot of its output. Now it gathers all error reasons generated from all attachment manipulators and put it in a single error message box.

comment:7 by osimons, 9 years ago

Cc: osimons added

Thanks for finding this bug, and obviously all warnings should be collected before we abort the save.

I've got a few comments though, mostly general and not patch-specific:

  1. I don't like collecting and exposing the plugin name to the user. We don't do this anywhere else in Trac, and no need to do it here.
  1. Why not use add_warning() like we do for Ticket manipulators and similar? Here is the ticket code, just modify slightly to redirect back to the same page and re-populate the form with description, checkbox and filename (if possible):
            valid = True
            # Custom validation rules
            for manipulator in self.ticket_manipulators:
                for field, message in manipulator.validate_ticket(req, ticket):
                    valid = False
                    if field:
                        add_warning(req, _("The ticket field '%(field)s' is "
                                           "invalid: %(message)s",
                                           field=field, message=message))
                    else:
                        add_warning(req, message)
            return valid
    
  1. I need to completely re-implement this validation and other checks for the RPC plugin - preferably without web-formatted errors… Perhaps trac.attachment would be a good first place to try to split the 2 levels into 3 - for a total of 4 components:
    • Attachment for model/db much as today.
    • AttachmentCore for exposing the public attachment api to be used by internal and external modules for all attachment manipulation, and that handles permissions, manipulators, listeners and so on.
    • AttachmentModule that exposes attachments to the web and handles web-based request manipulation presentation and feedback.
    • AttachmentAdmin that similarly provides reading and manipulation via the command line, that similarly would benefit from additions such as support for listeners.
  1. Various places in Trac where we check for 'warnings' or similarly collect potential issues, and I have the same in a number of my own plugins. These are then situations where you can either call a single TracError (or any of its derivatives), or collect a list of warnings to pass around. What I have personally ended up doing is making a new TracWarnings error class that I use internally in my code. I think it works well to signal collected warnings up the hierarchy, particularly useful when the number of layers increase from 2 to 3 by adding a core level that really should be output-agnostic. Simplified, it just looks like this:
    class TracWarnings(Exception):
        """
        A class used to signal up any warnings that may occur for
        operations at lower levels in the code. This exception types
        contain a ``.warnings`` array with any messages for user.
        """
    
        def __init__(self, warnings=None):
            self.warnings = warnings or []
    
    

in reply to:  7 ; comment:8 by Christian Boos, 9 years ago

Replying to osimons:

  • I'd rather have AttachmentSystem instead of AttachmentCore, for symmetry with other sub-systems; also do you think we should split attachment.py to attachment/{model,api,web_ui,admin}.py?
  • about TracWarnings: is it really worth having another Exception (not even inheriting from TracError) instead of extending TracError with this warnings property? If it's just used for warnings and not for a "real" error (i.e. the operation succeeds in the end), then is using an Exception really appropriate?

in reply to:  8 ; comment:9 by osimons, 9 years ago

Replying to cboos:

  • I'd rather have AttachmentSystem instead of AttachmentCore, for symmetry with other sub-systems;

Sure, it is much like the other *System classes, so the name is of less importance to me.

also do you think we should split attachment.py to attachment/{model,api,web_ui,admin}.py?

Not so important now, but I suppose that makes sense too at some stage going forward.

  • about TracWarnings: is it really worth having another Exception (not even inheriting from TracError) instead of extending TracError with this warnings property? If it's just used for warnings and not for a "real" error (i.e. the operation succeeds in the end), then is using an Exception really appropriate?

It is very much 'real' IMHO. It is checks that halts a processing, and very much an signal for out-of-the-ordinary handling. The idea with keeping it distinct from a TracError is that you can distinguish them from each other and handle them in different except clauses. And our other internal errors don't usually inherit from TracError? Extending a common error object with all kinds of facets and custom handling will instead litter the code with if else checks for various properties and circumstances.

Anyway, it is something I've found more intuitive for my own code. But, return warnings works too. It is just that raising some general InvalidAttachment and packing the message with formatted information is not ideal for me.

in reply to:  9 comment:10 by Christian Boos, 9 years ago

Replying to osimons:

Replying to cboos:

  • about TracWarnings: is it really worth having another Exception (not even inheriting from TracError) instead of extending TracError with this warnings property? If it's just used for warnings and not for a "real" error (i.e. the operation succeeds in the end), then is using an Exception really appropriate?

It is very much 'real' IMHO. It is checks that halts a processing, and very much an signal for out-of-the-ordinary handling.

If you mean to use that exception "locally", in a try: ... except TracWarnings, e: ..., but you don't want that exception to actually terminate a request the way TracError does, then my concerns about this class don't hold.

Otherwise, if you want to use it as an alternate way to terminate a request like a TracError can do, but with "structured" extra informations instead of markup, then I think it's probably easier to simply extend TracError in some ways (so that we can just go through the normal HTTPException.__init__, _send_user_error sequence).

In the example of the show_attachment_manipulator_error.2.diff this could translate to:

    raise InvalidAttachment(title=_("Invalid attachment"), reasons=all_errs)

And in TracError, the message property would derive from both _message and the new reasons attribute.

in reply to:  7 ; comment:11 by Scott Wang <scott_wang@…>, 9 years ago

Replying to osimons:

Thanks for finding this bug, and obviously all warnings should be collected before we abort the save.

I've got a few comments though, mostly general and not patch-specific:

  1. I don't like collecting and exposing the plugin name to the user. We don't do this anywhere else in Trac, and no need to do it here.

It is useful in my use case, but if it does not make sense in general cases, it can be removed as well.

  1. Why not use add_warning() like we do for Ticket manipulators and similar? Here is the ticket code, just modify slightly to redirect back to the same page and re-populate the form with description, checkbox and filename (if possible):
            valid = True
            # Custom validation rules
            for manipulator in self.ticket_manipulators:
                for field, message in manipulator.validate_ticket(req, ticket):
                    valid = False
                    if field:
                        add_warning(req, _("The ticket field '%(field)s' is "
                                           "invalid: %(message)s",
                                           field=field, message=message))
                    else:
                        add_warning(req, message)
            return valid
    

Actually the first version of the patch uses the add_warning method just like the one in the ticket manipulator does. How are we going to solve this problem?

in reply to:  11 comment:12 by Remy Blank, 9 years ago

Replying to Scott Wang <scott_wang@…>:

Actually the first version of the patch uses the add_warning method just like the one in the ticket manipulator does. How are we going to solve this problem?

I should have been clearer in comment:4, sorry. What I didn't like was displaying an error page, and having the errors in a separate warning box. An error page should have the error in the error box, so that's what I suggested.

What Simon suggests above is not to display an error page, but to go back to the attachment page, with all fields unchanged, and the warnings displayed on top. It's much more user-friendly, as the user doesn't have to go back to the attachment page. We do the same e.g. when submitting tickets, and we should really do it everywhere (e.g. invalid milestone times, …), because it's so much nicer.

by scott_wang@…, 9 years ago

by scott_wang@…, 9 years ago

Attachment: warning_msg.png added

comment:13 by scott_wang@…, 9 years ago

Has been busy for a while before fixing this issue.

New patch and screenshot attached. Now it will stay on the same page and show the warning message as suggested.

comment:14 by Remy Blank, 9 years ago

This looks much better. It should be possible to avoid the redirect by simply re-rendering the "Add Attachment" page, already pre-filled with the field values passed in the request.

in reply to:  14 ; comment:15 by scott_wang@…, 9 years ago

Replying to rblank:

This looks much better. It should be possible to avoid the redirect by simply re-rendering the "Add Attachment" page, already pre-filled with the field values passed in the request.

Well, I'm not familiar with the code and does not know how to do that efficiently without adding extra stuff. Maybe someone knows the attachment module better can do this?

by osimons, 9 years ago

Patch against current trunk, updated to handle re-populating fields.

in reply to:  15 ; comment:16 by osimons, 9 years ago

Cc: osimons removed
Owner: changed from Remy Blank to osimons

Replying to scott_wang@…:

Well, I'm not familiar with the code and does not know how to do that efficiently without adding extra stuff. Maybe someone knows the attachment module better can do this?

See t10353-show_attachment_manipulator_error_4-r10893.diff. Seeing the browser only forwards the filename and not the full path, there is no way that I can think of to actually repopulate the filename field too. Should we add another warning to make sure the user remember to re-select a file?

Note: The patch is now updated against trunk, and not 0.12. I doubt it will make stable at this point.

in reply to:  16 ; comment:17 by Remy Blank, 9 years ago

Replying to osimons:

See t10353-show_attachment_manipulator_error_4-r10893.diff. Seeing the browser only forwards the filename and not the full path, there is no way that I can think of to actually repopulate the filename field too. Should we add another warning to make sure the user remember to re-select a file?

That's probably a good idea, yes.

by osimons, 9 years ago

Added extra warning for user to select the file to upload again.

in reply to:  17 comment:18 by osimons, 9 years ago

Replying to rblank:

That's probably a good idea, yes.

Agree. Added to t10353-show_attachment_manipulator_error_5-r10893.diff.

comment:19 by Remy Blank, 8 years ago

Milestone: 1.01.0-triage

Preparing for 1.0.

comment:20 by Christian Boos, 8 years ago

Simon, any chance this could come to a conclusion for 1.0?

comment:21 by osimons, 8 years ago

The patch applies cleanly in latest version, and I have nothing further to add at this point. Shall I commit?

comment:22 by Remy Blank, 8 years ago

Yes, please do :)

comment:23 by osimons, 8 years ago

Milestone: next-stable-1.0.x1.0
Resolution: fixed
Status: newclosed

Committed as [11106].

comment:24 by Christian Boos, 8 years ago

Release Notes: modified (diff)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain osimons.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from osimons to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.