-
-
Notifications
You must be signed in to change notification settings - Fork 317
fix: Add an onCanceled callback to allow resolving edit and create APIs #3698
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
base: main
Are you sure you want to change the base?
fix: Add an onCanceled callback to allow resolving edit and create APIs #3698
Conversation
|
|
Thank you very much for this - I will test it soon... |
claremacrae
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks for looking at this.
My concern about this PR is the following test:
obsidian-tasks/tests/Api/createTaskLineModal.test.ts
Lines 60 to 71 in b3d753b
| /** | |
| * If the Modal is cancelled, the api function createTaskLineModal() should return an empty string | |
| */ | |
| it('should return an empty string if cancelled', async () => { | |
| const taskLinePromise = createTaskLineModal(app, []); | |
| const expected = ''; | |
| TaskModal.instance.cancel(); | |
| const result = await taskLinePromise; | |
| expect(result).toEqual(expected); | |
| }); |
See #3656 (comment).
Since that test didn't fail before the fix, and doesn't change in behaviour with this PR, it appears to not be a useful test - and therefore is not protecting us against future breakages of the Cancel feature, perhaps?
|
I was thinking about that too. The issue was the Mock just called onSubmit without checking the length of the return value. Let me verify that my new change fixes that and it would fail with the new mock code or add that check in so the mock is accurate. Thx @claremacrae |
|
Hi @justise, I'm going to be doing a release fairly soon, so if you had time to look at the test, it would really help. If you can't, just let me know and I will merge anyway, as it's clearly an improvement. |
|
Sorry, I got distracted with some beginning of the year stuff and holiday break. Haven't had a chance to circle back on this yet. |



Types of changes
Changes visible to users:
feat- non-breaking change which adds functionality)feat!!orfix!!- fix or feature that would cause existing functionality to not work as expected)fix- non-breaking change which fixes an issue)i18n- additions or improvements to the translations - see Support a new language)docs- improvements to any documentation content for users)vault- improvements to the Tasks-Demo sample vault)contrib- any improvements to documentation content for contributors - see Contributing to Tasks)Internal changes:
refactor- non-breaking change which only improves the design or structure of existing code, and making no changes to its external behaviour)test- additions and improvements to unit tests and the smoke tests)chore- examples include GitHub Actions, issue templates)Description
An earlier PR introduced a change to the TaskModal which only submitted when there were any task updates.
This means that the API calls were not resolving when you cancel.
The solution is to add an onCancel callback so we can resolve the API functions even on cancel.
The change was simple enough I did not break it up into multiple changes (some AI support was leveraged as well).
Motivation and Context
Without this bug fix, usage of the tasks API functions will result in bugs as canceling will leave the program with unresolved promises.
Addresses Issue: #3656
How has this been tested?
Manual using the linked plugin
Updated Unit tests
Screenshots (if appropriate)
n/a
Checklist
yarn run lint.Terms