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

feat: Allow for the deletion of any document #711

Merged
merged 20 commits into from
Dec 6, 2023

Conversation

ksushant6566
Copy link
Contributor

@ksushant6566 ksushant6566 commented Dec 2, 2023

Resolves #696

Copy link

vercel bot commented Dec 2, 2023

@ksushant6566 is attempting to deploy a commit to the Documenso Team Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Contributor

coderabbitai bot commented Dec 2, 2023

Important

Auto Review Skipped

Auto reviews are limited to the following labels: coderabbit. Please add one of these labels to enable auto reviews.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository.

To trigger a single review, invoke the @coderabbitai review command.


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.
  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

@ksushant6566
Copy link
Contributor Author

Hey @Mythie , just pushed the requested changes please have a look

@Mythie
Copy link
Collaborator

Mythie commented Dec 3, 2023

I think this is mostly missing interactions with what happens when a signer visits the signing page for a deleted document etc.

If a document was deleted do we redirect them? Show an error page? If we do either of those what do we do for completed documents with a valid token, should we just show the normal completion page?

@ksushant6566
Copy link
Contributor Author

I think this is mostly missing interactions with what happens when a signer visits the signing page for a deleted document etc.

If a document was deleted do we redirect them? Show an error page? If we do either of those what do we do for completed documents with a valid token, should we just show the normal completion page?

I think for completed documents we can just show the normal page & for pending documents we can disable actions and show a message that this document is no longer available to sign

wdyt?

@Mythie
Copy link
Collaborator

Mythie commented Dec 3, 2023

I think this is mostly missing interactions with what happens when a signer visits the signing page for a deleted document etc.
If a document was deleted do we redirect them? Show an error page? If we do either of those what do we do for completed documents with a valid token, should we just show the normal completion page?

I think for completed documents we can just show the normal page & for pending documents we can disable actions and show a message that this document is no longer available to sign

wdyt?

That sounds about right, if a pending document was deleted we can show them a screen letting them know that the document was removed while a completed document should just send the user to the completed page if the landed on /sign/<token>.

@Mythie
Copy link
Collaborator

Mythie commented Dec 3, 2023

I'll do a round of UI review with this now as well, I've only been stinging you on code so far 👀

@Mythie
Copy link
Collaborator

Mythie commented Dec 3, 2023

This will also need some guards for recipients adding signatures when a document has since been deleted 😄

@Mythie
Copy link
Collaborator

Mythie commented Dec 3, 2023

I'll do a round of UI review with this now as well, I've only been stinging you on code so far 👀

UI review was fine, just make the delete button a destructive variant since it's a destructive action

@ksushant6566
Copy link
Contributor Author

ksushant6566 commented Dec 3, 2023

I'll do a round of UI review with this now as well, I've only been stinging you on code so far 👀

  1. when the recipient lands on a doc she hasn't signed but the doc was deleted:
image
  1. when the recipient lands on a pending doc she has signed but the doc was deleted

image

Lemme know if these are fine, I'll push the final changes

@ksushant6566
Copy link
Contributor Author

This will also need some guards for recipients adding signatures when a document has since been deleted 😄

Done, added a check in sign-field-with-token.ts .

@ksushant6566
Copy link
Contributor Author

@Mythie
Copy link
Collaborator

Mythie commented Dec 3, 2023

round 2

@ksushant6566
Copy link
Contributor Author

Yo, I believe everything works now, but still do confirm if the logic below makes sense:

What documents users can see:

own documents
All documents created by me, but not the ones I deleted.

received documents

Pending documents:

  • not deleted: Yes
  • deleted but signed: Yes
  • deleted, not signed (Inbox): No

Completed documents:

  • not deleted: Yes
  • deleted: Yes

Note: Only owners can delete documents, users can't delete received documents. That will involve lot more work. (Maybe open another issue?)

PS: thanks a lot for your help throughout 🙌

@Mythie
Copy link
Collaborator

Mythie commented Dec 4, 2023

This all sounds correct to me, going to run through a few rounds of testing before I hit the merge button :)

@bilalqv
Copy link
Contributor

bilalqv commented Dec 4, 2023

@ksushant6566 , you've done a great job.

@Mythie Mythie merged commit 2068d98 into documenso:main Dec 6, 2023
7 of 11 checks passed
@david-loe david-loe mentioned this pull request Dec 20, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DOC-372] Allow for the deletion of any document
5 participants