1
\$\begingroup\$

In the JIRA add-on I have a business service to encapsulate creating/update and deleting worklogs (for time tracking on issues).

The service calls JIRA through its API and throws and error if the issue for the worklog is closed. In JIRA it is possible to override the security check for create with WorklogInputParametersImpl.editableCheckRequired but not for update/delete.

In my service I want to always ignore this JIRA check. My workaround is to update the status of the issue to Editable if it is not editable, do the operation and then finally update the status back.

The current implementation WorklogUpdateManagerImpl is already hard to unit test with 5 dependencies. Adding the functionality there does not feel appealing to me, hard to unit test, hard to remove functionality, break SRP?, adding more complexity that might be removed or changed.

I decided to go with a decorator to keep the original implementation as is and enhance the implementation with a decorator that makes the issue Editable if needed and reverts it.

Dictionary:

  • Retain = Update the worklog and keep the issue remaining estimate as is
  • Auto adjust = Update the worklog and auto adjust the issue remaining estimate according to logged time
  • New Remaining = Update the worklog and override the issue remaining estimate

I would like to get feedback on:

  1. The decision of the use of the decorator pattern
  2. The use of multiple parameters in each method
  3. Code duplication in the original implementation

The interface

package com.myplugin.worklog;

import com.atlassian.jira.issue.worklog.Worklog;
import org.joda.time.LocalDateTime;

public interface WorklogUpdateManager {

    Worklog createAndRetainRemainingEstimate(long issueId, String workerKey, LocalDateTime startDate, String comment, long timeSpentSeconds);

    Worklog createAndAutoAdjustRemainingEstimate(long issueId, String workerKey, LocalDateTime startDate, String comment, long timeSpentSeconds);

    Worklog createWithNewRemainingEstimate(long issueId, String workerKey, LocalDateTime startDate, String comment, long timeSpentSeconds, long remainingEstimate);

    Worklog updateAndRetainRemainingEstimate(long worklogId, LocalDateTime startDate, String comment, long timeSpentSeconds);

    Worklog updateAndAutoAdjustRemainingEstimate(long worklogId, LocalDateTime startDate, String comment, long timeSpentSeconds);

    Worklog updateWithNewRemainingEstimate(long worklogId, LocalDateTime startDate, String comment, long timeSpentSeconds, long remainingEstimate);

    void deleteAndRetainRemainingEstimate(long worklogId);

    void deleteAndAutoAdjustRemainingEstimate(long worklogId);
}

The Decorator

package com.myplugin.worklog;

import com.atlassian.jira.issue.Issue;
import com.atlassian.jira.issue.worklog.Worklog;
import com.atlassian.jira.issue.worklog.WorklogManager;
import com.myplugin.issue.service.IssueService;
import com.myplugin.service.workflow.workflowService;
import com.myplugin.service.workflow.workflowServiceImpl.PreviousStateHolder;
import org.joda.time.LocalDateTime;

public class WorklogUpdateManagerAllowNotEditable implements WorklogUpdateManager {

    private final WorklogUpdateManager worklogUpdateManager;
    private final IssueService issueService;
    private final WorklogManager worklogManager;
    private final WorkflowService workflowService;

    public WorklogUpdateManagerAllowNotEditable(WorklogUpdateManager worklogUpdateManager,
                                                 IssueService issueService,
                                                 WorklogManager worklogManager,
                                                 WorkflowService workflowService) {
        this.worklogUpdateManager = worklogUpdateManager;
        this.issueService = issueService;
        this.worklogManager = worklogManager;
        this.workflowService = workflowService;
    }

    // NOTE: Would be very nice if these parameters were objects instead of objects, easier to decorate

    @Override
    public Worklog createAndRetainRemainingEstimate(long issueId,
                                                    String workerKey,
                                                    LocalDateTime startDate,
                                                    String comment,
                                                    long timeSpentSeconds) {

        return doIssueInEditableState(issueId,
         () -> worklogUpdateManager.createAndRetainRemainingEstimate(issueId,
          workerKey,
          startDate,
          comment,
          timeSpentSeconds));
    }

    @Override
    public Worklog createAndAutoAdjustRemainingEstimate(long issueId,
                                                        String workerKey,
                                                        LocalDateTime startDate,
                                                        String comment,
                                                        long timeSpentSeconds) {
        return doIssueInEditableState(issueId,
         () -> worklogUpdateManager.createAndAutoAdjustRemainingEstimate(issueId,
          workerKey,
          startDate,
          comment,
          timeSpentSeconds));
    }

    @Override
    public Worklog createWithNewRemainingEstimate(long issueId,
                                                  String workerKey,
                                                  LocalDateTime startDate,
                                                  String comment,
                                                  long timeSpentSeconds,
                                                  long remainingEstimate) {
        return doIssueInEditableState(issueId,
         () -> worklogUpdateManager.createWithNewRemainingEstimate(issueId,
          workerKey,
          startDate,
          comment,
          timeSpentSeconds,
          remainingEstimate));
    }

