Add/guard installation flow #58493

gcsecsey · 2021-11-25T11:45:28Z

Changes proposed in this Pull Request

prevents the installation if the URL is visited without clicking the Install button on the plugins page
if plugin A was installed previously, it still prevents the installation of plugin B from the URL directly

Testing instructions

Marketplace flow

Prevents direct access

choose a site with any plan
visit /marketplace/<pluginname>/install/<siteURL>
verify that the this error message is shown:

Allows install after `Install` button click

choose a site with any plan
visit /plugins/<pluginname>/<siteURL>
click the Install or Upgrade and Install button
perform the upgrade if needed
verify that the install executes normally

Throws incorrect plan error message correctly

choose a site with a free plan
visit /plugins/<pluginname>/<siteURL>
click the Upgrade and Install button
from the checkout page, click the X button in the upper left corner
verify that the this error message is shown:

Prevents direct access after a successful plugin install

choose a site with any plan
visit /plugins/<pluginname>/<siteURL>
click the Install or Upgrade and Install button
perform the upgrade if needed
verify that the install executes normally
visit /marketplace/<secondpluginname>/install/<siteURL>
verify that the this error message is shown:

Plugin upload flow

Prevents direct access

choose a site with any plan
visit /marketplace/install/<siteURL>
verify that the this error message is shown:

Allows install after plugin upload

choose a site with any plan
visit /plugins/upload/<siteURL>
upload a plugin
verify that the install executes normally

Fixes #58297

Still need to clean up local state (nested ternary) and translation

github-actions · 2021-11-25T11:45:48Z

Link to Calypso live: https://calypso.live?image=registry.a8c.com/calypso/app:build-21288
Link to Jetpack Cloud live: https://calypso.live?image=registry.a8c.com/calypso/app:build-21288&env=jetpack

gcsecsey · 2021-11-25T11:47:05Z

When displaying the error pages, there's a margin-top: 350px rule applied on the container:

@cpapazoglou @WBerredo any ideas why it's needed or if we can remove it perhaps?

matticbot · 2021-11-25T11:56:30Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~2336 bytes added 📈 [gzipped])

name         parsed_size           gzip_size
plugins          +7510 B  (+1.2%)    +1991 B  (+1.1%)
marketplace      +1379 B  (+0.2%)     +345 B  (+0.2%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~101 bytes added 📈 [gzipped])

