Edgewall Software

Opened 10 years ago

Last modified 4 years ago

#11723 closed enhancement

Call ticket manipulators during batch modify — at Version 16

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 validators are called in batch modify request.

API Changes:
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 (17)

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)
Note: See TracTickets for help on using tickets.