    @Override
    public Worklog updateAndRetainRemainingEstimate(long worklogId,
                                                    LocalDateTime startDate,
                                                    String comment,
                                                    long timeSpentSeconds) {
        return doWorklogIssueInEditableState(worklogId,
         () -> worklogUpdateManager.updateAndRetainRemainingEstimate(worklogId,
          startDate,
          comment,
          timeSpentSeconds));
    }

    @Override
    public Worklog updateAndAutoAdjustRemainingEstimate(long worklogId,
                                                        LocalDateTime startDate,
                                                        String comment,
                                                        long timeSpentSeconds) {
        return doWorklogIssueInEditableState(worklogId,
         () -> worklogUpdateManager.updateAndAutoAdjustRemainingEstimate(worklogId,
          startDate,
          comment,
          timeSpentSeconds));
    }

    @Override
    public Worklog updateWithNewRemainingEstimate(long worklogId,
                                                  LocalDateTime startDate,
                                                  String comment,
                                                  long timeSpentSeconds,
                                                  long remainingEstimate) {
        return doWorklogIssueInEditableState(worklogId,
         () -> worklogUpdateManager.updateWithNewRemainingEstimate(worklogId,
          startDate,
          comment,
          timeSpentSeconds,
          remainingEstimate));
    }

    @Override
    public void deleteAndRetainRemainingEstimate(long worklogId) {
        doWorklogIssueInEditableState(worklogId,
         () -> {
             worklogUpdateManager.deleteAndRetainRemainingEstimate(worklogId);
             return null;
         });
    }

    @Override
    public void deleteAndAutoAdjustRemainingEstimate(long worklogId) {
        doWorklogIssueInEditableState(worklogId,
         () -> {
             worklogUpdateManager.deleteAndAutoAdjustRemainingEstimate(worklogId);
             return null;
         });
    }

    private <T> T doWorklogIssueInEditableState(long worklogId, CommandWithResult<T> command) {
        return doIssueInEditableState(getIssueForWorklog(worklogId).getId(), command);
    }

    private <T> T doIssueInEditableState(long issueId, CommandWithResult<T> command) {
        PreviousStateHolder previousStateHolder = workflowService.ensureIssueIsEditable(getIssue(issueId));

        try {
            return command.execute();
        } finally {
            // NOTE: Must fetch the issue again to reflect the new state
            workflowService.revertIssueStatus(getIssue(issueId), previousStateHolder);
        }
    }

    private interface CommandWithResult<T> {
        T execute();
    }

    private Issue getIssue(long issueId) {
        return issueService.getIssue(issueId);
    }

    private Issue getIssueForWorklog(long worklogId) {
        return worklogManager.getById(worklogId).getIssue();
    }
}

The original implementation

package com.myplugin.worklog;

import com.atlassian.jira.bc.issue.worklog.TimeTrackingConfiguration;
import com.atlassian.jira.bc.issue.worklog.WorklogAdjustmentAmountInputParameters;
import com.atlassian.jira.bc.issue.worklog.WorklogInputParameters;
import com.atlassian.jira.bc.issue.worklog.WorklogInputParametersImpl;
import com.atlassian.jira.bc.issue.worklog.WorklogNewEstimateInputParameters;
import com.atlassian.jira.bc.issue.worklog.WorklogNewEstimateResult;
import com.atlassian.jira.bc.issue.worklog.WorklogResult;
import com.atlassian.jira.issue.worklog.Worklog;
import com.atlassian.jira.issue.worklog.WorklogManager;
import com.myplugin.issue.service.IssueService;
import com.myplugin.platform.api.user.UserAuthenticationContext;
import org.joda.time.LocalDateTime;
import org.springframework.beans.factory.annotation.Autowired;

public class WorklogUpdateManagerImpl implements WorklogUpdateManager {

    private final WorklogServiceContextFactory worklogServiceContextFactory;
    private final WorklogManager worklogManager;
    private final IssueService issueService;
    private final UserAuthenticationContext userAuthenticationContext;
    private final TimeTrackingConfiguration timeTrackingConfiguration;

    public WorklogUpdateManagerImpl(WorklogServiceContextFactory worklogServiceContextFactory,
                                      WorklogManager worklogManager,
                                      IssueService issueService,
                                      UserAuthenticationContext userAuthenticationContext,
                                      TimeTrackingConfiguration timeTrackingConfiguration) {
        this.worklogServiceContextFactory = worklogServiceContextFactory;
        this.worklogManager = worklogManager;
        this.issueService = issueService;
        this.userAuthenticationContext = userAuthenticationContext;
        this.timeTrackingConfiguration = timeTrackingConfiguration;
    }

