-
Notifications
You must be signed in to change notification settings - Fork 127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix "Create issue modal crashes when assignee is selected before switching projects" #834
Conversation
Hello @sanjaydemansol, Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here. |
@hanzei yes. checking it |
Codecov Report
@@ Coverage Diff @@
## master #834 +/- ##
==========================================
- Coverage 34.84% 34.81% -0.04%
==========================================
Files 52 52
Lines 5949 5949
==========================================
- Hits 2073 2071 -2
- Misses 3664 3666 +2
Partials 212 212
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Note :- Non Blocking note that, this PR binary file is not working (crash when i enable it) in the demansol-sandbox server 6.3 but the same binary is working on My stagingTK server 6.3
@mickmister over to you and @levb |
label: ( | ||
<span> | ||
<img | ||
src={v.avatarUrls[AvatarSize.SMALL]} | ||
style={{width: '24px', marginRight: '10px'}} | ||
/> | ||
<span>{v.displayName}</span> | ||
</span> | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @sanjaydemansol, I'm not sure how this fixes the issue, mainly because I don't know what the problem originally was that was causing the crash. Do you mind explaining what the problem is, and how this PR solves it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This component backend_selector.tsx
is meant for general purpose autocomplete functionality, so we shouldn't have anything to do with users or avatars in this file. Why are you thinking this fix needs to go here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mickmister There was v
assigned to label and that was an object, because of that it was not able to render it properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sanjaydemansol I'm thinking the assignee-specific change needs to be applied to https://github.com/mattermost/mattermost-plugin-jira/blob/master/webapp/src/components/data_selectors/jira_user_selector/jira_user_selector.tsx. It makes sense if there's also a small change needed in backend_selector.tsx
, but we shouldn't be referencing anything to do with avatars in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also if you accidentally leave out a mention in your comment, can you please create a new comment, rather than editing the comment to include the mention? I didn't get a notification because of this, and I just happened to have the tab open and saw your comment here 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mickmister I'll remove avatar, as you've suggested here (#834 (comment)).
I'll check and put specific and which won't generate error at label.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mickmister code updated, removed avatar and code is not generating any errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code is not generating any errors.
@sanjaydemansol To be clear: Before your fix, the webapp was crashing correct? And after your fix, it is not crashing.
@@ -8,7 +8,7 @@ import AsyncSelect from 'react-select/async'; | |||
|
|||
import {Theme} from 'mattermost-redux/types/preferences'; | |||
|
|||
import {IssueMetadata, ReactSelectOption, JiraIssue, SearchIssueParams, APIResponse} from 'types/model'; | |||
import {IssueMetadata, ReactSelectOption, JiraIssue, SearchIssueParams, APIResponse, AvatarSize} from 'types/model'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This import should be removed
return { | ||
label: v, | ||
label: v.displayName, | ||
value: v, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sanjaydemansol Can you please explain this fix? All other types of selectors (non-user) seem to be working correctly. Do they actually need to have this change (which they do receive the change if it is applied in this file)? It seems the problem is specific to users, so I would expect the fix to go into jira_user_selector.tsx
.
Can you please describe how you discovered the root cause, and how this fixes it? What is the error printed to the console during the crash, and what line does it occur on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mickmister this is unrelated to the issue. I'm not sure why it is here. I have updated the code to remove this. The crash is caused by the assignee
property not being reset. I am awaiting dipak to finish the qa on this one.
@hanzei Do you know if someone else able to implement the feedback suggested above instead of @sanjaydemansol? |
@maisnamrajusingh Who is taking ownership of this PR? |
* wip - first wizard works * wip * wip * wip * first steps cloud + new flow * Cloud working, complete sans copy, images * cleanup * more cleanup * go.mod * Updates to flow lib * Added delegation * Update server/setup_flow.go Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> * Update server/setup_flow.go Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> * Update server/setup_flow.go Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> * Update server/setup_flow.go Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> * Update server/setup_flow.go Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> * Update server/setup_flow.go Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> * PR feedback: copy * go mod tidy * Added Jira Server flow * adjusted IsJiraAccessible test * Update server/setup_flow.go Co-authored-by: Ben Schumacher <ben.schumacher@mattermost.com> * Update server/setup_flow.go Co-authored-by: Ben Schumacher <ben.schumacher@mattermost.com> * PR feeback * cloud subtype text, not url * PR feedback, fixed public key encoding * go.mod point at the latest plugin-api * PR feeback and fixes * Removed skip button * Removed SKIP button * Added telemetry * fixed a crash in telemetry * copy consistency * fixed delegate complete notification, connect was not working, now done * Fixed webhook URL display escaping * Finished draft copy * Added announcement to channel steps * Fixed capitalization in titles * Update server/command_test.go Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> * Update server/command_test.go Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> * Update server/command_test.go Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> * Update server/command_test.go Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> * Update server/command_test.go Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> * Update server/command_test.go Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> * Update server/utils/utils.go Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> * Update server/utils/utils.go Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> * copy/test fixups * PR Feedback: delegate step, copy, style * Copy changes (mattermost#838) * Update text Some small text changes * Jira update text * Updates commit up to line 267 * Additonal text changes. * lint * Update server/setup_flow.go Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> * Update server/setup_flow.go Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> * Update server/setup_flow.go Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> * Update server/setup_flow.go Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> * Update server/setup_flow.go Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> * Update server/setup_flow.go Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> * Update server/setup_flow.go Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> * Update server/setup_flow.go Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> * Update server/setup_flow.go Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> * Update server/setup_flow.go Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> * Update server/setup_flow.go Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> * Update server/setup_flow.go Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> * PR feedback: copy Co-authored-by: Aaron Rothschild <aaron.rothschild@gmail.com> Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> * PR feedback: cancel and done * PR feedback: webhook copy * PR feedback: removed 'Confirmed' title as redundant * Removed stats, added daily telemetry (mattermost#836) * Removed stats, added daily telemetry * lint * Fixed webhook URL display escaping * Finished draft copy * go.mod like github * added the hooks * PR feedback: copy * PR feedback: go.mod clean comment * Flow Step.WithImage changes, gofmt * Updated configure webhook screenshot Co-authored-by: Lev Brouk <levbrouk@levs-mbp.lan> Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> Co-authored-by: Ben Schumacher <ben.schumacher@mattermost.com> Co-authored-by: Aaron Rothschild <aaron.rothschild@gmail.com> Co-authored-by: Mattermod <mattermod@users.noreply.github.com>
im taking over @hanzei |
Co-authored-by: Lev Brouk <lev@mattermost.com> Co-authored-by: Mattermod <mattermod@users.noreply.github.com>
@DHaussermann I'm sending out a pr that fixes this. right now @dipak-demansol is doing qa on it. |
@mickmister closing this one and opening another one as there are lots of unrelated changes |
Summary
Ticket Link
Fixes #819