search

christianrondeau / jira-toggl-sync

A simple tool to synchronize Toggl time entries into Atlassian JIRA's work log entries
9 stars 6 forks source link

Option to purge jira #10

Closed vipetrul closed 7 years ago

vipetrul commented 7 years ago

This is for #8

I had to refactor entire pipeline to allow for better composition and testing. Added tests for key processes.

Notes about tests: Replaced MS Tests with NUnit v3 - which allows to write test cases much easier. Added dependency for FluentAssertions - which allows much easier to compare complex objects (among many other features it supports)

Notes about sync process: It appears that when toggl time is very small and it is rounded to zero, then JIRA Service will not be able to save it - this is something we need to be aware of.

christianrondeau commented 7 years ago

Hi! I see you've been hard at work :) Usually, big refactorings like this are frowned upon, but it looks like things I would have done and technology I usually prefer, so I'm good with it :)

You'll want however to watch for tabs v.s. spaces (this project use tabs) - if you use Visual Studio 2017, you can create a .editorconfig file so that you don't have to change your environment (e.g. https://github.com/jawa-the-hutt/lz-string-csharp/blob/master/.editorconfig though this one uses tabs)

I'll code review this as soon as I can (let me know if you plan on doing further structural changes), but this looks pretty good at a glance! I appreciate the new tests, this will make my job easier.

vipetrul commented 7 years ago

You'll want however to watch for tabs v.s. spaces (this project use tabs) - if you use Visual Studio 2017, you can create a .editorconfig file so that you don't have to change your environment (e.g. https://github.com/jawa-the-hutt/lz-string-csharp/blob/master/.editorconfig though this one uses tabs)

Thank you - I saw this when I was comparing new vs old files. Now I know how to control this per project.

let me know if you plan on doing further structural changes

I have nothing new planned - just need to determine what causing CI to throw System.BadImageFormatException: Could not load file or assembly 'Toggl, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null' or one of its dependencies. An attempt was made to load a program with an incorr . Probably something to do with 32 vs 64 bit architecture...

vipetrul commented 7 years ago

I believe there is a problem with current PR: Since toggl (source) returns only entries for specific user, and jira returns entries for all users - purge will delete all entries that were created by others users.

I need to think about this ...

vipetrul commented 7 years ago

so we need to make a determination:

What do you think?

christianrondeau commented 7 years ago

I would definitely go with only syncing your own entries. Since Toggl's worklogs are user-specific anyway, it would not make sense to modify other users entries in JIRA.

vipetrul commented 7 years ago

Sounds good, i was thinking along the same lines.

On Jun 30, 2017 7:02 PM, "Christian Rondeau" notifications@github.com wrote:

I would definitely go with only syncing your own entries. Since Toggl's worklogs are user-specific anyway, it would not make sense to modify other users entries in JIRA.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/christianrondeau/jira-toggl-sync/pull/10#issuecomment-312396641, or mute the thread https://github.com/notifications/unsubscribe-auth/AB6d2xLijREd_1rYsWZi8wdcpYBqX5wgks5sJYylgaJpZM4OK0JJ .

vipetrul commented 7 years ago

Today I showed this tool to my coworkers that had to sync toggl time entries from one of the projects. It worked pretty well! One suggestion that came out of it - (y/n) questions should not automatically execute when key is pressed. What was happening, is that they were pressing y then Enter which executed two commands y then n (i.e. key != 'y').

What are your thoughts on this?

vipetrul commented 7 years ago

Bugs found:

I will be working on them.

christianrondeau commented 7 years ago

The whole y/n flow feels weird. I'm a Vim user, so pressing Enter after a command seems to much :) Here's what I'd like (this would probably be another issue though)

That would be something using e.g. ncurses (https://github.com/sushihangover/CursesSharp).

But that's way out of this issue :) My point being that "y" and "enter" makes twice the keypresses...

The "negative" impact of pressing "y" is pretty minimal, so as long as it's clear, I think it's okay.

As for the first bug, I'd be curious how you'll "fix" this... if it was synced with another JIRA number, I guess it's normal it's not finding it. Unless you can get a "description history" for the Toggl entry, I think that's fine.

For the second bug, you're right, it should either stay unchanged for both, or update for both (I prefer the former).

vipetrul commented 7 years ago

I believe first bug was resolved by my previous commit, corresponding test scenario was added as well. Tomorrow I will be looking into JIRA API documentation in order to solve 2nd problem.

Please open a separate issue to discuss this, and keep the PR lean :)

Will do 👍

vipetrul commented 7 years ago

When orphaned work log entries were deleted from JIRA, remaining time stayed unchanged. When new entries were added to JIRA, remaining time increased based on entered new value.

My initial assessment was wrong. It appears user who manually edited work logs in jira made wrong adjustments in time remaining.

Please proceed with review, I'm not planning to modifying anything else at this moment.

christianrondeau commented 7 years ago

Thanks. Might not be immediate, I have some major projects going on, but I'll do my best :)

dstj commented 7 years ago

I'll be testing this pull request this week as I enter my work log. Will let you know my findings.

dstj commented 7 years ago

Hi,

To be able to compile, I had to update to RestSharp, Version=105.2.3.0 (form 104.1) in the JiraRestClient submodules.

My first test (sync 2 days of work logs) was very slow. The first two lines of WorksheetSyncService.Synchronize() took around 2+ minutes to complete.

public SyncReport Syncronize(DateTime fromDate, DateTime toDate, IEnumerable<string> jiraProjectKeys, bool doPurge, int roundMinutes)
{
    var sourceEntries = _source.GetEntries(fromDate, toDate, jiraProjectKeys);
    var targetEntries = _target.GetEntries(fromDate, toDate, jiraProjectKeys);
vipetrul commented 7 years ago

I suspect that var targetEntries = _target.GetEntries(fromDate, toDate, jiraProjectKeys); took the longest to complete, because it recursively retrieves workLog entries for each updated JIRA item. Although in our environment this operation just takes ~5 seconds, I can see how it can be much slower on other environments. How many projects are you trying to sync? and approx how many time entries?

Note. I just pushed a small change to JiraRestService class, make sure you pull it when you test.

If we cannot find a better way to retrieve workLog entries from target source, then we can consider performing these operations async to boost performance.

vipetrul commented 7 years ago

@dstj , you right - if there are many projects to sync, getting all workLog entries from source (JIRA) will take a while. I did update current implementation of JiraRepository.GetEntries to run asynchronously, which cuts the execution time approx in half.

To expedite the process even further, we need to use a more efficient way to retrieve work log entries form JIRA. JIRA API does have an endpoint to Get ids of worklogs modified since , but our current JIRA service doesn't have it implemented.

christianrondeau commented 7 years ago

I guess we can just make the JSON calls directly then. The library was a shortcut, but if it's creating more trouble than necessary...

This being said, apart from being slow, if it works and it can be turned off without negative impact on performance, I'll accept the PR anyway, since it works for "smaller projects" :) @dstj did you try disabling purge? If it works for you, I'll just accept it: https://github.com/christianrondeau/jira-toggl-sync/pull/10/commits/f0bc59bbebf7a6a37b237c32d17159aad7f23755#diff-cb919c6f2f938d79bd41d95c5e57ea13R56

If you feel like making another PR for the speed, feel free :) I have no emotional attachment to the toggl and the jira librairies I use today ;)

vipetrul commented 7 years ago

@christianrondeau , you are missing couple latest commits. Agree that performance can be addressed in separate PR. If purge is disabled, then performance should be same as before, since no records from source are being retrieved.

christianrondeau commented 7 years ago

Sounds good to me. Let's close this then, we can always improve later. Thank you @vipetrul for your contribution, and I'm glad to know the tool is useful to you :)