    @Override
    public Worklog createAndRetainRemainingEstimate(long issueId, String workerKey, LocalDateTime startDate, String comment, long timeSpentSeconds) {
        WorklogServiceContext worklogServiceContext = createWorklogServiceContextForWorker(workerKey);

        WorklogInputParameters inputParameters = WorklogInputParametersImpl
                .issue(issueService.getIssue(issueId))
                .timeSpent(formatDurationForInput(timeSpentSeconds))
                .startDate(startDate.toDate())
                .comment(comment)
                .build();
        WorklogResult worklogResult = worklogServiceContext.validateCreate(inputParameters);

        return worklogServiceContext.createAndRetainRemainingEstimate(worklogResult);
    }

    @Override
    public Worklog createAndAutoAdjustRemainingEstimate(long issueId, String workerKey, LocalDateTime startDate, String comment, long timeSpentSeconds) {
        WorklogServiceContext worklogServiceContext = createWorklogServiceContextForWorker(workerKey);

        WorklogAdjustmentAmountInputParameters inputParameters = WorklogInputParametersImpl
                .issue(issueService.getIssue(issueId))
                .timeSpent(formatDurationForInput(timeSpentSeconds))
                .startDate(startDate.toDate())
                .comment(comment)
                .buildAdjustmentAmount();
        WorklogResult worklogResult = worklogServiceContext.validateCreate(inputParameters);

        return worklogServiceContext.createAndAutoAdjustRemainingEstimate(worklogResult);
    }

    @Override
    public Worklog createWithNewRemainingEstimate(long issueId, String workerKey, LocalDateTime startDate, String comment, long timeSpentSeconds, long remainingEstimate) {
        WorklogServiceContext worklogServiceContext = createWorklogServiceContextForWorker(workerKey);

        WorklogNewEstimateInputParameters inputParameters = WorklogInputParametersImpl
                .issue(issueService.getIssue(issueId))
                .timeSpent(formatDurationForInput(timeSpentSeconds))
                .startDate(startDate.toDate())
                .comment(comment)
                .newEstimate(DurationFormatter.fromSeconds(remainingEstimate))
                .buildNewEstimate();

        WorklogNewEstimateResult worklogNewEstimateResult = worklogServiceContext.validateCreateWithNewEstimate(inputParameters);

       return worklogServiceContext.createWithNewRemainingEstimate(worklogNewEstimateResult);
    }

    @Override
    public Worklog updateAndRetainRemainingEstimate(long worklogId, LocalDateTime startDate, String comment, long timeSpentSeconds) {
        WorklogServiceContext worklogServiceContext = createWorklogServiceContextForWorklog(worklogId);

        Worklog worklog = worklogManager.getById(worklogId);
        WorklogInputParameters inputParameters = WorklogInputParametersImpl
                .timeSpent(formatDurationForInput(timeSpentSeconds))
                .worklogId(worklog.getId())
                .startDate(startDate.toDate())
                .comment(comment)
                .issue(worklog.getIssue())
                .build();
        WorklogResult worklogResult = worklogServiceContext.validateUpdate( inputParameters);

        return worklogServiceContext.updateAndRetainRemainingEstimate(worklogResult);
    }

    @Override
    public Worklog updateAndAutoAdjustRemainingEstimate(long worklogId, LocalDateTime startDate, String comment, long timeSpentSeconds) {
        WorklogServiceContext worklogServiceContext = createWorklogServiceContextForWorklog(worklogId);

        Worklog worklog = worklogManager.getById(worklogId);
        WorklogAdjustmentAmountInputParameters inputParameters = WorklogInputParametersImpl
                .timeSpent(formatDurationForInput(timeSpentSeconds))
                .worklogId(worklog.getId())
                .startDate(startDate.toDate())
                .comment(comment)
                .issue(worklog.getIssue())
                .buildAdjustmentAmount();
        WorklogResult worklogResult = worklogServiceContext.validateUpdate(inputParameters);

        return worklogServiceContext.updateAndAutoAdjustRemainingEstimate(worklogResult);
    }

    @Override
    public Worklog updateWithNewRemainingEstimate(long worklogId, LocalDateTime startDate, String comment, long timeSpentSeconds, long remainingEstimate) {
        WorklogServiceContext worklogServiceContext = createWorklogServiceContextForWorklog(worklogId);

        Worklog worklog = worklogManager.getById(worklogId);
        WorklogNewEstimateInputParameters inputParameters = WorklogInputParametersImpl
                .timeSpent(formatDurationForInput(timeSpentSeconds))
                .worklogId(worklog.getId())
                .startDate(startDate.toDate())
                .comment(comment)
                .issue(worklog.getIssue())
                .newEstimate(DurationFormatter.fromSeconds(remainingEstimate)) // NOTE: Why update timespentseconds differently
                .buildNewEstimate();

        WorklogNewEstimateResult worklogNewEstimateResult = worklogServiceContext.validateUpdateWithNewEstimate(inputParameters);

        return worklogServiceContext.updateWithNewRemainingEstimate(worklogNewEstimateResult);
    }