name                                                                           parsed_size           gzip_size
async-load-calypso-my-sites-marketplace-pages-marketplace-plugin-setup-status       +424 B  (+0.8%)     +101 B  (+0.6%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

cpapazoglou · 2021-11-25T11:56:54Z

When displaying the error pages, there's a margin-top: 350px rule applied on the container:

@cpapazoglou @WBerredo any ideas why it's needed or if we can remove it perhaps?

I suppose it was developed that it appears vertically centered? Probably we can make it better by utilizing flex?

gcsecsey · 2021-11-26T11:55:14Z

As we're handling multiple types of errors now in this component, I've tried consolidating the error handling with the help of an enum, but ran into some race condition issues down the line.

I see we're checking some conditions in an async way here:

wp-calypso/client/my-sites/marketplace/pages/marketplace-plugin-upload-status/index.tsx

Line 98 in 0d38fdc

waitFor( 2 ).then(

@WBerredo could you help me with why the callback is needed here? Do we have some piece of state that changes on page load? Thanks 🙂

WBerredo · 2021-11-26T13:54:40Z

This useEffect will run at every render (we don't pass the properties array) and check there is an unsupported plan, if there is, it will check again in 2 seconds and if the problem persists, it shows an error.

wp-calypso/client/my-sites/marketplace/pages/marketplace-plugin-upload-status/index.tsx

Lines 96 to 102 in 0d38fdc

    
           useEffect( () => { 
        
           	if ( ! supportsAtomicUpgrade.current ) { 
        
           		waitFor( 2 ).then( 
        
           			() => ! supportsAtomicUpgrade.current && setNonInstallablePlanError( true ) 
        
           		); 
        
           	} 
        
           } );

why the callback is needed here?

On the upgrade and install flow, the plan we receive on the first render is the old/low tier plan, but this plan gets updated on the subsequent renders. That's why we wait 2 seconds before showing the error, and we consider if it didn't get updated in 2 seconds this is the current plan of the user.

Does that makes sense? There is anything that still needs explaining?

gcsecsey · 2021-11-26T16:01:50Z

@WBerredo I thought that we can't use the data that we have on the initial render, but wasn't sure, thanks for clarifying! 🎉

WBerredo

I was able to test the general path, but I would like to point two things:

I didn't get the "upgrade to business plan" error, when following the steps I got the "direct access" error.
Shouldn't we store which plugin triggered the install and check it on the state? Otherwise, I would be able to trigger an install and access the direct URL with another plugin installation if done at the same time.

gcsecsey · 2021-11-29T12:02:39Z

Thanks for testing this @WBerredo, it'd be a great idea to add the plugin to the state! I'll check the issue with the errors and add this additional check. 🙂

client/my-sites/plugins/plugin-details.jsx

gcsecsey · 2021-12-01T10:13:32Z

Shouldn't we store which plugin triggered the install and check it on the state? Otherwise, I would be able to trigger an install and access the direct URL with another plugin installation if done at the same time.

We will need to add similar events to the Plugin Upload flow https://wordpress.com/plugins/upload/[DOMAIN] which is now returning an error.

I've added these changes and updated the test instructions with steps for the plugin upload flow. When trying to visit the install page directly on https://wordpress.com/plugins/upload/[DOMAIN], we're now showing a flow-specific error message that helps the user navigate back to the plugin upload page.

I didn't get the "upgrade to business plan" error, when following the steps I got the "direct access" error.

This was a small mistake on my side, I've accidentally copied over the other error message. 😄 I've fixed it now, it should work as per the test instructions. 👍

WBerredo

I am seeing an error on the upgrade and install flow, if I complete the purchase or not I am always getting the "This URL should not be accessed directly. Please click the Install button on the plugin page." error.

I'm not sure if this is related to the environment it is created on the PR. But I think this worth some research.

`pluginInstallationStatus` could be overwritten by other actions

gcsecsey · 2021-12-03T14:45:54Z

I am seeing an error on the upgrade and install flow, if I complete the purchase or not I am always getting the "This URL should not be accessed directly. Please click the Install button on the plugin page." error.

I've looked into this, and found that previously, the pluginInstallationStatus state var was written from both actions that were fired, resulting in a race condition when persisting the state:

wp-calypso/client/my-sites/plugins/plugin-details.jsx

Lines 444 to 451 in 91a7d3b

    
           dispatch( productToBeInstalled( null, slug, selectedSite.slug ) ); 
        
           dispatch( 
        
           	pluginInstallationStateChange( 
        
           		MARKETPLACE_ASYNC_PROCESS_STATUS.IN_PROGRESS, 
        
           		'preauthorize plugin installation URL' 
        
           	) 
        
           );

wp-calypso/client/state/marketplace/purchase-flow/reducer.ts

Lines 34 to 38 in a184952

    
           case MARKETPLACE_QUEUE_PRODUCT_INSTALL: 
        
           	return { 
        
           		...state, 
        
           		siteTransferStatus: MARKETPLACE_ASYNC_PROCESS_STATUS.UNKNOWN, 
        
           		pluginInstallationStatus: MARKETPLACE_ASYNC_PROCESS_STATUS.UNKNOWN,

I've updated the installation check to still validate the primaryDomain and productSlug variables, and to check that the plugin installation hasn't been completed yet:

wp-calypso/client/my-sites/marketplace/pages/marketplace-plugin-upload-status/index.tsx

Line 82 in a184952

pluginInstallationStatus !== MARKETPLACE_ASYNC_PROCESS_STATUS.COMPLETED &&

wp-calypso/client/my-sites/marketplace/pages/marketplace-plugin-upload-status/index.tsx

Line 87 in a184952

pluginInstallationStatus !== MARKETPLACE_ASYNC_PROCESS_STATUS.COMPLETED &&

gcsecsey · 2021-12-03T18:26:23Z

With @WBerredo we've discovered a new edge case.

When the user navigates away from the checkout page by changing the URL in the browser, the IndexedDB state is reset to the defaultState value:
https://d.pr/v/rDDfH9

However, when the user navigates away from the checkout page by clicking the X icon in the upper left corner of the checkout page, the IndexedDB state is persisted as expected:
https://d.pr/v/tqbGqp

It seems that by navigating to a new URL, the Redux init clears the persisted state instead of loading the values from IndexedDB, and hence breaks the purchase flow. When navigating through client routes, the defaultState isn't loaded again and the feature works as expected.

We've tried removing the defaultState so that values in IndexedDB can be read back on a later visit. However, this results in an error, the slice reducer can't be initiated without a default state:

It seems that we won't be able to persist state between URL hops by utilizing IndexedDB. For the time being, I've updated the testing steps to clarify that the feature only works when navigating via UI elements.

WBerredo

Tested the last version and it is all working good 🚀

PS: The only edge case I found is mentioned at this PR but I don't think we can do anything about it.

a8ci18n · 2021-12-06T12:16:32Z

This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/7024500

Thank you @gcsecsey for including a screenshot in the description! This is really helpful for our translators.

* [WIP] add state var when installing plugins from the UI * [WIP] prevent plugin installation if the URL is visited directly Still need to clean up local state (nested ternary) and translation * [WIP] update error handling * [WIP] fail early when loading the URL directly * center error message vertically * update error handling * unset state var when plugin installation is complete * fix error handling * verify plugins slug and domain name when installing plugin from URL * fix plan upgrade error message * delete unnecessary whitespace * guard plugin upload flow too * add new error message to plugin upload flow * verify only `primaryDomain` and `productSlug` when installing plugins `pluginInstallationStatus` could be overwritten by other actions * evaluate error causes in priority order

a8ci18n · 2021-12-17T13:28:47Z

Translation for this Pull Request has now been finished.

gcsecsey added 4 commits November 23, 2021 17:12

[WIP] add state var when installing plugins from the UI

2564ac9

[WIP] prevent plugin installation if the URL is visited directly

f7b3d12

Still need to clean up local state (nested ternary) and translation

[WIP] update error handling

c7d6943

[WIP] fail early when loading the URL directly

e1f99a2

gcsecsey added 3 commits November 25, 2021 20:15

center error message vertically

8bda801

update error handling

edab571

unset state var when plugin installation is complete

06150e0

gcsecsey requested a review from a team as a code owner November 25, 2021 19:16

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 25, 2021

fix error handling

0d38fdc

cpapazoglou requested review from cpapazoglou and removed request for a team and cpapazoglou November 26, 2021 13:36

WBerredo mentioned this pull request Nov 26, 2021

Plugin Installation: Better guard the installation flow #58297

Closed

WBerredo reviewed Nov 26, 2021

View reviewed changes

gcsecsey added 4 commits November 29, 2021 14:41

verify plugins slug and domain name when installing plugin from URL

bfad104

fix plan upgrade error message

4354729

Merge branch 'trunk' into add/guard-installation-flow

0d4f2a9

delete unnecessary whitespace

69bb0b4

cpapazoglou reviewed Nov 30, 2021

View reviewed changes

client/my-sites/plugins/plugin-details.jsx Outdated Show resolved Hide resolved

gcsecsey added 2 commits November 30, 2021 16:23

guard plugin upload flow too

ffc7f88

add new error message to plugin upload flow

91a7d3b

WBerredo requested changes Dec 1, 2021

View reviewed changes

gcsecsey added 2 commits December 3, 2021 13:03

verify only primaryDomain and productSlug when installing plugins

7bbf011

`pluginInstallationStatus` could be overwritten by other actions

Merge branch 'trunk' into add/guard-installation-flow

a184952

gcsecsey requested a review from WBerredo December 3, 2021 15:16

evaluate error causes in priority order

5984a5f

WBerredo approved these changes Dec 3, 2021

View reviewed changes

gcsecsey merged commit 73b9742 into trunk Dec 6, 2021

gcsecsey deleted the add/guard-installation-flow branch December 6, 2021 11:13

github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add/guard installation flow #58493

Add/guard installation flow #58493

gcsecsey commented Nov 25, 2021 •

edited

Loading

github-actions bot commented Nov 25, 2021 •

edited by matticbot

Loading

gcsecsey commented Nov 25, 2021

matticbot commented Nov 25, 2021 •

edited

Loading

cpapazoglou commented Nov 25, 2021

gcsecsey commented Nov 26, 2021

WBerredo commented Nov 26, 2021

gcsecsey commented Nov 26, 2021

WBerredo left a comment

gcsecsey commented Nov 29, 2021

gcsecsey commented Dec 1, 2021 •

edited

Loading

WBerredo left a comment

gcsecsey commented Dec 3, 2021 •

edited

Loading

gcsecsey commented Dec 3, 2021 •

edited

Loading

WBerredo left a comment

a8ci18n commented Dec 6, 2021

a8ci18n commented Dec 17, 2021

Add/guard installation flow #58493

Add/guard installation flow #58493

Conversation

gcsecsey commented Nov 25, 2021 • edited Loading

Changes proposed in this Pull Request

Testing instructions

Marketplace flow

Prevents direct access

Allows install after Install button click

Throws incorrect plan error message correctly

Prevents direct access after a successful plugin install

Plugin upload flow

Prevents direct access

Allows install after plugin upload

github-actions bot commented Nov 25, 2021 • edited by matticbot Loading

gcsecsey commented Nov 25, 2021

matticbot commented Nov 25, 2021 • edited Loading

cpapazoglou commented Nov 25, 2021

gcsecsey commented Nov 26, 2021

WBerredo commented Nov 26, 2021

gcsecsey commented Nov 26, 2021

WBerredo left a comment

Choose a reason for hiding this comment

gcsecsey commented Nov 29, 2021

gcsecsey commented Dec 1, 2021 • edited Loading

WBerredo left a comment

Choose a reason for hiding this comment

gcsecsey commented Dec 3, 2021 • edited Loading

gcsecsey commented Dec 3, 2021 • edited Loading

WBerredo left a comment

Choose a reason for hiding this comment

a8ci18n commented Dec 6, 2021

a8ci18n commented Dec 17, 2021

gcsecsey commented Nov 25, 2021 •

edited

Loading

Allows install after `Install` button click

github-actions bot commented Nov 25, 2021 •

edited by matticbot

Loading

matticbot commented Nov 25, 2021 •

edited

Loading

gcsecsey commented Dec 1, 2021 •

edited

Loading

gcsecsey commented Dec 3, 2021 •

edited

Loading

gcsecsey commented Dec 3, 2021 •

edited

Loading