Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for trac 1.4 #13

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Support for trac 1.4 #13

wants to merge 3 commits into from

Conversation

the-moog
Copy link

I think this creates support for trac 1.4, or at least it seems to be working for us. Not sure I used the correct trac API methods though and I'm fairly sure there is more that needs updating. (I'm not familiar with trac APIs)

Copy link
Member

@rjollos rjollos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good aside from the minor things I noted. If we can remove the commented code too, I think it's close to ready to merge.

@@ -1,4 +1,4 @@
from api import *
import pkg_resources

pkg_resources.require('Trac >= 1.0')
pkg_resources.require('Trac >= 1.3')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes you made should be compatible with 1.2 as well. Could we set this to 1.2?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you can test it, sure

from trac.perm import IPermissionRequestor, PermissionError
from trac.resource import get_resource_url, get_resource_name

from genshi.core import Markup
from genshi.builder import tag
from genshi.filters import Transformer

from trac.web.chrome import web_context

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd move this into the from trac.* block of imports.

def __init__(self, target, time, author, comment=None):

super(TicketChangeEvent, self).__init__('ticket', 'reminder', target,
time, author)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class may not be necessary. TicketChangeEvent has a 6th arg comment. Can TicketChangeEvent be used instead?

@rjollos rjollos self-assigned this Nov 20, 2020
@the-moog
Copy link
Author

I may have been a bit premature. Still testing on our system. Found one issue already. Will know more tomorrow. I subclassed TicketChangeEvent as I thought perhaps a reminder needed some extra info e.g. age (not added yet)

@the-moog
Copy link
Author

I think I've found the cause of the problem. Not sure what to do to fix it.
Can somebody assist?
See the-moog#1 (comment)

@rjollos
Copy link
Member

rjollos commented Nov 23, 2020

I think I've found the cause of the problem. Not sure what to do to fix it.
Can somebody assist?
See the-moog#1 (comment)

You'll need to implement an INotificationSubscriber. Examples:

AlwaysEmailSubscriber is a subscriber that is always active. TicketOwnerSubscriber requires the user to subscribe by setting their preferences (/prefs/notification), or a default subscription configured. See INotificationSubscriber.

Let me know if you have any questions. Some aspects of implementing INotificationSubscriber can be confusing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants