Studio: Be more specific in push success email by listing individual sync elements #2007
Conversation
…ecific-data-to-backend-for-email
📊 Performance Test ResultsComparing 351b7d1 vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
sejas
left a comment
sejas
left a comment
There was a problem hiding this comment.
@katinthehatsite , it works great! I tested it the PR with the backend PR 195447-ghe-Automattic/wpcom . However I noticed that we are sending paths option, which I believe is not necessary. In that case I also suggest renaming the include_path_list param to avoid confusion with a valid param that uses VaultPress.
| if ( specificSelectionPaths && specificSelectionPaths.length > 0 ) { | ||
| const options = optionsToSync ? [ ...optionsToSync, 'paths' ] : [ 'paths' ]; | ||
| formData.push( [ 'options', options.join( ',' ) ] ); | ||
|
|
||
| const joinedPaths = specificSelectionPaths.join( ',' ); | ||
| formData.push( [ 'include_path_list', joinedPaths ] ); | ||
| } else if ( optionsToSync ) { |
There was a problem hiding this comment.
@katinthehatsite , Is there any specific reason we are sending the option paths ?
I think that option is not necessary. For the same reason I would suggest renaming include_path_list var name. Maybe push_selection_paths? 🤔 .
| if ( specificSelectionPaths && specificSelectionPaths.length > 0 ) { | |
| const options = optionsToSync ? [ ...optionsToSync, 'paths' ] : [ 'paths' ]; | |
| formData.push( [ 'options', options.join( ',' ) ] ); | |
| const joinedPaths = specificSelectionPaths.join( ',' ); | |
| formData.push( [ 'include_path_list', joinedPaths ] ); | |
| } else if ( optionsToSync ) { | |
| if ( specificSelectionPaths && specificSelectionPaths.length > 0 ) { | |
| const joinedPaths = specificSelectionPaths.join( ',' ); | |
| formData.push( [ 'push_selection_paths', joinedPaths ] ); | |
| } | |
| if ( optionsToSync ) { |
There was a problem hiding this comment.
@katinthehatsite , Is there any specific reason we are sending the option paths ?
Don't we need to send SQL which does not come as a part of wp-content so that we can display information regarding the database being pushed to the user?
There was a problem hiding this comment.
For example, this is what we get when choosing database:
optionsToSync [ 'sqls', 'plugins' ]
specificSelectionPaths [ 'plugins/akismet' ]
And this is what we get when choosing all the files:
This is what happens when we select everything:
```optionsToSync [ 'all' ]
specificSelectionPaths undefined
And this is what happens when we select just database:
optionsToSync [ 'sqls' ]
specificSelectionPaths undefined
There was a problem hiding this comment.
My comment is specific about appending pahts to the options we send to the backend:
const options = optionsToSync ? [ ...optionsToSync, 'paths' ] : [ 'paths' ];
Studio was not using that option before, and AFAIK that option is not necessary.
There was a problem hiding this comment.
I was suspicious of the paths option because the official parameter include_path_list that VaultPress requires needs to be in base64. It seems to me that we are sending both the paths option and the include_path_list to the backend, and then from the backend to VaultPress. However, the include_path_list will actually be ignored because it's not in base64.
Don't we need to send SQL which does not come as a part of wp-content so that we can display information regarding the database being pushed to the user?
Yes, sqls is a valid option, that we need to send when database is checked.
For example, this is what we get when choosing database: ....
Those are correct values ! 👌
Related issues
Closes STU-759
Proposed Changes
Testing Instructions
Pre-merge Checklist