Call ticket manipulators during batch modify

[Ryan J Ollos]

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.

[Ryan J Ollos]

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

[walty <walty8@…>]

hi,

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

thanks.

[Ryan J Ollos]

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

[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?

[walty <walty8@…>]

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.

[walty <walty8@…>]

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.

[Ryan J Ollos]

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.

[Ryan J Ollos]

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.

[walty8@…]

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.

[walty8@…]

hi,

Sorry for the late.

Here is the latest patch: ​564e0ed

I have updated the test cases as well, thanks.

[Ryan J Ollos]

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

[Christian Boos]

Moving to some later 1.3.x version.

[Ryan J Ollos]

Proposed changes in log:rjollos.git:t11723_batch_modify.

[Ryan J Ollos]

I've reconsidered and will revise these changes.

[Ryan J Ollos]

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.

[Ryan J Ollos]

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

[Ryan J Ollos]

Replying to Ryan J Ollos:

• [013616b9/rjollos.git]: add validate_comment method to interface

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

[Ryan J Ollos]

Committed to trunk in r15861.

[Jun Omae]

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

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

[Ryan J Ollos]

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.

[Jun Omae]

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.

[Ryan J Ollos]

Thanks, looks useful!

[Jun Omae]

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

[Jun Omae]

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

[Ryan J Ollos]

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.

[Jun Omae]

Thanks. Committed in [16123].

[Ryan J Ollos]

Fixed misnamed interface argument in r17290, r17291.

Related ticket: #13284