Edgewall Software

Opened 10 years ago

Closed 7 years ago

Last modified 4 years ago

#11723 closed enhancement (fixed)

Call ticket manipulators during batch modify — at Version 30

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.3.2
Component: query system Version:
Severity: normal Keywords: batch-modify
Cc: Branch:
Release Notes:
  • Ticket manipulators are called in batch modify request.
  • TICKET_BATCH_MODIFY grants TICKET_MODIFY.
API Changes:

Moved attributes max_comment_size, max_description_size and max_summary_size from TicketModule to TicketSystem. The TicketModule has accessors for backward compatibility, to be removed in 1.5.1.

Internal Changes:

Description

We should call the ticket manipulators or have a special batch ticket manipulator extension point in order to address issues such as th:#7269 and bh:#761.

Change History (31)

comment:1 by Ryan J Ollos, 9 years ago

Keywords: bitesized added

This might be as simple as copying the validation code from web_ui.py and calling it before _save_ticket_changes.

comment:2 by walty <walty8@…>, 9 years ago

Cc: walty8@… added

comment:3 by walty <walty8@…>, 9 years ago

hi,

for the case like this (call of manipulators for extensions), is that possible to have the unit tests?

thanks.

in reply to:  3 comment:4 by Ryan J Ollos, 9 years ago

Replying to walty <walty8@gmail.com>:

for the case like this (call of manipulators for extensions), is that possible to have the unit tests?

Yeah, you can create a class implementing ITicketManipulator in your TestCase. For example, see tags/trac-1.1.6/trac/web/tests/main.py@:319-325#L316.

comment:5 by walty <walty8@…>, 9 years ago

should the validations be atomic? I mean if there is a batch update of 5 tickets, but 1 ticket has failed the validation, should the other 4 tickets be still updated?

by walty <walty8@…>, 9 years ago

comment:6 by walty <walty8@…>, 9 years ago

I assumed atomic validation is preferred, i.e. if one ticket failed validation, the whole batch would not pass.

There is one issue when I was testing the manipulator. For some reason the batch modifier does not have the default action (accept / assign / close), so the ticket validator would not be triggered even if there is violation of rule.

For example, I set up a validation rule that the keyword field must exist for the assigned state.

If I try to remove the keyword from ticket using batch modifier, it would still pass. (Though the validator manipulator did get called).

But If I try to remove the keyword from ticket using batch modifier, AND checked leave the ticket assigned option, then the change would not get passed, with corresponding warning message shown up.

But I guess this issue might be better solved in another ticket.

comment:7 by walty <walty8@…>, 9 years ago

Btw, I also tried to add the unit test to the file of trac/ticket/tests/batch.py, here is the full changeset:

    def test_validate_tickets(self):
        '''Validate tickets with new values'''
        class TicketValidator(Component):
            implements(api.ITicketManipulator)
            def validate_ticket(self, req, ticket):
                errors = []
                if ticket['component'] == 'foo':
                    errors.append(('component', 'invalid component'))
                return errors

        first_ticket_id = self._insert_ticket('Test 1', reporter='joe')
        second_ticket_id = self._insert_ticket('Test 2', reporter='joe')
        selected_tickets = [first_ticket_id, second_ticket_id]
        batch = BatchModifyModule(self.env)

        new_values = {'component': 'bar'}
        self.assertTrue(batch._validate_tickets(self.req, selected_tickets,
                                                new_values))
        new_values = {'component':'foo'}
        self.assertFalse(batch._validate_tickets(self.req, selected_tickets,
                                                 new_values))

Yet, I got the following error during testing:

ERROR: test_validate_tickets (__main__.BatchModifyTestCase)
Validate tickets with new values
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tracDev/trac-trunk/trac/ticket/tests/batch.py", line 250, in test_validate_tickets
    new_values))
  File "trac/ticket/batch.py", line 203, in _validate_tickets
    message=message))
  File "trac/web/chrome.py", line 200, in add_warning
    if msg not in req.chrome['warnings']:
AttributeError: 'Mock' object has no attribute 'chrome'

So I finally removed the unit test.

Last edited 7 years ago by Ryan J Ollos (previous) (diff)

comment:8 by Ryan J Ollos, 8 years ago

Milestone: next-dev-1.1.x1.2
Owner: set to Ryan J Ollos
Status: newassigned

in reply to:  5 comment:9 by Ryan J Ollos, 8 years ago

Replying to walty <walty8@…>:

should the validations be atomic? I mean if there is a batch update of 5 tickets, but 1 ticket has failed the validation, should the other 4 tickets be still updated?

Yes, I tend to think the validations should be atomic.

comment:10 by Ryan J Ollos, 8 years ago

Cc: walty8@… removed
Milestone: 1.21.3.1
Owner: changed from Ryan J Ollos to walty8@…

I'll have to test out the patch to understand the issue in comment:6.

Regarding the issue with unit testing in comment:7, you shouldn't see the issue after #12211. However, I recommend calling BatchModify.process_request in the unit test rather than testing the internal method _validate_tickets.

Would you mind updating the test and regenerating the patch against the current trunk, or putting the changes in Git and rebasing against the current trunk? We can aim to push the changes in milestone:1.3.1.

Btw, comment:7 was one of the motivations for finally pushing ahead with #12211. I hope that MockRequest makes it easier for contributors to write unit tests.

Last edited 8 years ago by Ryan J Ollos (previous) (diff)

