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 , 9 years ago
Keywords: | bitesized added |
---|
comment:2 by , 9 years ago
Cc: | added |
---|
follow-up: 4 comment:3 by , 9 years ago
hi,
for the case like this (call of manipulators for extensions), is that possible to have the unit tests?
thanks.
comment:4 by , 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.
follow-up: 9 comment:5 by , 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 , 9 years ago
Attachment: | 11723-batch-modify-manipulator.v1.patch added |
---|
comment:6 by , 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 , 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.
comment:8 by , 8 years ago
Milestone: | next-dev-1.1.x → 1.2 |
---|---|
Owner: | set to |
Status: | new → assigned |
comment:9 by , 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.
follow-up: 11 comment:10 by , 8 years ago
Cc: | removed |
---|---|
Milestone: | 1.2 → 1.3.1 |
Owner: | changed from | to
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.
comment:11 by , 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 , 8 years ago
hi,
Sorry for the late.
Here is the latest patch: 564e0ed
I have updated the test cases as well, thanks.
comment:15 by , 7 years ago
Milestone: | next-dev-1.3.x → 1.3.2 |
---|---|
Owner: | changed from | to
comment:16 by , 7 years ago
Release Notes: | modified (diff) |
---|
Proposed changes in log:rjollos.git:t11723_batch_modify.
This might be as simple as copying the validation code from web_ui.py and calling it before _save_ticket_changes.