    @Override
    public void deleteAndRetainRemainingEstimate(long worklogId) {
        WorklogServiceContext worklogServiceContext = createWorklogServiceContextForWorklog(worklogId);

        WorklogResult worklogResult = worklogServiceContext.validateDelete(worklogId);

        worklogServiceContext.deleteAndRetainRemainingEstimate(worklogResult);
    }

    @Override
    public void deleteAndAutoAdjustRemainingEstimate(long worklogId) {
        WorklogServiceContext worklogServiceContext = createWorklogServiceContextForWorklog(worklogId);

        WorklogResult worklogResult = worklogServiceContext.validateDelete(worklogId);

        worklogServiceContext.deleteAndAutoAdjustRemainingEstimate(worklogResult);
    }

    private WorklogServiceContext createWorklogServiceContextForWorker(String workerKey) {
        return worklogServiceContextFactory.createForWorker(workerKey);
    }

    private WorklogServiceContext createWorklogServiceContextForWorklog(long worklogId) {
        return worklogServiceContextFactory.createForWorklog(worklogId);

    }

    private String formatDurationForInput(long seconds) {
        return DurationFormatter.fromSecondsForInput(seconds, timeTrackingConfiguration.getDefaultUnit(), userAuthenticationContext.getAuthenticatedUser().getLocale());
    }
}
\$\endgroup\$
1
\$\begingroup\$

Well, from a design pattern perspective, the decorator's intent is

  1. Change behaviour during runtime
  2. Decorators provide a flexible alternative to subclassing for extending functionality.

(http://wiki.c2.com/?DecoratorPattern)

The first one doesn't match, so that's an argument against decorator.

You say, you decied not to modify your existing service, because it's "hard to unit test", with all the dependencies. In my opinion, the usage of the decorator pattern makes it more complex. In your case, you only have two actual implementations (original / extension), that wouldn't be too complicated, but if you have just one more implementation, it will be horrible, because A can composite B, B can composite C, and so on. So it's very hard, just to understand how a specific composition works, especially in your case, when states of an object will be changed. If you're argument will be, "well, it's only two implementations", that will be an argument against decorator, too.

What I find amusing: You decided to use decorator, because it's hard to test. Well, if something is hard to test, it's usually a sign of a design problem. Or bad test code. At first glance, it doesn't really seem very complicated - if we're talking about unit tests, and not component tests. I mean, most of the methods just call other services. When I look at createAndRetainRemainingEstimate, the things that have to be tested, in my opinion:

  • worklogServiceContextFactory.createForWorker(workerKey);Has to be called with the given workerKey
  • worklogServiceContext.validateCreate(inputParameters);: Has to be called with the correct inputParameters
  • worklogServiceContext.createAndRetainRemainingEstimate(worklogResult); Has to be called with the worklogResult, which was created by the worklogServiceContext.validateCreate() method - which will be mocked anway.

That's it, isn't it? That's three things, you have to verify for this class. All the other dependencies, well, their implementations, have to be unit tested in separate test cases anyway.

Potential improvement would be the WorkflowServiceContext, I think this can be decoupled to a separate service. Instead of creating a worklogServiceContext using the worker key, and then call it with the inputParameters, you could wrap validateCreate and createAndRetainRemainingEstimate in one method ("Tell, don't ask", https://martinfowler.com/bliki/TellDontAsk.html).

With that said: You might want to let us review your test code.

Hope this helps,

slowy

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ WorklogUpdateManagerImpl was implemented to solve a problem which it does. It is already a level of indirection hiding the complexities of talking to JIRA. Creating yet another level of abstraction just to be able to add logic does not make sense to me. According to the Open Closed principle you should be able to extend the functionality of code without modifying it. As your comment above nr. 2 seems to fit extending functionality. How else can we extend this functionality without changing the implementation? \$\endgroup\$ – Jakob Nov 30 '17 at 20:09
  • \$\begingroup\$ @Jakob Hm. No, the WorklogUpdateManagerImpl doesn't solve the problem (it can't change issues when they're closed). My opinion is, the extension belongs into the WorklogUpdateManager, because it can't do its job without that extension. So your requirement to extend the implementation without changing the existing one is a bit conflicting. And about open/closed: I'd say it's more important to extend your code without modyfing the code that's dependent on it. Full open/closed principle is applied by a plugin architecture, e.g. you extend intellij with a plugin, without changing intellij code. \$\endgroup\$ – slowy Dec 1 '17 at 12:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.