Happychat: Rework selected site logic to send proper site info to happychat and kayako #15013
Conversation
unDemian
added
the
[Status] Needs Review
| @@ -38,6 +36,7 @@ export const HelpContactForm = React.createClass( { | |||
| showSubjectField: PropTypes.bool, | |||
| showSiteField: PropTypes.bool, | |||
| showHelpLanguagePrompt: PropTypes.bool, | |||
| selectedSite: PropTypes.object, | |||
There was a problem hiding this comment.
Since we're only using the ID can we just pass selectedSiteId instead?
There was a problem hiding this comment.
I was thinking that if we ever need any other site details in the future they'll be there, I guess I could just send the id for now and we could rework it later.
There was a problem hiding this comment.
Hm, sorry to put a block on this, but I'm not sure I think this is the best approach here.
The contact form gathers information from the user and then passes this information up when the form is submitted. So it seems odd to me that in this implementation, even though the contact form is setting the selected site, the parent is reaching into Redux state for that data.
The bug appeared when siteId was removed from this.state in the Contact Form. Isn't the simplest solution to keep the Contact Form in charge of passing that data back on submit?
I think submitForm in the Contact Form should be changed — instead of naively passing back this.state (including any new state that we actually don't want the parent to know about) we should explicitly build up what we're passing back:
submitForm() {
const {
howCanWeHelp,
howYouFeel,
message,
subject
} = this.state;
this.props.onSubmit( {
howCanWeHelp,
howYouFeel,
message,
subject,
siteId: this.props.selectedSiteId,
} );
},
This makes the contract between the component and its parent much clearer — you'll get these params back when the form is submitted. I haven't tested, but I think this is the only code you need to modify to fix the bug.
I just worry about the Contact Form being responsible for setting the site, but relying on the parent to send the selected site back in as props. But I'm just making this review a "Comment" in case there's something I'm missing, or there's even a better way than my suggestion.
There was a problem hiding this comment.
I made a note about a regression in your changes that causes the user to always default to their primary site but I think I mostly agree with comment @mattwondra left.
client/state/help/selectors.js
Outdated
| import createSelector from 'lib/create-selector'; | ||
|
|
||
| export const getHelpSelectedSiteId = createSelector( | ||
| state => state.help.selectedSiteId | ||
| ); | ||
|
|
||
| export const getHelpSelectedSite = ( state ) => { | ||
| const siteId = getHelpSelectedSiteId( state ) || getPrimarySiteId( state ); |
There was a problem hiding this comment.
I think you should be using getSelectedOrPrimarySiteId here instead of getPrimarySiteId. Without the form will always default to the users primary site instead of the site they currently have selected
There was a problem hiding this comment.
Nice, I didn't notice that it was added. Updated, thanks.
No worries, that is why we do reviews.
I agree, (the only thing is that with #13520 we introduced a dependency on having siteId in the help page before the form is submitted, Happychat button might not even be active if the site you selected is part of a group that has no online operators -- partly true ).
Definitely, this could be a future enhancement but is not part of the scope of this PR. Not sure if we should do it part of this issue as it was a fairly simply high priority thing that we need to ship asap.
That is how I started but then I removed it after our discussion here https://github.com/Automattic/wp-calypso/pull/13520/files/27face45b7fb423c570d08cd1c369d08cbdf285b#diff-bc7120f8349b381d24b34e6e3a2daeb2 the thing is we still need the one in redux so we can catch the value in the connection middleware. Thanks for taking the time to review this, I pushed some updates for the inline comments. Feel free to re-review. |
|
Somewhat related discussion: #13520 (comment) |
|
I'll chime in and say that this sort of logic duplicated from our site-related selectors IMO is a code smell indicating that this isn't the right approach. (Among other things, you probably want to use |
There was a problem hiding this comment.
Agreeing with what @mattwondra said; this should be handled in HelpContactForm's component (not Redux) state.
Can you help me understand this? I'm guessing what you mean is, without Ultimately this should probably just get shipped in some form, because this whole component structure needs a refactor and that shouldn't hold this stuff up. I'd like to see clearer component responsibility, but these were not built with current use cases in mind so I'm fine with moving forward however gets this out the door. |
Feedback Responses
Exactly, but I added the 'default' siteId in happychat connect which should almost solve the issue.
Thanks, updated.
I agree, this is not ideal but I would like to move forward with the issue at hand as it's affecting HEs, I created a refactor issue 573-gh-happychat where we can add more info and a proper estimate. And maybe someone with a better high level understanding of react-redux best practices can rework some of the components or maybe future me :) New ApproachConsidering feedback from here, #13520 and a private conversation that I had with @ockham I pushed some updates. Basically the For now we need to have it in redux, and if I understood correctly if Please re-review. And thanks a lot for helping me better understand how to structure react components. |
I tried to address how to drop
Not familiar with the TBH, I'd still prefer if we were able to drop the extraneous Redux state (instead of introducing yet another selector)... |
Your example is referring to the initial message sent via By Honestly, I cannot see an easy way to not having it in redux. But I'm open to suggestions. |
|
Here's an annotated pseudo-code example of how to re-structure the responsibilities so that we don't need to store anything in Redux state. The overview:
// MAIN HELP PAGE COMPONENT
// This component takes all responsibility for keeping track of the current siteId.
class HelpContact extends React.Component {
constructor() {
this.state = {
siteId: '<Get default site ID from Redux, so initial groups are correct>'
};
}
render() {
// By passing the same siteId into <HappychatConnection /> and <HelpContactForm />,
// we ensure that the site shown to the user always matches the groups Happychat
// is connecting the user to.
const { siteId } = this.state;
return (
<div>
{ /*
Calculate groups based on the current siteId and pass them into the
<HappychatConnection />. This lets that component take all responsibility
for setting and maintaining groups as siteId changes.
*/ }
<HappychatConnection groups={ this.props.getGroups( siteId ) } />
{ /*
Pass the siteId, tracked in my state, to the <HelpContactForm />, and
update my state whenever the input's value changes. This is like treating
the form as a multi-value controlled input.
*/ }
<HelpContactForm
siteId={ siteId }
onSiteChange={ this.handleSiteChange }
...
/>
</div>
)
}
handleSiteChange( siteId ) {
// When the siteId changes in the form, update siteId in state.
// This causes HappychatConnection to receive the new siteId as props.
this.setState( { siteId } );
}
}// CONTACT FORM COMPONENT
// This component becomes very dumb and agnostic about siteId — the parent must
// pass it in and update when the dropdown makes a new selection.
class HelpContactForm extends React.Component {
render() {
// The SitesDropdown takes its selected site from props, and on changes
// passes the new ID back to the parent, which will then update its state
// and pass this back in as props.
return (
<div>
...
<SitesDropdown
selectedSiteId={ this.props.siteId }
onSiteSelect={ this.props.onSiteChange } />
...
</div>
)
}
}// HAPPYCHAT CONNECTION COMPONENT
// Add groups management to this component. Pass in a groups prop, and this component
// ensures that Happychat always knows which group the user belongs to.
class HappychatConnection extends Component {
componentDidMount() {
if ( this.props.isUninitialized ) {
// Update connectChat to take a list of groups, so the customer is put in
// the right groups immediately.
this.props.connectChat( { groups: this.props.groups } );
} else {
// If the chat has already been initialized, send the groups since they might
// have changed since the initial connection.
//
// Create a new Redux action that fires HAPPYCHAT_SET_GROUPS with these
// groups. Middleware can catch it and send the preferences to the socket.
// We don't need to keep any of this in the Redux store though.
this.props.setHappychatGroups( this.props.groups );
}
}
componentWillReceiveProps( nextProps ) {
// Whenever groups change, tell Happychat about it
if ( this.props.groups !== nextProps.groups ) {
this.props.setHappychatGroups( nextProps.groups );
}
}
render() {
return null;
}
} |
|
Accidentally submitted my last comment before I finished: It may seem like it's adding complication to pass I don't love that the site selector is the only "controlled" input in that form — it might actually make sense to drop all state from the form and force all input values to be passed in and updated in the parent. But that's really not necessary to get this going, again considering that this whole suite of components is overdue for a refactor anyways. I tried to make this as clear as possible, but I might have not explained something or just be wrong about how this will end up working. But this is the avenue I would explore. |
|
We need to land some fix for this today, even if it's not the perfect fix. I'm happy to go back immediately afterward and spend some time getting the perfect fix in, but this is impacting our ability to provide quality, efficient service to our customers, and I'd rather we fix what's broken quickly and then go back and make it better than that we hold up further for a perfect fix the first time around at the cost of days of broken tools. |
|
@mattwondra really appreciate your input, helps me a lot. I moved it to 573-gh-happychat so we can address it as soon as possible. Could you guys take another look at the fix in place so we can move forward with this. |
matticbot
removed
the
[Status] Needs Review
This PR moves selected site logic to an upper level as prop of the
helpcomponent. This will provide us with the selected site info which are sent when initializing a new chat or creating a kayakoTesting steps
Implementation description
redux/help.selectedSiteIdwas connected tohelpcomponent as a prophelp/selectorsthat will provide site info for the selected site or if a site is not selected for the primary site.selectedSiteis passed down as a common prop tohelp-contact-formFixes: #14997