in reply to:  10 comment:11 by walty8@…, 8 years ago

Replying to Ryan J Ollos:

Would you mind updating the test and regenerating the patch against the current trunk, or putting the changes in Git and rebasing against the current trunk? We can aim to push the changes in milestone:1.3.1.

No problem, I would try to accomplish it by the weekend. And I would probably use github for the patching this time.

comment:12 by walty8@…, 8 years ago

hi,

Sorry for the late.

Here is the latest patch: 564e0ed

I have updated the test cases as well, thanks.

comment:13 by Ryan J Ollos, 8 years ago

Thanks. I'll review and probably commit at start of 1.3.1.

comment:14 by Christian Boos, 7 years ago

Milestone: 1.3.1next-dev-1.3.x

Moving to some later 1.3.x version.

comment:15 by Ryan J Ollos, 7 years ago

Milestone: next-dev-1.3.x1.3.2
Owner: changed from walty8@… to Ryan J Ollos

comment:16 by Ryan J Ollos, 7 years ago

Release Notes: modified (diff)

comment:17 by Ryan J Ollos, 7 years ago

I've reconsidered and will revise these changes.

comment:18 by Ryan J Ollos, 7 years ago

I added more changes to log:rjollos.git:t11723_batch_modify.1. TicketSystem implements ITicketManipulator, which can be used by both TicketModule and BatchModule.

I can't find a good way to have the ticket comment validated in the ITicketManipulator implementation since the value is retrieved from the request and not available in the Ticket object. Instead, the ticket comment validation is done separately. With that in mind, perhaps another method on ITicketManipulator, validate_comment, would be appropriate (comment:2:ticket:12722). Or, we could modify the signature of validate_ticket to add an optional comment parameter, with appropriate handling for backward-compatibility.

I'm posting early for feedback. Before committing I'll add accessors on the TicketModule class for the moved max_ attributes (see the TODO in code). I'll also rework some of the old test cases in batch.py so that we are testing through the public interfaces rather than private methods.

comment:19 by Ryan J Ollos, 7 years ago

log:rjollos.git:t11723_batch_modify.2 experiments with two approaches for validating the ticket comment:

  • [04b74819/rjollos.git]: add comment parameter to validate_ticket. Downside is that the comment is validated for every ticket in batch modify rather than once per batch modify request.
  • [013616b9/rjollos.git]: add validate_comment method to interface

in reply to:  19 comment:20 by Ryan J Ollos, 7 years ago

API Changes: modified (diff)
Release Notes: modified (diff)

Replying to Ryan J Ollos:

Expanded on this change to add backward-compatible accessors to TicketModule, and refactored some test cases: log:rjollos.git:t11723_batch_modify.3.

comment:21 by Ryan J Ollos, 7 years ago

Resolution: fixed
Status: assignedclosed

Committed to trunk in r15861.

comment:22 by Jun Omae, 7 years ago

After [15861], 1 failure of unit tests if Babel is unavailable.

======================================================================
FAIL: test_validate_time_fields (trac.ticket.tests.batch.BatchModifyTestCase)
The time fields are validated.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/run/shm/ca0f3c5509ff4884aaa3c5fe9e2dbadc1dd2693b/py27-sqlite/trac/ticket/tests/batch.py", line 667, in test_validate_time_fields
    unicode(req2.chrome['warnings'][0]))
AssertionError: 'The ticket field <strong>Time1</strong> is invalid: "invalid" is an invalid date, or the date format is not known. Try "MMM d, y" or "YYYY-MM-DD" instead.' != u'The ticket field <strong>Time1</strong> is invalid: "invalid" is an invalid date, or the date format is not known. Try "MM/dd/YY" or "YYYY-MM-DD" instead.'

----------------------------------------------------------------------

comment:23 by Ryan J Ollos, 7 years ago

Thanks, I've fixed the failure in r15881. We should look at adding a TravisCI test configuration that runs tests in an environment without the optional dependencies.

in reply to:  23 comment:24 by Jun Omae, 7 years ago

Replying to Ryan J Ollos:

We should look at adding a TravisCI test configuration that runs tests in an environment without the optional dependencies.

Proposed changes in 36ce8447a, added build without optional dependencies to build matrix. The result is in https://travis-ci.org/jun66j5/trac/builds/231775454.

comment:25 by Ryan J Ollos, 7 years ago

Thanks, looks useful!

comment:26 by Jun Omae, 7 years ago

Added minimum dependencies build for 1.2-stable and trunk in [15905,15906].

comment:27 by Jun Omae, 7 years ago

I just changed a message introduced in [15861] like this:

-The ticket %(field)s is invalid: %(message)s
+The ticket %(comment)s is invalid: %(message)s

However, I think it would be simple and clear to remove tag.strong(_('comment')) parameter.

-                    add_warning(req, tag_("The ticket %(comment)s is invalid: "
-                                          "%(message)s",
-                                          comment=tag.strong(_('comment')),
+                    add_warning(req, tag_("The ticket comment is invalid: "
+                                          "%(message)s",
                                           message=message))

comment:28 by Ryan J Ollos, 7 years ago

That change looks fine to me. I added the strong for consistency with the the message "The ticket field FieldName is invalid", but I agree that it doesn't add much and we should consider the ease for translators.

comment:29 by Jun Omae, 7 years ago

Thanks. Committed in [16123].

comment:30 by Ryan J Ollos, 6 years ago

Release Notes: modified (diff)
Note: See TracTickets for help on using tickets.