#11723 closed enhancement (fixed)
Call ticket manipulators during batch modify
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: |
|
||
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.
Attachments (1)
Change History (35)
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 , 9 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.
comment:18 by , 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.
follow-up: 20 comment:19 by , 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 tovalidate_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
comment:20 by , 7 years ago
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.
comment:21 by , 7 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Committed to trunk in r15861.
comment:22 by , 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.' ----------------------------------------------------------------------
follow-up: 24 comment:23 by , 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.
comment:24 by , 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:26 by , 7 years ago
Added minimum dependencies build for 1.2-stable and trunk in [15905,15906].
comment:27 by , 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 , 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:30 by , 7 years ago
Release Notes: | modified (diff) |
---|
comment:31 by , 4 years ago
comment:32 by , 4 years ago
API Changes: | modified (diff) |
---|
comment:33 by , 4 years ago
comment:34 by , 4 years ago
Keywords: | bitesized removed |
---|
This might be as simple as copying the validation code from web_ui.py and calling it before _save_ticket_changes.