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.

Sign up for GitHub

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

Jump to bottom

js: isso.js: Disable Postbox submit button on click, enable after response #993

Merged
merged 1 commit into from May 17, 2024
Merged

js: isso.js: Disable Postbox submit button on click, enable after response #993

merged 1 commit into from May 17, 2024

Conversation

pkvach
Copy link
Contributor

@pkvach pkvach commented Feb 22, 2024 •
edited

Checklist

  • All new and existing tests are passing
  • I have added an entry to CHANGES.rst because this is a user-facing change or an important bugfix
  • I have written proper commit message(s)

What changes does this Pull Request introduce?

Disable the submit button in Postbox to prevent double posting upon click. Enable the button after receiving a response from the API endpoint.

Why is this necessary?

Fixes #913

@pkvach pkvach force-pushed the fix/js-prevent-multiple-submit branch from 716c072 to d3a1b82 Compare March 20, 2024 20:02
@pkvach pkvach force-pushed the fix/js-prevent-multiple-submit branch from c774e0a to e4178a1 Compare March 30, 2024 23:22
ix5
ix5 requested changes Apr 13, 2024
View reviewed changes
Copy link
Member

@ix5 ix5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few small issues to correct (see inline review), otherwise a nice PR. Thank you @pkvach!

isso/js/app/isso.js Outdated Show resolved Hide resolved
isso/js/app/isso.js Show resolved Hide resolved
isso/js/app/isso.js Outdated Show resolved Hide resolved
isso/js/tests/integration/puppet.test.js Outdated Show resolved Hide resolved
isso/js/tests/integration/puppet.test.js
@@ -264,3 +269,32 @@ test("should execute GET/PUT/POST/DELETE requests correctly", async () => {
{ text: 'New comment body' },
);
});

test("Postbox submit button should be disabled on submit click and enabled after response", async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test case doesn't actually test whether the button is re-enabled after the response, only on an aborted request. Should be trivial to add.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing that out.

I've actually already added a check to ensure the button isn't disabled after a response in the existing test:
"should fill Postbox with valid data and receive 201 reply."

You can see the change here: https://github.com/isso-comments/isso/pull/993/files#diff-f636e0fc5201601aa0484b5e325aa506fae5d9b6fcba34d635497f94939ed059R151.

Since that covers this scenario, a separate test might be redundant.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, you're covering all the bases already. Just please adjust the test titles (this one and the one above outside the diff) so that they reflect what's actually happening.

@ix5
Copy link
Member

ix5 commented Apr 13, 2024

Also, you have 3 identically named commits with the same commit body. You probably meant to amend a single commit or split the tests but forgot to write commit messages.

@pkvach pkvach force-pushed the fix/js-prevent-multiple-submit branch 3 times, most recently from b0d3d0e to e532af7 Compare April 19, 2024 21:21
@pkvach pkvach marked this pull request as draft April 19, 2024 21:36
@ix5 ix5 added client (Javascript) client code and CSS feature labels Apr 23, 2024
@ix5 ix5 added this to the 0.14 milestone Apr 23, 2024
@ix5
Copy link
Member

ix5 commented Apr 29, 2024

Is there something missing that prevents you from removing the draft status?

@pkvach
Copy link
Contributor Author

pkvach commented Apr 30, 2024

Is there something missing that prevents you from removing the draft status?

I've noticed that E2E tests are failing due to timeout on checks, and it's happening in different places. I thought it was due to changes in PR and was looking for a solution.
So for now, I'm marking PR as a draft to look for a solution.

@ix5
Copy link
Member

ix5 commented Apr 30, 2024

I've noticed that E2E tests are failing due to timeout on checks, and it's happening in different places. I thought it was due to changes in PR and was looking for a solution. So for now, I'm marking PR as a draft to look for a solution.

Sorry about that, I'm still trying to understand why the integration tests are so flaky. Maybe it has something to do with using docker compose from inside the GH Actions environment, which is already container-based? The first faux-user initiated POST/PUT requests seem to work, but then there's an issue with a PUT request sometimes not actually going through.

@ix5 ix5 mentioned this pull request May 5, 2024
Merged
5 tasks
@ix5 ix5 modified the milestones: 0.14, 0.13.1 May 9, 2024
@pkvach pkvach force-pushed the fix/js-prevent-multiple-submit branch 5 times, most recently from 69c1cbf to 5c9a0c9 Compare May 14, 2024 07:45
@pkvach
js: isso.js: Disable Postbox submit button on click, enable after res…
27f55e1 
…ponse

Disable the submit button in Postbox to prevent double posting upon click. Enable the button after receiving a response from the API endpoint.

Fixes isso-comments#913
@pkvach pkvach force-pushed the fix/js-prevent-multiple-submit branch from 5c9a0c9 to 27f55e1 Compare May 14, 2024 21:51
@pkvach pkvach marked this pull request as ready for review May 14, 2024 21:58
ix5
ix5 approved these changes May 17, 2024
View reviewed changes
Copy link
Member

@ix5 ix5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ix5 ix5 merged commit 6f3874c into isso-comments:master May 17, 2024
19 checks passed
@ix5
Copy link
Member

ix5 commented May 17, 2024

I hope the usability of the screenshot tests wasn't too bad for you. I found them useful at the time when I created them because we were messing up some of the CSS/layout with a lot of attempted cleanups.

At least the tests don't seem to be flaky any more ;)

@pkvach pkvach deleted the fix/js-prevent-multiple-submit branch May 18, 2024 04:53
@pkvach
Copy link
Contributor Author

pkvach commented May 18, 2024

I hope the usability of the screenshot tests wasn't too bad for you. I found them useful at the time when I created them because we were messing up some of the CSS/layout with a lot of attempted cleanups.

At least the tests don't seem to be flaky any more ;)

It took me a little while to get the hang of them, but I found them very helpful.
Thank you for creating them. They're definitely useful for moving forward!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ix5 ix5 ix5 approved these changes

Assignees
No one assigned
Labels
client (Javascript) client code and CSS feature
Projects
None yet
Milestone
0.13.1
Development

Successfully merging this pull request may close these issues.

Prevent clicking submit twice
2 participants
@pkvach @ix5