Present an Option to Confirm Send #538

ChayimFriedman2 · 2020-03-06T12:49:12Z

Solve issue #478.
I've chosen the following format:
Confirm with a custom message:

@confirm-send(Do you want to send?)
https://github.com

Confirm without a custom message, the default message is "Are you sure?" and it's customizable through setting:

@confirm-send()
https://github.com

Or:

@confirm-send
https://github.com

The command must be on the first line, without any spaces before or after it.

Add '$' at the regex end

I want to apply the confirmation also to CURL requests. The shared place between HttpRequestParser and CurlRequestParser is IRequestParser.

ChayimFriedman2 · 2020-03-17T08:12:42Z

What's happening? Do you not accept PRs?

Huachao · 2020-03-17T08:30:10Z

@ChayimFriedman2 actually PRs are warmly welcomed, however, I will consider this feature/bug fix acceptable to me. And I'd like to thank for your contribution and usage. Back to your PR, firstly, I think this feature is reasonable since we need to let users know that some APIs are dangerous operations and need to be careful. So we need to come up with a syntax for request that users think dangerous (not only DELETE request). Secondly, the syntax @confirm-send() is a bit ambiguous with the file variable definition. Maybe I prefer a syntax like request variable definition that inlined in comment lines and just above the request, like # @note. The name note is just an example. Thirdly, I think the users don't need to customize the message info in setting, if we follow step 2, the syntax can be extended to following

# @name deleteUser
# @note Don't do this until you are convinced
DELETE https://example.org/users/1

ChayimFriedman2 · 2020-03-17T09:11:41Z

I think this feature is reasonable since we need to let users know that some APIs are dangerous operations and need to be careful. So we need to come up with a syntax for request that users think dangerous (not only DELETE request)

What do you mean?

Huachao · 2020-03-17T09:15:38Z

I think this feature is reasonable since we need to let users know that some APIs are dangerous operations and need to be careful. So we need to come up with a syntax for request that users think dangerous (not only DELETE request)

What do you mean?

I mean this plan should be designed for all requests that users think need to be confirmed, not only for deletion confirmation.

@name

To simplify the process, I ignored the situation of invalid #@name in the syntax highlighting, so that `#@name Hi, this is me` will still be considered a valid name by the highlighter

ChayimFriedman2 · 2020-03-17T10:22:42Z

I'v completed the work on your suggestions :)

About your comment, in the first PR I already implemented it for every request type - not just DELETE.

Huachao

I add some comments and open for discussion.

Huachao · 2020-03-17T14:19:24Z

src/common/constants.ts

