add a command to edit existing worklogs #46

Vivec1990 · 2020-12-17T19:39:34Z

This should resolve #45
The workaround to changing the associated issue is to delete the existing worklog and create a new one with the given configuration.

… assigned issue key is to be changed

szymonkozak

Thanks for the contribution! :)

Could you also add tests for this feature?

Also, please run npm run lint (this fixes some lint issues and shows you also errors that you have to fix manually).

szymonkozak · 2020-12-17T20:41:07Z

src/commands/edit.ts

+
+    static examples = [
+        `${appName} edit 123456 -d "new desc"`,
+        `${appName} e 123456 -i ISSUE_KEY_OR_ALIAS`,


This should be rather something like ${appName} e 123456 -i abc-123 as in other examples.

szymonkozak · 2020-12-17T20:49:30Z

src/commands/edit.ts

+    static examples = [
+        `${appName} edit 123456 -d "new desc"`,
+        `${appName} e 123456 -i ISSUE_KEY_OR_ALIAS`,
+        `${appName} e 123456 -t 13-15:30 `


I would add more examples (with remaining flags).

szymonkozak · 2020-12-17T20:54:49Z

src/tempo.ts

+        return execute(async () => {
+            cli.action.start(`Updating worklog with id ${worklog_id}`)
+            const worklog = await worklogs.updateWorklog(worklog_id, input)
+            cli.action.stop('Done.')


What do you think about printing also worklog id here? (this value may have been changed which is not obvious)

Indeed it should, it seems like I forgot to add it.
Would the message be as in the addWorklog method, to include how to delete the, possibly changed, worklog?

Would the message be as in the addWorklog method, to include how to delete the, possibly changed, worklog?

Yes.

szymonkozak · 2020-12-17T20:57:57Z

src/worklogs/worklogs.ts

@@ -95,7 +98,50 @@ export default {
            credentials.accountId
        )
        return { worklogs, date, scheduleDetails }
+    },
+
+    async updateWorklog(worklogIdInput: string, input: UpdateWorklogInput) {


worklogIdInput should be the part of UpdateWorklogInput type.

szymonkozak · 2020-12-17T21:02:08Z

src/worklogs/worklogs.ts

@@ -95,7 +98,50 @@ export default {
            credentials.accountId
        )
        return { worklogs, date, scheduleDetails }
+    },
+
+    async updateWorklog(worklogIdInput: string, input: UpdateWorklogInput) {


First, you should call await checkToken() to check if token is properly set. See the other methods in this scope.

szymonkozak · 2020-12-17T21:30:37Z

src/worklogs/worklogs.ts

+            throw Error('Error. Worklog id should be an integer number.')
+        }
+        const currentWorklog = await api.getWorklog(worklogId)
+        const referenceDate = parseWhenArg(time.now(), input.when ? input.when : currentWorklog.startDate);


input.when ? input.when : currentWorklog.startDate
can be replaced with
input.when ?? currentWorklog.startDate

szymonkozak · 2020-12-17T21:30:57Z

src/worklogs/worklogs.ts

+            parseResult = parseInputDate(input.durationOrInterval, referenceDate)
+        }
+
+        const issueKey = input.issueKeyOrAlias ? input.issueKeyOrAlias : currentWorklog.issue.key;


As above, I would use ?? here.

szymonkozak · 2020-12-17T21:31:24Z

src/worklogs/worklogs.ts

+            startDate: format(referenceDate, DATE_FORMAT),
+            startTime: parseResult ? startTime(parseResult, input.startTime, referenceDate) : currentWorklog.startTime,
+            description: input.description ? input.description : currentWorklog.description,
+            remainingEstimateSeconds: remainingEstimateSeconds(referenceDate, input.remainingEstimate)


Why we don't use remaining estimate from current worklog?

Setting the remaining estimate seems to be optional, which would mean not setting it results in it not being changed.
But it could be done explicitly to make it clearer, i'll see how it behaves now.

Setting the remaining estimate seems to be optional, which would mean not setting it results in it not being changed.

But technically if the issueKey was changed, we will create a completely new worklog, so we have to move all things from the old to the new one, including optional parameters.

Correct me if I am wrong.

szymonkozak · 2020-12-17T21:36:50Z

src/worklogs/worklogs.ts

+            timeSpentSeconds: parseResult ? parseResult.seconds : currentWorklog.timeSpentSeconds,
+            startDate: format(referenceDate, DATE_FORMAT),
+            startTime: parseResult ? startTime(parseResult, input.startTime, referenceDate) : currentWorklog.startTime,
+            description: input.description ? input.description : currentWorklog.description,


As above (using ??)

szymonkozak · 2020-12-17T21:37:51Z

src/worklogs/worklogs.ts

+        }
+        const currentWorklog = await api.getWorklog(worklogId)
+        const referenceDate = parseWhenArg(time.now(), input.when ? input.when : currentWorklog.startDate);
+        let parseResult = undefined


We don't need to declare variable as undefined explicitly.

szymonkozak

Comments above.

MiSikora

I left some nitpick formatting issues.

But this code needs to be covered with some acceptance tests. Scenarios that come to my mind.

Worklog parameters can be explicitly overridden.
Worklog parameters are not implicitly overriden.
Missing worklog parameters can be set.
Not deleting worklog if it failed to be updated.
Deleting worklog if it updated succesfully.
User is informed that worklog did update but the old one failed to be deleted.

If you wish to add unit tests for some of the code it is also cool but I don't think it is necessary.

src/api/api.ts

src/commands/edit.ts

src/worklogs/worklogs.ts

szymonkozak · 2020-12-18T11:52:23Z

@MiSikora Formatting issues can be fixed automatically with running npm run lint.

szymonkozak · 2021-05-30T22:27:49Z

@Vivec1990 Are you going to finish implementing this or should I close this PR?

szymonkozak · 2021-06-07T21:49:38Z

I am closing this due to a lack of activity.

Vivec1990 added 4 commits December 16, 2020 16:56

add a command to edit an existing worklog

c7429e8

remove unneeded duplicate request type

8f942d5

fix edit functionality to delete existing and create a new one if the…

c5c6751

… assigned issue key is to be changed

remove duplicated code fragment

27aacbb

szymonkozak reviewed Dec 17, 2020

View reviewed changes

szymonkozak requested changes Dec 17, 2020

View reviewed changes

MiSikora requested changes Dec 18, 2020

View reviewed changes

src/api/api.ts Show resolved Hide resolved

src/commands/edit.ts Show resolved Hide resolved

src/commands/edit.ts Show resolved Hide resolved

src/worklogs/worklogs.ts Show resolved Hide resolved

src/worklogs/worklogs.ts Show resolved Hide resolved

szymonkozak closed this Jun 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add a command to edit existing worklogs #46

add a command to edit existing worklogs #46

Vivec1990 commented Dec 17, 2020

szymonkozak left a comment •

edited

Loading

szymonkozak Dec 17, 2020

szymonkozak Dec 17, 2020

szymonkozak Dec 17, 2020

Vivec1990 Dec 18, 2020

szymonkozak Dec 18, 2020

szymonkozak Dec 17, 2020

szymonkozak Dec 17, 2020

szymonkozak Dec 17, 2020

szymonkozak Dec 17, 2020

szymonkozak Dec 17, 2020

Vivec1990 Dec 18, 2020

szymonkozak Dec 18, 2020

szymonkozak Dec 17, 2020

szymonkozak Dec 17, 2020

szymonkozak left a comment

MiSikora left a comment •

edited

Loading

szymonkozak commented Dec 18, 2020

szymonkozak commented May 30, 2021

szymonkozak commented Jun 7, 2021

add a command to edit existing worklogs #46

add a command to edit existing worklogs #46

Conversation

Vivec1990 commented Dec 17, 2020

szymonkozak left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szymonkozak left a comment

Choose a reason for hiding this comment

MiSikora left a comment • edited Loading

Choose a reason for hiding this comment

szymonkozak commented Dec 18, 2020

szymonkozak commented May 30, 2021

szymonkozak commented Jun 7, 2021

szymonkozak left a comment •

edited

Loading

MiSikora left a comment •

edited

Loading