Skip to content
This repository has been archived by the owner on Dec 27, 2022. It is now read-only.

Snapshot merge #92

Merged
merged 14 commits into from
Oct 10, 2016
Merged

Snapshot merge #92

merged 14 commits into from
Oct 10, 2016

Conversation

PatelUtkarsh
Copy link
Member

Fixes #67

} );

foreach ( $posts as $post ) {
$post->post_content = $this->get_post_content( $post );
Copy link
Contributor

@westonruter westonruter Oct 6, 2016

Choose a reason for hiding this comment

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

I think the data parsed from the post_content should be put into another array instead of stuffed back into $post. So maybe:

$snapshots_data = array();
foreach ( $posts as $post ) {
    $snapshots_data[] = $this->get_post_content( $post );
}

And use $snapshots_data instead of $content.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have changed that, I kept it as such because I thought we will need mapping of post and data. However, I never used that mapping, but now to add uuid in merge_conflcit i will use it,
Creating another array like

$snapshot_post_data = array();
foreach ( $posts as $post ) {
    $snapshot_post_data[] = array(
        'data' => $this->get_post_content( $post ),
        'uuid' => $post->post_name,
    );
}

}
$content = wp_list_pluck( $posts, 'post_content' );
$conflict_keys = call_user_func_array( 'array_intersect_key', $content );
$merge_value = call_user_func_array( 'array_merge', $content );
Copy link
Contributor

@westonruter westonruter Oct 6, 2016

Choose a reason for hiding this comment

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

Better name than $merge_value would be $merged_snapshot_data.

@@ -115,6 +115,16 @@ public function register() {
add_action( 'admin_notices', array( $this, 'show_publish_error_admin_notice' ) );
add_action( 'post_submitbox_minor_actions', array( $this, 'hide_disabled_publishing_actions' ) );
add_action( 'admin_print_scripts-revision.php', array( $this, 'disable_revision_ui_for_published_posts' ) );

// Version check for bulk action.
if ( version_compare( get_bloginfo( 'version' ), '4.7', '>=' ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. Better than using the $wp_version global which I usually use. I'll do this in the future.

$original_values[] = $post->post_content[ $key ];
}
}
array_pop( $original_values );
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe leave the winner in the merge conflict intact?

}
}
array_pop( $original_values );
$merge_value[ $key ]['merge_conflict'] = $original_values;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe merge_conflict should use the source UUID as they key for each value here? Then we'll be able to look up later which snapshot the value came from.

jQuery('<option>').val('merge_snapshot').text('<?php esc_html_e( 'Merge Snapshot', 'customize-snapshots' ); ?>').appendTo("select[name='action']");
jQuery('<option>').val('merge_snapshot').text('<?php esc_html_e( 'Merge Snapshot', 'customize-snapshots' ); ?>').appendTo("select[name='action2']");
});
</script>
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation problem.

<script type="text/javascript">
jQuery(document).ready(function() {
jQuery('<option>').val('merge_snapshot').text('<?php esc_html_e( 'Merge Snapshot', 'customize-snapshots' ); ?>').appendTo("select[name='action']");
jQuery('<option>').val('merge_snapshot').text('<?php esc_html_e( 'Merge Snapshot', 'customize-snapshots' ); ?>').appendTo("select[name='action2']");
Copy link
Contributor

Choose a reason for hiding this comment

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

These lines aren't right. There could be a translation string that has an apostrophe and this would cause a JS error. I suggest refactoring as follows:

jQuery( function( $ ) {
    var optionText = <?php echo wp_json_encode( __( 'Merge Snapshot', 'customize-snapshots' ) ); ?>;
    $( 'select[name="action"], select[name="action2"]' ).each( function() {
        var option = $( '<option>', {
            text: optionText,
            value: 'merge_snapshot'
        } );
        $( this ).append( option );
    } );
} );

Copy link
Member Author

Choose a reason for hiding this comment

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

Super clean 👍 🙇

@coveralls
Copy link

coveralls commented Oct 7, 2016

Coverage Status

Coverage decreased (-1.7%) to 89.326% when pulling 37e3b7b on feature/snapshot-merge into 9625866 on develop.

@coveralls
Copy link

coveralls commented Oct 7, 2016

Coverage Status

Coverage decreased (-0.2%) to 90.814% when pulling 2919e51 on feature/snapshot-merge into 9625866 on develop.

@westonruter
Copy link
Contributor

@PatelUtkarsh is this still a WIP or is it complete from your perspective?

@PatelUtkarsh
Copy link
Member Author

@westonruter Resolve Conflict UI part is remaining.

@PatelUtkarsh
Copy link
Member Author

@westonruter I am working on Resolve Conflict UI on a separate branch so this can be reviewed and merged if you think Resolve Conflict UI is not critical.

@westonruter westonruter changed the title [WIP] Snapshot merge Snapshot merge Oct 10, 2016
@coveralls
Copy link

coveralls commented Oct 10, 2016

Coverage Status

Coverage decreased (-0.2%) to 90.814% when pulling f2cefad on feature/snapshot-merge into 9625866 on develop.

@westonruter westonruter merged commit 59b8b66 into develop Oct 10, 2016
@westonruter westonruter deleted the feature/snapshot-merge branch October 10, 2016 17:01
@westonruter westonruter added this to the 0.6.0 milestone Jul 5, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants