Remove offline commenting feature and add post request handler to display offline/error templates. #728

rutviksavsani · 2022-03-10T16:34:51Z

Fixes: #363

This PR

Removes the Offline commenting feature.
Introduces a request handler to display offline/error for POST requests.

Before

After

* Show offline and server error templates on the POST request.

codecov-commenter · 2022-03-10T16:42:48Z

Codecov Report

Merging #728 (e1e38e2) into develop (124baab) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@              Coverage Diff              @@
##             develop     #728      +/-   ##
=============================================
- Coverage      19.03%   19.02%   -0.01%     
  Complexity       346      346              
=============================================
  Files             57       57              
  Lines           2322     2323       +1     
=============================================
  Hits             442      442              
- Misses          1880     1881       +1

Flag	Coverage Δ
php	`19.02% <0.00%> (-0.01%)`	⬇️
unit	`19.02% <0.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...wp-service-worker-navigation-routing-component.php	`1.47% <0.00%> (ø)`
wp-includes/template.php	`10.90% <0.00%> (-0.21%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 124baab...e1e38e2. Read the comment docs.

* Show a message with `go back` link when user is making a POST request and offline/server-error occurs.

…ate tag to show submission failure on POST request

thelovekesh · 2022-03-14T17:13:22Z

When a user makes a POST request and submission fails due to offline/server-error:

s1.mp4

When a user makes a POST request and it opens on a new page:

s-2.mp4

thelovekesh · 2022-03-14T17:19:20Z

@westonruter On thinking more about to show go back link, two use cases came up to my mind:

1 - When POST requests use the same tab to fulfill the request. In this case history.back() will work and go back to last occurrence that will be having bfcache.
2- When the user makes a POST request and it opens in a new tab(probably when target=_blank). In this case, there will be no history but document.referrer will be present.

I am considering both use cases as you can see in the upper comment. Please let me know your thought on it.

westonruter · 2022-03-19T00:47:32Z

@thelovekesh You've made a great point about the go back link not working as expected when opening in a new window. In the case of form submitting into a new window, we may not be able to get document.referrer either if the form had a rel="noreferrer" as well. The other possibility would be check history.length as noted in https://stackoverflow.com/a/3588420/93579 but that doesn't look reliable.

It may be better to just tell the user to go back manually.

thelovekesh · 2022-03-19T04:27:44Z

It may be better to just tell the user to go back manually.

This option looks cleaner to me.

thelovekesh · 2022-03-19T04:33:23Z

@westonruter I have changes made changes to go back as you suggested in the above comment. This PR is ready for your review now.

westonruter · 2022-04-01T02:42:35Z

Can you enable me to be able to push commits to your fork?

wp-includes/js/service-worker-navigation-routing.js

maitreyie-chavan · 2022-04-01T04:35:08Z

Can you enable me to be able to push commits to your fork?

@westonruter I've sent over the invite ✔️

…ancement/363-remove-offline-commenting-and-add-post-request-handler * 'develop' of github.com:GoogleChromeLabs/pwa-wp: (84 commits) Improve test phpdoc descriptions Further generalize naming for site icon code after bca28f1 Add customize cap check before adding button to Customizer Add paragraph wrapper and harden translation escaping Improve name of JS file Reset notifications when site icon has been removed Remove extraneous comments Fix boolean condition for whether icon is too small Improve method naming and return value Improve strings and presentation of site health test Add warning notifications for non-PNG and non-square icons Reuse l10n object in Customizer Remove extraneous variable Show missing icon notification when attachment is no longer present Use attachment data for notifications and update notifications when setting changes Improve function name Add notifications to control instead of section Use warning notification type Reuse existing variable and improve variable name Skip test_pwa_validate_site_icon_not_png on PHP 7.1 ...

…ancement/363-remove-offline-commenting-and-add-post-request-handler * 'develop' of github.com:GoogleChromeLabs/pwa-wp: Bump eslint-plugin-jsdoc from 39.2.0 to 39.2.1 Bump grunt from 1.5.1 to 1.5.2 Bump workbox-cli from 6.5.2 to 6.5.3 Bump eslint-plugin-jsdoc from 39.1.0 to 39.2.0 Bump actions/setup-node from 3.1.0 to 3.1.1 Bump eslint-plugin-jsdoc from 39.0.1 to 39.1.0 Bump grunt from 1.4.1 to 1.5.1 Bump eslint-plugin-jsdoc from 38.1.6 to 39.0.1 Bump @wordpress/scripts from 22.3.0 to 22.4.0 Bump php-stubs/wordpress-stubs from 5.9.1 to 5.9.3 Bump eslint-plugin-import from 2.25.4 to 2.26.0 Bump codecov/codecov-action from 2.1.0 to 3 Bump eslint-plugin-jsdoc from 38.1.4 to 38.1.6 Bump prettier from 2.6.1 to 2.6.2 Bump actions/setup-node from 3.0.0 to 3.1.0

westonruter · 2022-04-14T23:10:35Z

wp-includes/js/service-worker-navigation-routing.js

+							body = body.replace(
+								'{{{WP_SERVICE_WORKER_SUBMISSION_FAILURE_MESSAGE}}}',
+								''
+							);


A problem here is that this will leave the document with:

<p><strong></strong></p>

westonruter · 2022-04-14T23:31:03Z

When I try navigating to a page while offline:

If I try submitting a comment while offline:

When I forcibly try to introduce a 500 error:

In this last case, I'm seeing something which may be a regression caused by #438:

infinite-reload-on-post-submission-failure.mov

This seems like a bug. If there is a 500 error, we definitely shouldn't be attempting to reload the page, and we shouldn't be doing a reload for a POST request either.

westonruter

But that can be addressed in another PR. This looks good and behaves as expected otherwise.

thelovekesh and others added 3 commits March 9, 2022 22:48

Remove offline commenting feature

d16c2c0

Add offline post request handling service worker

5240df6

* Show offline and server error templates on the POST request.

Remove offline commenting and introduce post request handler.

a551c89

thelovekesh requested a review from westonruter March 10, 2022 19:21

thelovekesh marked this pull request as draft March 11, 2022 06:32

thelovekesh added 4 commits March 14, 2022 13:27

Add submission failure error message and mustache template tag

45dad66

* Show a message with `go back` link when user is making a POST request and offline/server-error occurs.

Add handling for WP_SERVICE_WORKER_SUBMISSION_FAILURE_MESSAGE templ…

2d74362

…ate tag to show submission failure on POST request

Add goBack function to go to previos page when user submission fails

4a3f64e

Add test cases for goBack function use case

e803385

thelovekesh marked this pull request as ready for review March 14, 2022 17:13

thelovekesh self-assigned this Mar 15, 2022

This was referenced Mar 15, 2022

Add Deprecation warning for offline commenting feature. #719

Closed

Remove novalidate attribute from comment form to enable client-side validation #717

Closed

thelovekesh added 2 commits March 19, 2022 10:01

Remove goBack link functionality

001a741

Remove goBack link functionality tests

4753877

westonruter added this to the 0.7 milestone Apr 1, 2022

westonruter reviewed Apr 1, 2022

View reviewed changes

wp-includes/js/service-worker-navigation-routing.js Outdated Show resolved Hide resolved

thelovekesh and others added 3 commits April 1, 2022 12:29

Fix errorMessages which was quoted

61ae7ba

westonruter reviewed Apr 14, 2022

View reviewed changes

Combine strings into single WP_SERVICE_WORKER_ERROR_MESSAGE

23667a6

westonruter mentioned this pull request Apr 14, 2022

Add page reloading in offline.php templates #697

Merged

1 task

westonruter approved these changes Apr 14, 2022

View reviewed changes

westonruter merged commit f93356d into GoogleChromeLabs:develop Apr 14, 2022

This was referenced Apr 14, 2022

Prevent auto-reloading on error pages in response to POST requests #763

Merged

Offline commenting can fail if the user does not provide valid fields #364

Closed

thelovekesh deleted the enhancement/363-remove-offline-commenting-and-add-post-request-handler branch April 15, 2022 04:06

westonruter added the changelogged label Apr 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove offline commenting feature and add post request handler to display offline/error templates. #728

Remove offline commenting feature and add post request handler to display offline/error templates. #728

rutviksavsani commented Mar 10, 2022

codecov-commenter commented Mar 10, 2022 •

edited

Loading

thelovekesh commented Mar 14, 2022

thelovekesh commented Mar 14, 2022

westonruter commented Mar 19, 2022

thelovekesh commented Mar 19, 2022

thelovekesh commented Mar 19, 2022

westonruter commented Apr 1, 2022

maitreyie-chavan commented Apr 1, 2022

westonruter Apr 14, 2022

westonruter commented Apr 14, 2022 •

edited

Loading

westonruter left a comment

Remove offline commenting feature and add post request handler to display offline/error templates. #728

Remove offline commenting feature and add post request handler to display offline/error templates. #728

Conversation

rutviksavsani commented Mar 10, 2022

Before

After

codecov-commenter commented Mar 10, 2022 • edited Loading

Codecov Report

thelovekesh commented Mar 14, 2022

thelovekesh commented Mar 14, 2022

westonruter commented Mar 19, 2022

thelovekesh commented Mar 19, 2022

thelovekesh commented Mar 19, 2022

westonruter commented Apr 1, 2022

maitreyie-chavan commented Apr 1, 2022

westonruter Apr 14, 2022

Choose a reason for hiding this comment

westonruter commented Apr 14, 2022 • edited Loading

westonruter left a comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 10, 2022 •

edited

Loading

westonruter commented Apr 14, 2022 •

edited

Loading