@@ -77,4 +77,6 @@ export const RequestVariableDefinitionWithNameRegexFactory = (name: string, flag

 export const RequestVariableDefinitionRegex: RegExp = RequestVariableDefinitionWithNameRegexFactory("\\w+", "m");

+export const DangerousNoteDefinitionRegex = /^\s*(?:#{1,}|\/{2,})\s+@note\s+(.+)\s*$/m;


I think the customized message is normally optional, even I am considering why we should expose this to user? My idea is if user put the note metadata, we always put the same message like Are you sure you want to send this request?. If the request has name(defined by @name), we should use its name like Are you sure you want to send this request {{requestName}}?

Huachao · 2020-03-17T14:20:58Z

src/common/constants.ts

@@ -77,4 +77,6 @@ export const RequestVariableDefinitionWithNameRegexFactory = (name: string, flag

 export const RequestVariableDefinitionRegex: RegExp = RequestVariableDefinitionWithNameRegexFactory("\\w+", "m");

+export const DangerousNoteDefinitionRegex = /^\s*(?:#{1,}|\/{2,})\s+@note\s+(.+)\s*$/m;


What do you think about the name note or confirmation, or any other ideas 😄

Huachao · 2020-03-17T16:08:50Z

src/controllers/requestController.ts

+        if (undefined !== httpRequest.confirmSendMsg) {
+            const userConfirmed = await window.showQuickPick(['Yes', 'No'], { canPickMany: false, placeHolder: httpRequest.confirmSendMsg });
+            if ('Yes' !== userConfirmed) {
+              return;
+            }
+        }


I think we should move this logic to the run method, since rerun method doesn't need to rely on this logic personally.

Huachao · 2020-03-17T16:12:25Z

src/models/httpRequest.ts

@@ -11,7 +11,8 @@ export class HttpRequest {
        public headers: RequestHeaders,
        public body: string | Stream | undefined,
        public rawBody: string | undefined,
-        public requestVariableCacheKey?: RequestVariableCacheKey) {
+        public requestVariableCacheKey?: RequestVariableCacheKey,
+        public confirmSendMsg?: string) {


The confirmSendMsg field is only used for request send confirmation, I think we don't need to put it in the request text.

And what's about serialization?

I think no, we don't need to show this info for history

src/controllers/requestController.ts

Do not require spaces after the '#' or '//'

Change 'bool' to 'boolean'

ChayimFriedman2 · 2020-03-25T05:37:55Z

Changes made.

src/common/constants.ts

src/utils/selector.ts

syntaxes/http.tmLanguage

Huachao · 2020-03-25T08:01:44Z

syntaxes/http.tmLanguage

@@ -124,7 +124,7 @@
            </dict>
            <dict>
                <key>match</key>
-                <string>^\s*#{1,}\s*(?:((@)name)\s+(\S+))?.*$</string>
+                <string>^\s*/{2,}\s*(?:((@)name)\s+(.+)|((@)note))?.*$</string>


Please check the code that both # @name and // @name exist in the http.tmLanguage. You need to update both. And shall we create a separate match for note.

Huachao · 2020-03-25T08:35:47Z

@ChayimFriedman2 merged, thanks.

ChayimFriedman2 added 17 commits March 6, 2020 11:46

Add confirm-send regex

c6ec655

Set confirm send message if present

9869a94

If there is confirm message, consider the second line as the url line

3e6e2ce

Update HttpRequest and SerializedHttpRequest to include confirmSendMsg

ad4ad3f

Update the returned HttpRequest

f622440

Allow omitting confirm message, causing the default

983f6f2

Make confirmSendLineRegex match full text

9496b96

Add '$' at the regex end

Extract logic to IRequestParser

8077cc4

I want to apply the confirmation also to CURL requests. The shared place between HttpRequestParser and CurlRequestParser is IRequestParser.

Add function to get a string until a char

014f07e

Add a function to substring from a char

7ccbbc9

Support confirmation in CURL too

5aff509

Pass the new property in send()

a7f35d1

Implemen the confirmation actually

3cd7fbc

Provide a setting to set the default message

6a3d810

Fix typpo

aa9039b

Run tslint --fix

6a71356

Prefix new setting with rest-client.

7330bac

ChayimFriedman2 added 9 commits March 17, 2020 11:20

Merge branch 'master' into confirm-send

b64bf7e

Roll back some changes as requested by owner

1d45be0

Add regex for #@note syntax

41220e9

Parse the #@note

d5b5cae

Return the note, is found

e9d4080

Pass the confirmation message to runCore()

1127523

Remove the "default confirmation message" setting

e583999

Match every char as note message

12d1269

Add syntax highlighting to #@note

10f5f49

To simplify the process, I ignored the situation of invalid #@name in the syntax highlighting, so that `#@name Hi, this is me` will still be considered a valid name by the highlighter

Run tslint --fix

e92c285

Huachao reviewed Mar 17, 2020

View reviewed changes

Huachao reviewed Mar 18, 2020

View reviewed changes

src/controllers/requestController.ts Outdated Show resolved Hide resolved

ChayimFriedman2 added 6 commits March 24, 2020 17:20

Place confirmation logic in run() (and not runCore())

d67ac64

Use showWarningMessage() instead of showQuickPick()

9b5e4a8

Fix bug

dd58a69

Do not require spaces after the '#' or '//'

Do not pass message to #@note

97c985c

Merge branch 'master' into confirm-send

480ea40

Fix compile error

bdf09b0

Change 'bool' to 'boolean'

Huachao requested changes Mar 25, 2020

View reviewed changes

src/common/constants.ts Outdated Show resolved Hide resolved

src/utils/selector.ts Outdated Show resolved Hide resolved

syntaxes/http.tmLanguage Outdated Show resolved Hide resolved

ChayimFriedman2 added 3 commits March 25, 2020 09:37

Simplify regex check

9ff7c95

Fix syntax grammar

0f5cbb6

Rename to simplify names

b73cd7d

ChayimFriedman2 requested a review from Huachao March 25, 2020 07:58

Huachao requested changes Mar 25, 2020

View reviewed changes

Separate syntaxes for #@name and #@note

03a70e7

ChayimFriedman2 requested a review from Huachao March 25, 2020 08:16

Huachao merged commit 98e52b5 into Huachao:master Mar 25, 2020

ChayimFriedman2 deleted the confirm-send branch March 25, 2020 08:36

Huachao linked an issue Jun 12, 2020 that may be closed by this pull request

DELETE confirmation #478

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Present an Option to Confirm Send #538

Present an Option to Confirm Send #538

ChayimFriedman2 commented Mar 6, 2020

ChayimFriedman2 commented Mar 17, 2020

Huachao commented Mar 17, 2020

ChayimFriedman2 commented Mar 17, 2020

Huachao commented Mar 17, 2020

ChayimFriedman2 commented Mar 17, 2020

Huachao left a comment

Huachao Mar 17, 2020

Huachao Mar 17, 2020

Huachao Mar 17, 2020

Huachao Mar 17, 2020

ChayimFriedman2 Mar 17, 2020

Huachao Mar 18, 2020

ChayimFriedman2 commented Mar 25, 2020

Huachao Mar 25, 2020

Huachao commented Mar 25, 2020

		@@ -77,4 +77,6 @@ export const RequestVariableDefinitionWithNameRegexFactory = (name: string, flag

		export const RequestVariableDefinitionRegex: RegExp = RequestVariableDefinitionWithNameRegexFactory("\\w+", "m");

		export const DangerousNoteDefinitionRegex = /^\s(?:#{1,}\|\/{2,})\s+@note\s+(.+)\s$/m;

Present an Option to Confirm Send #538

Present an Option to Confirm Send #538

Conversation

ChayimFriedman2 commented Mar 6, 2020

ChayimFriedman2 commented Mar 17, 2020

Huachao commented Mar 17, 2020

ChayimFriedman2 commented Mar 17, 2020

Huachao commented Mar 17, 2020

ChayimFriedman2 commented Mar 17, 2020

Huachao left a comment

Choose a reason for hiding this comment

Huachao Mar 17, 2020

Choose a reason for hiding this comment

Huachao Mar 17, 2020

Choose a reason for hiding this comment

Huachao Mar 17, 2020

Choose a reason for hiding this comment

Huachao Mar 17, 2020

Choose a reason for hiding this comment

ChayimFriedman2 Mar 17, 2020

Choose a reason for hiding this comment

Huachao Mar 18, 2020

Choose a reason for hiding this comment

ChayimFriedman2 commented Mar 25, 2020

Huachao Mar 25, 2020

Choose a reason for hiding this comment

Huachao commented Mar 25, 2020