Migrate delete_post_modal.jsx to be pure and use Redux #989

JasperVanEsveld · 2018-03-22T11:34:02Z

Summary

Migrate delete_post_modal.jsx to be pure and use Redux.
Updated unit tests for editing post and menu item.
Also added new unit tests for delete_post_modal

Ticket Link

mattermost/mattermost#7660

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

Ran make check-style to check for style errors (required for all pull requests)
Ran make test to ensure unit and component tests passed
Added or updated unit tests

Migrate DeletePostModal to be pure and use Redux

jwilander

Nice work! Just a few minor comments and then it LGTM

jwilander · 2018-03-22T12:47:51Z

components/delete_post_modal/delete_post_modal.jsx

-        super(props);
+        /**
+         * post data
+         */


We can probably drop some comments where the prop names are self-explanatory

jwilander · 2018-03-22T12:49:14Z

components/delete_post_modal/delete_post_modal.jsx

+    constructor(props) {
+        super(props);
+        this.handleDelete = this.handleDelete.bind(this);
+        this.onHide = this.onHide.bind(this);


No action required but just a heads up if you use arrow syntax you don't need these binds (since it will autobind). e.g

handleDelete = () => { // stuff }

Oh, that is good to know. Thanks!

jwilander · 2018-03-22T12:51:16Z

components/delete_post_modal/index.js

+
+function mapStateToProps(state, ownProps) {
+    return {
+        ...ownProps,


This is our fault because we do it everywhere (results of a giant copy/paste mistake :P ), but you don't actually need the ...ownProps here at all. The props will get merged with what is returned here automatically

saturninoabril

Awesome, just minor comments.

saturninoabril · 2018-03-22T12:56:27Z

components/dot_menu/dot_menu.jsx

-            /*
-             * Function to set the edting post
+            /**
+             * Function to set the editing post


Nice catch!

saturninoabril · 2018-03-22T13:01:11Z

components/edit_post_modal/edit_post_modal.jsx

@@ -165,7 +165,18 @@ export default class EditPostModal extends React.PureComponent {
        const hasAttachment = editingPost.post.file_ids && editingPost.post.file_ids.length > 0;
        if (updatedPost.message.trim().length === 0 && !hasAttachment) {
            this.handleHide(false);
-            GlobalActions.showDeletePostModal(Selectors.getPost(getState(), editingPost.postId), editingPost.commentsCount, editingPost.isRHS);
+
+            const modalData = {


Please change to deletePostModalData just to be explicit.

saturninoabril · 2018-03-22T13:11:17Z

components/delete_post_modal/delete_post_modal.jsx

            }
        }
    }

    render() {
-        if (!this.state.post) {
+        if (!this.props.post) {


I think this is not necessary since post is defined as required in propTypes. So either remove this or define post like:

post: PropTypes.object,

saturninoabril · 2018-03-22T13:19:06Z

components/channel_layout/channel_controller.jsx

@@ -7,7 +7,7 @@ import {Route} from 'react-router-dom';

 import Pluggable from 'plugins/pluggable';
 import AnnouncementBar from 'components/announcement_bar';
-import DeletePostModal from 'components/delete_post_modal.jsx';
+import DeletePostModal from 'components/delete_post_modal';


This is no longer needed here, and corresponding line https://github.com/mattermost/mattermost-webapp/blob/master/components/channel_layout/channel_controller.jsx#L67 can be deleted.

JasperVanEsveld · 2018-03-22T18:23:25Z

@jwilander @saturninoabril
I've updated the pull request in my last commit according to your guys reviews
Please check if it is alright now

saturninoabril · 2018-03-22T19:29:31Z

components/delete_post_modal/delete_post_modal.jsx

    }

-    handleDelete = () => {
-        deletePost(


Thanks @GitHubJasper!
Forgot to mention earlier:

the deletePost at https://github.com/mattermost/mattermost-webapp/blob/master/actions/post_actions.jsx#L205 is of no use now. It should be deleted but make sure to consider moving these codes, maybe like so:

handleDelete = async () => { const { actions, channelName, focusedPostId, post, teamURL, } = this.props; const {data} = await actions.deletePost(post); if (post.id === focusedPostId && channelName) { browserHistory.push(teamURL + '/channels/' + channelName); } if (data) { this.onHide(); } }

Then at delete_post_modal/index.js, need to generate those new props, like so:

import {getChannel} from 'mattermost-redux/selectors/entities/channels'; import {getCurrentTeamUrl} from 'mattermost-redux/selectors/entities/teams'; function mapStateToProps(state, ownProps) { const channel = getChannel(state, ownProps.post.channel_id); let channelName = ''; if (channel) { channelName = channel.name; } const {focusedPostId} = state.views.channel; return { channelName, focusedPostId, teamURL: getCurrentTeamUrl(state), }; }

(I have not tested above code but more or less it should be like that. Feel free to test functionality :) )

Added some functionality from the old action to the component

JasperVanEsveld · 2018-03-22T21:35:31Z

@saturninoabril
I tried to add the functionality as it was in the previous action but I am not sure how to test that it works.
Don't know how to make the post.id === focusedPostId return true but it should be the same as it was

saturninoabril · 2018-03-22T22:50:35Z

You may test it like:

    test('should call browserHistory.push on handleDelete with post.id === focusedPostId && channelName', async () => {
        browserHistory.push = jest.fn();
        const actions = {deletePost: jest.fn()};
        actions.deletePost.mockReturnValueOnce({data: true});
        const props = {...baseProps, actions, focusedPostId: '123', channelName: 'channel_name', currentTeamDetails: {name: 'team_name'}};
        const wrapper = shallow(
            <DeletePostModal {...props}/>
        );

        await wrapper.instance().handleDelete();
        expect(browserHistory.push).toHaveBeenCalledWith('/team_name/channels/channel_name');
    });

(Should replace currentTeamDetails object to teamName string)

saturninoabril · 2018-03-22T22:52:15Z

components/delete_post_modal/index.js

+    return {
+        channelName,
+        focusedPostId,
+        currentTeamDetails: getCurrentTeam(state),


Just pass a member not the whole object like: teamName: getCurrentTeam(state).name,

saturninoabril · 2018-03-22T22:53:19Z

@GitHubJasper That's my last comment and I'm all good with your PR. Good job!

Clean up test baseProps Pass teamname instead of team object

JasperVanEsveld · 2018-03-23T00:01:36Z

@saturninoabril Thanks :)
It should be alright now.
And thanks for helping me test @CoolTomatos 👍

saturninoabril

Awesome @GitHubJasper and @CoolTomatos, looks good to me!

jwilander · 2018-03-23T12:20:12Z

Great work on this guys!

Migrate DeletePostModal to be pure and use Redux

58f2b09

Migrate DeletePostModal to be pure and use Redux

jasonblais added the 2: Dev Review Requires review by a core commiter label Mar 22, 2018

jasonblais requested review from jwilander and saturninoabril March 22, 2018 11:37

Fix styling problem

77bf8a3

jwilander reviewed Mar 22, 2018

View reviewed changes

saturninoabril suggested changes Mar 22, 2018

View reviewed changes

Changes based on reviews

319488d

saturninoabril reviewed Mar 22, 2018

View reviewed changes

JasperVanEsveld added 2 commits March 22, 2018 22:24

Removed old post action

792d809

Added some functionality from the old action to the component

Remove unused imports from post_actions

a144107

saturninoabril reviewed Mar 22, 2018

View reviewed changes

JasperVanEsveld added 2 commits March 23, 2018 00:57

Add browserhistory test

ffcd8a0

Clean up test baseProps Pass teamname instead of team object

Fix styling problem

2a47bd3

saturninoabril approved these changes Mar 23, 2018

View reviewed changes

jwilander approved these changes Mar 23, 2018

View reviewed changes

jwilander added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels Mar 23, 2018

jwilander merged commit bc48cd4 into mattermost:master Mar 23, 2018

CoolTomatos deleted the delete-post-modal branch March 23, 2018 12:20

amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Mar 23, 2018

lindalumitchell added the Tests/Not Needed Does not require new release tests label Mar 24, 2018

jwilander mentioned this pull request Mar 27, 2018

MM-9969 Re-add post removal for user performing the delete #1013

Merged

hmhealey pushed a commit that referenced this pull request Aug 28, 2020

Fix sync persisted channels (#989)

417a483

hmhealey pushed a commit that referenced this pull request Mar 17, 2021

Fix sync persisted channels (#989)

ebd4fd3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate delete_post_modal.jsx to be pure and use Redux #989

Migrate delete_post_modal.jsx to be pure and use Redux #989

JasperVanEsveld commented Mar 22, 2018 •

edited

jwilander left a comment

jwilander Mar 22, 2018

jwilander Mar 22, 2018

JasperVanEsveld Mar 22, 2018

jwilander Mar 22, 2018

saturninoabril left a comment

saturninoabril Mar 22, 2018

saturninoabril Mar 22, 2018

saturninoabril Mar 22, 2018

saturninoabril Mar 22, 2018

JasperVanEsveld commented Mar 22, 2018

saturninoabril Mar 22, 2018

JasperVanEsveld commented Mar 22, 2018 •

edited

saturninoabril commented Mar 22, 2018

saturninoabril Mar 22, 2018

saturninoabril commented Mar 22, 2018

JasperVanEsveld commented Mar 23, 2018 •

edited

saturninoabril left a comment

jwilander commented Mar 23, 2018

Migrate delete_post_modal.jsx to be pure and use Redux #989

Migrate delete_post_modal.jsx to be pure and use Redux #989

Conversation

JasperVanEsveld commented Mar 22, 2018 • edited

Summary

Ticket Link

Checklist

jwilander left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saturninoabril left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasperVanEsveld commented Mar 22, 2018

Choose a reason for hiding this comment

JasperVanEsveld commented Mar 22, 2018 • edited

saturninoabril commented Mar 22, 2018

Choose a reason for hiding this comment

saturninoabril commented Mar 22, 2018

JasperVanEsveld commented Mar 23, 2018 • edited

saturninoabril left a comment

Choose a reason for hiding this comment

jwilander commented Mar 23, 2018

JasperVanEsveld commented Mar 22, 2018 •

edited

JasperVanEsveld commented Mar 22, 2018 •

edited

JasperVanEsveld commented Mar 23, 2018 •

edited