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

Redmine improvements #395

Merged
merged 17 commits into from
Jan 29, 2017

Conversation

kostajh
Copy link
Contributor

@kostajh kostajh commented Oct 12, 2016

This PR is for #390. I'm posting this now in case someone wants to carry on, I'm going to be mostly offline for the next few weeks. (If you do work on this please post here so we're not working to cross purposes!)

Still TODO:

  • Load comments as annotations
  • Support for custom fields
  • Pagination support for more than 100 records?
  • Fix tests

Everything here is working for me, btw.

@ryneeverett
Copy link
Collaborator

I rebased. I'm a little horrified that this new github feature encourages this kind of thing though. Anyone with commit access can anonymously screw with PR's and the modifications are unlikely to be detected if they're careful. In other words, you can no longer count on the submitter to have reviewed their own PR!

@kostajh kostajh force-pushed the redmine-improvements branch from 19fe8db to f033927 Compare December 6, 2016 16:23
@kostajh
Copy link
Contributor Author

kostajh commented Jan 3, 2017

@ryneeverett this all works, but I could use help figuring out what to do with the tests. Also, is it best practice to map the Redmine due date directly to a TW due date? I'm not sure the other services do this.

if issue_limit is not None:
args["limit"] = issue_limit

args["assigned_to_id"] = 'me'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you mean to leave this this way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but, should it be conditional based on whether the user has selected only_if_assigned = True in their config?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I hadn't realized that "me" is a special value in the redmine API.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, but, should it be conditional based on whether the user has selected only_if_assigned = True in their config?

Yes, I believe that's correct.

@ryneeverett
Copy link
Collaborator

Reactions in no particular order:

  1. Do we know why redmine's time zones are different than other services? In the tests, the other services return a datetime with a pytz.UTC timezone while redmine returns a datetime with a dateutil.tz.tz.tzutc(). This may well be an inconsistency in the way I wrote the tests, but it is curious.
  2. The UDA ASSIGNED_TO appears to be equivalent to gitlab's ASSIGNEE and bz's ASSIGNED. Would it make sense to standardize? I'd be inclined towards ASSIGNEE.
  3. The following UDA's appear to be completely unique across services:
  • TRACKER
  • CATEGORY
  • START_DATE
  • SPENT_HOURS
  • ESTIMATED_HOURS

I haven't developed an opinion on their merits, but this may be worth considering and soliciting feedback on.

@kostajh
Copy link
Contributor Author

kostajh commented Jan 8, 2017

@ryneeverett thank you for fixing the tests!

Do we know why redmine's time zones are different than other services? In the tests, the other services return a datetime with a pytz.UTC timezone while redmine returns a datetime with a dateutil.tz.tz.tzutc(). This may well be an inconsistency in the way I wrote the tests, but it is curious.

I'm not sure about this, sorry. I do know that what's in redmine.py in this PR works for keeping the dates consistent after running bugwarrior-pull, when I first started working on this PR, Taskwarrior thought that the date field was changing across each sync and would update each task on every pull.

The UDA ASSIGNED_TO appears to be equivalent to gitlab's ASSIGNEE and bz's ASSIGNED. Would it make sense to standardize? I'd be inclined towards ASSIGNEE.

Up to you. assigned_to and assigned are how these fields are defined in Redmine, so IMO it makes sense to keep the same format for this service, but I don't have a strong feeling about it one way or the other.

The following UDA's appear to be completely unique across services

Same as above, these are core fields provided by Redmine for issues. I use all of them in my Taskwarrior reports, so I'd like to keep them :)

A related point, in Redmine one can have custom fields. I'm not sure if it's within scope to allow a user to define custom fields to pull from or how we could achieve these in .bugwarriorrc. But if there is a solution for custom fields, then I could use that for ESTIMATED_HOURS and SPENT_HOURS, etc.

@kostajh kostajh changed the title [WIP] Redmine improvements Redmine improvements Jan 8, 2017
         self.issue_limit = issue_limit

-    def find_issues(self, issue_limit=100):
+    def find_issues(self, issue_limit=100, only_if_assigned=False):
         args = {}
         # TODO: if issue_limit is greater than 100, implement pagination to return all issues.
         # Leave the implementation of this to the unlucky soul with >100 issues assigned to them.
         if issue_limit is not None:
             args["limit"] = issue_limit

-        args["assigned_to_id"] = 'me'
+        if only_if_assigned:
+            args["assigned_to_id"] = 'me'
         return self.call_api("/issues.json", args)["issues"]

     def call_api(self, uri, params):
@@ -251,8 +252,8 @@ class RedMineService(IssueService):
         IssueService.validate_config(config, target)

     def issues(self):
-
-        issues = self.client.find_issues(self.issue_limit)
+        only_if_assigned = self.config_get_default('only_if_assigned', False)
+        issues = self.client.find_issues(self.issue_limit, only_if_assigned=only_if_assigned)
         log.debug(" Found %i total.", len(issues))
         for issue in issues:
             yield self.get_issue_for_record(issue)
diff --git a/tests/test_redmine.py b/tests/test_redmine.py
index 7ed8941..8b1fe64 100644
--- a/tests/test_redmine.py
+++ b/tests/test_redmine.py
@@ -97,7 +97,7 @@ class TestRedmineIssue(AbstractServiceTest, ServiceTest):
     @responses.activate
     def test_issues(self):
         self.add_response(
-            'https://something/issues.json?assigned_to_id=me&limit=100',
+            'https://something/issues.json?limit=100',
             json={'issues': [self.arbitrary_issue]})

         issue = next(self.service.issues())
@ryneeverett
Copy link
Collaborator

Also, is it best practice to map the Redmine due date directly to a TW due date?

No. See #385.

Copy link
Collaborator

@ryneeverett ryneeverett left a comment

Choose a reason for hiding this comment

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

The date format is the only remaining thing I see that still needs consideration.

calc = tw._execute('calc', estimated_hours)
return (
calc[0].rstrip()
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

None of the other services convert to taskwarrior format so I'm doubtful we should be here. You want to use the redmine duedate as your taskwarrior duedate though, and I'm not sure how, or if it's possible, to do this if the due dates are stored in integer format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The estimated_hours field is different from due date. Estimated hours comes from Redmine in the format 2, 2.9, 3.25, etc. Here I am converting this integer value to a taskwarrior duration field.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wow, I have no idea what I was thinking an hour ago.

@kostajh
Copy link
Contributor Author

kostajh commented Jan 11, 2017

thanks very much for your help @ryneeverett! I'll follow up later with annotations support. Also, do you have any thoughts on how custom field support might best be implemented?

@ryneeverett
Copy link
Collaborator

I'll follow up later with annotations support.

In a separate PR? I think this one's big enough and pretty well ready to merge.

Also, do you have any thoughts on how custom field support might best be implemented?

I'm assuming you know about field templates. Can you be a bit more specific about what you're trying to do?

@kostajh
Copy link
Contributor Author

kostajh commented Jan 11, 2017

In a separate PR? I think this one's big enough and pretty well ready to merge.

Yes in a separate PR.

I'm assuming you know about field templates. Can you be a bit more specific about what you're trying to do?

Sure. And this is also for a future PR. My redmine instance has custom fields on issues, for example we use a field called "GitHub Branch / PR" where one can paste in the URL to the relevant branch/PR on GitHub that relates to a particular Redmine issue.

This comes across in the issues.json like so:

      "custom_fields": [
        {
          "id": 12,
          "name": "GitHub Branch / PR",
          "value": "https://github.com/ralphbean/bugwarrior/pull/395"
        }
      ],

I'm not sure I see from the field templates how I could set up my configuration to write the value of this field to a new UDA.

@ryneeverett
Copy link
Collaborator

Gotcha, I wasn't sure what you meant by "fields". I would think the "custom fields" functionality you're talking about should be implemented as a new Common Service Configuration Option, right?

@ryneeverett ryneeverett merged commit 7ff491e into GothenburgBitFactory:develop Jan 29, 2017
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