Skip to content

Commit 779f10f

Browse files
Nityanand13Nityanand13
authored
[MI-2814] Done the review fixes of github PR mattermost#636 (#22)
* [MI-2736]: Review fixes done 1. Improved code readability * [MI-2736]: Review fixes done 1. Fixed linting errors * [MI-2736]: Review fixes done 1. Fixed linting error * [MI-2736]: Review fixes done 1. Improved code readability * [MI-2736]: Review fixes done 1. Changed the case of few endpoints to snake case * [MI-2814]: Review fixes done of github PR mattermost#636 * [MI-2814]: Review fixes done 1. Made comment field editable when trying to add a comment from message action. * [MI-2814]: Review fixes done 1. Improved code readability * [MI-2814]: Review fixes done 1. Used GET method on getting the issue info 2. Improved code readability * [MI-2814]: Improved linting error
1 parent 3cecd5d commit 779f10f

File tree

12 files changed

+107
-227
lines changed

12 files changed

+107
-227
lines changed

‎server/plugin/api.go‎

Lines changed: 32 additions & 100 deletions
Original file line numberDiff line numberDiff line change
@@ -97,9 +97,7 @@ func (p *Plugin) initializeAPI() {
9797
apiRouter.HandleFunc("/create_issue", p.checkAuth(p.attachUserContext(p.createIssue), ResponseTypePlain)).Methods(http.MethodPost)
9898
apiRouter.HandleFunc("/close_or_reopen_issue", p.checkAuth(p.attachUserContext(p.closeOrReopenIssue), ResponseTypePlain)).Methods(http.MethodPost)
9999
apiRouter.HandleFunc("/update_issue", p.checkAuth(p.attachUserContext(p.updateIssue), ResponseTypePlain)).Methods(http.MethodPost)
100-
apiRouter.HandleFunc("/edit_issue_modal", p.checkAuth(p.attachUserContext(p.openIssueEditModal), ResponseTypePlain)).Methods(http.MethodPost)
101-
apiRouter.HandleFunc("/close_reopen_issue_modal", p.checkAuth(p.attachUserContext(p.openCloseOrReopenIssueModal), ResponseTypePlain)).Methods(http.MethodPost)
102-
apiRouter.HandleFunc("/attach_comment_issue_modal", p.checkAuth(p.attachUserContext(p.openAttachCommentIssueModal), ResponseTypePlain)).Methods(http.MethodPost)
100+
apiRouter.HandleFunc("/issue_info", p.checkAuth(p.attachUserContext(p.getIssueInfo), ResponseTypePlain)).Methods(http.MethodGet)
103101
apiRouter.HandleFunc("/create_issue_comment", p.checkAuth(p.attachUserContext(p.createIssueComment), ResponseTypePlain)).Methods(http.MethodPost)
104102
apiRouter.HandleFunc("/mentions", p.checkAuth(p.attachUserContext(p.getMentions), ResponseTypePlain)).Methods(http.MethodGet)
105103
apiRouter.HandleFunc("/unreads", p.checkAuth(p.attachUserContext(p.getUnreads), ResponseTypePlain)).Methods(http.MethodGet)
@@ -959,99 +957,38 @@ func (p *Plugin) updateSettings(c *serializer.UserContext, w http.ResponseWriter
959957
p.writeJSON(w, info.Settings)
960958
}
961959

962-
func (p *Plugin) openAttachCommentIssueModal(c *serializer.UserContext, w http.ResponseWriter, r *http.Request) {
963-
req := &serializer.OpenCreateCommentOrEditIssueModalRequestBody{}
964-
if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
965-
c.Log.WithError(err).Warnf("Error decoding the JSON body")
966-
p.writeAPIError(w, &serializer.APIErrorResponse{ID: "", Message: "Please provide a valid JSON object.", StatusCode: http.StatusBadRequest})
967-
return
968-
}
969-
970-
userID := r.Header.Get(constants.HeaderMattermostUserID)
971-
post, appErr := p.API.GetPost(req.PostID)
972-
if appErr != nil {
973-
p.writeAPIError(w, &serializer.APIErrorResponse{ID: "", Message: fmt.Sprintf("failed to load the post %s", req.PostID), StatusCode: http.StatusInternalServerError})
974-
return
975-
}
976-
if post == nil {
977-
p.writeAPIError(w, &serializer.APIErrorResponse{ID: "", Message: fmt.Sprintf("failed to load the post %s : not found", req.PostID), StatusCode: http.StatusNotFound})
978-
return
979-
}
980-
981-
p.API.PublishWebSocketEvent(
982-
wsEventAttachCommentToIssue,
983-
map[string]interface{}{
984-
"postId": post.Id,
985-
"owner": req.RepoOwner,
986-
"repo": req.RepoName,
987-
"number": req.IssueNumber,
988-
},
989-
&model.WebsocketBroadcast{UserId: userID},
990-
)
991-
}
992-
993-
func (p *Plugin) openCloseOrReopenIssueModal(c *serializer.UserContext, w http.ResponseWriter, r *http.Request) {
994-
req := &serializer.OpenCreateCommentOrEditIssueModalRequestBody{}
995-
if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
996-
c.Log.WithError(err).Warnf("Error decoding the JSON body")
997-
p.writeAPIError(w, &serializer.APIErrorResponse{ID: "", Message: "Please provide a valid JSON object.", StatusCode: http.StatusBadRequest})
998-
return
999-
}
1000-
1001-
userID := r.Header.Get(constants.HeaderMattermostUserID)
1002-
1003-
post, appErr := p.API.GetPost(req.PostID)
1004-
if appErr != nil {
1005-
p.writeAPIError(w, &serializer.APIErrorResponse{ID: "", Message: fmt.Sprintf("failed to load the post %s", req.PostID), StatusCode: http.StatusInternalServerError})
1006-
return
1007-
}
1008-
if post == nil {
1009-
p.writeAPIError(w, &serializer.APIErrorResponse{ID: "", Message: fmt.Sprintf("failed to load the post %s : not found", req.PostID), StatusCode: http.StatusNotFound})
1010-
return
1011-
}
1012-
1013-
p.API.PublishWebSocketEvent(
1014-
wsEventCloseOrReopenIssue,
1015-
map[string]interface{}{
1016-
"channel_id": post.ChannelId,
1017-
"owner": req.RepoOwner,
1018-
"repo": req.RepoName,
1019-
"number": req.IssueNumber,
1020-
"status": req.Status,
1021-
"postId": req.PostID,
1022-
},
1023-
&model.WebsocketBroadcast{UserId: userID},
1024-
)
1025-
}
960+
func (p *Plugin) getIssueInfo(c *serializer.UserContext, w http.ResponseWriter, r *http.Request) {
961+
owner := r.FormValue(constants.OwnerQueryParam)
962+
repo := r.FormValue(constants.RepoQueryParam)
963+
number := r.FormValue(constants.NumberQueryParam)
964+
postID := r.FormValue(constants.PostIDQueryParam)
1026965

1027-
func (p *Plugin) openIssueEditModal(c *serializer.UserContext, w http.ResponseWriter, r *http.Request) {
1028-
req := &serializer.OpenCreateCommentOrEditIssueModalRequestBody{}
1029-
if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
1030-
c.Log.WithError(err).Warnf("Error decoding the JSON body")
1031-
p.writeAPIError(w, &serializer.APIErrorResponse{ID: "", Message: "Please provide a valid JSON object.", StatusCode: http.StatusBadRequest})
966+
issueNumber, err := strconv.Atoi(number)
967+
if err != nil {
968+
p.writeAPIError(w, &serializer.APIErrorResponse{Message: "Invalid param 'number'.", StatusCode: http.StatusBadRequest})
1032969
return
1033970
}
1034971

1035972
githubClient := p.githubConnectUser(c.Context.Ctx, c.GHInfo)
1036-
issue, _, err := githubClient.Issues.Get(c.Ctx, req.RepoOwner, req.RepoName, req.IssueNumber)
973+
issue, _, err := githubClient.Issues.Get(c.Ctx, owner, repo, issueNumber)
1037974
if err != nil {
1038975
// If the issue is not found, it probably belongs to a private repo.
1039976
// Return an empty response in that case.
1040977
var gerr *github.ErrorResponse
1041978
if errors.As(err, &gerr) && gerr.Response.StatusCode == http.StatusNotFound {
1042979
c.Log.WithError(err).With(logger.LogContext{
1043-
"owner": req.RepoOwner,
1044-
"repo": req.RepoName,
1045-
"number": req.IssueNumber,
980+
"owner": owner,
981+
"repo": repo,
982+
"number": issueNumber,
1046983
}).Debugf("Issue not found")
1047984
p.writeJSON(w, nil)
1048985
return
1049986
}
1050987

1051988
c.Log.WithError(err).With(logger.LogContext{
1052-
"owner": req.RepoOwner,
1053-
"repo": req.RepoName,
1054-
"number": req.IssueNumber,
989+
"owner": owner,
990+
"repo": repo,
991+
"number": issueNumber,
1055992
}).Debugf("Could not get the issue")
1056993
p.writeAPIError(w, &serializer.APIErrorResponse{Message: "Could not get the issue", StatusCode: http.StatusInternalServerError})
1057994
return
@@ -1080,35 +1017,30 @@ func (p *Plugin) openIssueEditModal(c *serializer.UserContext, w http.ResponseWr
10801017
milestoneNumber = *issue.Milestone.Number
10811018
}
10821019

1083-
userID := r.Header.Get(constants.HeaderMattermostUserID)
1084-
post, appErr := p.API.GetPost(req.PostID)
1020+
post, appErr := p.API.GetPost(postID)
10851021
if appErr != nil {
1086-
p.writeAPIError(w, &serializer.APIErrorResponse{ID: "", Message: fmt.Sprintf("failed to load the post %s", req.PostID), StatusCode: http.StatusInternalServerError})
1022+
p.writeAPIError(w, &serializer.APIErrorResponse{ID: "", Message: fmt.Sprintf("failed to load the post %s", postID), StatusCode: http.StatusInternalServerError})
10871023
return
10881024
}
10891025
if post == nil {
1090-
p.writeAPIError(w, &serializer.APIErrorResponse{ID: "", Message: fmt.Sprintf("failed to load the post %s : not found", req.PostID), StatusCode: http.StatusNotFound})
1026+
p.writeAPIError(w, &serializer.APIErrorResponse{ID: "", Message: fmt.Sprintf("failed to load the post %s : not found", postID), StatusCode: http.StatusNotFound})
10911027
return
10921028
}
10931029

1094-
p.API.PublishWebSocketEvent(
1095-
wsEventCreateOrUpdateIssue,
1096-
map[string]interface{}{
1097-
"title": *issue.Title,
1098-
"channel_id": post.ChannelId,
1099-
"postId": req.PostID,
1100-
"milestone_title": milestoneTitle,
1101-
"milestone_number": milestoneNumber,
1102-
"assignees": assignees,
1103-
"labels": labels,
1104-
"description": description,
1105-
"repo_full_name": fmt.Sprintf("%s/%s", req.RepoOwner, req.RepoName),
1106-
"issue_number": *issue.Number,
1107-
},
1108-
&model.WebsocketBroadcast{UserId: userID},
1109-
)
1030+
issueInfo := map[string]interface{}{
1031+
"title": *issue.Title,
1032+
"channel_id": post.ChannelId,
1033+
"postId": postID,
1034+
"milestone_title": milestoneTitle,
1035+
"milestone_number": milestoneNumber,
1036+
"assignees": assignees,
1037+
"labels": labels,
1038+
"description": description,
1039+
"repo_full_name": fmt.Sprintf("%s/%s", owner, repo),
1040+
"issue_number": *issue.Number,
1041+
}
11101042

1111-
p.writeJSON(w, issue)
1043+
p.writeJSON(w, issueInfo)
11121044
}
11131045

11141046
func (p *Plugin) getIssueByNumber(c *serializer.UserContext, w http.ResponseWriter, r *http.Request) {

‎server/plugin/plugin.go‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -40,11 +40,9 @@ const (
4040
wsEventConnect = "connect"
4141
wsEventDisconnect = "disconnect"
4242
// WSEventConfigUpdate is the WebSocket event to update the configurations on webapp.
43-
WSEventConfigUpdate = "config_update"
44-
wsEventRefresh = "refresh"
45-
wsEventCreateOrUpdateIssue = "createOrUpdateIssue"
46-
wsEventCloseOrReopenIssue = "closeOrReopenIssue"
47-
wsEventAttachCommentToIssue = "attachCommentToIssue"
43+
WSEventConfigUpdate = "config_update"
44+
wsEventRefresh = "refresh"
45+
wsEventCreateIssue = "createIssue"
4846

4947
WSEventRefresh = "refresh"
5048

@@ -482,7 +480,7 @@ func (p *Plugin) disconnectGitHubAccount(userID string) {
482480

483481
func (p *Plugin) openIssueCreateModal(userID string, channelID string, title string) {
484482
p.API.PublishWebSocketEvent(
485-
wsEventCreateOrUpdateIssue,
483+
wsEventCreateIssue,
486484
map[string]interface{}{
487485
"title": title,
488486
"channel_id": channelID,

‎server/serializer/issue.go‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -42,11 +42,3 @@ type CommentAndCloseRequest struct {
4242
Status string `json:"status"`
4343
PostID string `json:"postId"`
4444
}
45-
46-
type OpenCreateCommentOrEditIssueModalRequestBody struct {
47-
RepoOwner string `json:"repo_owner"`
48-
RepoName string `json:"repo_name"`
49-
IssueNumber int `json:"issue_number"`
50-
PostID string `json:"postId"`
51-
Status string `json:"status"`
52-
}

‎webapp/src/actions/index.js‎

Lines changed: 2 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -211,47 +211,11 @@ export function getMilestoneOptions(repo) {
211211
};
212212
}
213213

214-
export function attachCommentIssueModal(payload) {
214+
export function getIssueInfo(owner, repo, issueNumber, postID) {
215215
return async (dispatch, getState) => {
216216
let data;
217217
try {
218-
data = await Client.attachCommentIssueModal(payload);
219-
} catch (error) {
220-
return {error};
221-
}
222-
223-
const connected = await checkAndHandleNotConnected(data)(dispatch, getState);
224-
if (!connected) {
225-
return {error: data};
226-
}
227-
228-
return {data};
229-
};
230-
}
231-
232-
export function editIssueModal(payload) {
233-
return async (dispatch, getState) => {
234-
let data;
235-
try {
236-
data = await Client.editIssueModal(payload);
237-
} catch (error) {
238-
return {error};
239-
}
240-
241-
const connected = await checkAndHandleNotConnected(data)(dispatch, getState);
242-
if (!connected) {
243-
return {error: data};
244-
}
245-
246-
return {data};
247-
};
248-
}
249-
250-
export function closeOrReopenIssueModal(payload) {
251-
return async (dispatch, getState) => {
252-
let data;
253-
try {
254-
data = await Client.closeOrReopenIssueModal(payload);
218+
data = await Client.getIssueInfo(owner, repo, issueNumber, postID);
255219
} catch (error) {
256220
return {error};
257221
}

‎webapp/src/client/client.js‎

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -7,16 +7,8 @@ import {ClientError} from 'mattermost-redux/client/client4';
77
import {id as pluginId} from '../manifest';
88

99
export default class Client {
10-
editIssueModal = async (payload) => {
11-
return this.doPost(`${this.url}/edit_issue_modal`, payload);
12-
}
13-
14-
closeOrReopenIssueModal = async (payload) => {
15-
return this.doPost(`${this.url}/close_reopen_issue_modal`, payload);
16-
}
17-
18-
attachCommentIssueModal = async (payload) => {
19-
return this.doPost(`${this.url}/attach_comment_issue_modal`, payload);
10+
getIssueInfo = async (owner, repo, issueNumber, postID) => {
11+
return this.doGet(`${this.url}/issue_info?owner=${owner}&repo=${repo}&number=${issueNumber}&postId=${postID}`);
2012
}
2113

2214
setServerRoute(url) {

‎webapp/src/components/github_issue/index.tsx‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import {Theme} from 'mattermost-redux/types/preferences';
44
import {Post} from 'mattermost-redux/types/posts';
55
import {useDispatch} from 'react-redux';
66

7-
import {attachCommentIssueModal, editIssueModal, closeOrReopenIssueModal} from '../../actions';
7+
import {openCreateCommentOnIssueModal, openCreateOrUpdateIssueModal, openCloseOrReopenIssueModal} from '../../actions';
88

99
type GithubIssueProps = {
1010
theme: Theme,
@@ -24,24 +24,25 @@ const GithubIssue = ({theme, post}: GithubIssueProps) => {
2424
issue_number: postProps.issue_number,
2525
postId: post.id,
2626
status: postProps.status,
27+
channel_id: post.channel_id,
2728
};
2829

2930
const content = (
3031
<div>
3132
<button
3233
style={{...style.button, ...style.other_buttons}}
3334
className='btn btn-primary'
34-
onClick={() => dispatch(attachCommentIssueModal(issue))}
35+
onClick={() => dispatch(openCreateCommentOnIssueModal(issue))}
3536
>{'Comment'}</button>
3637
<button
3738
style={{...style.button, ...style.other_buttons}}
3839
className='btn btn-primary'
39-
onClick={() => dispatch(editIssueModal(issue))}
40+
onClick={() => dispatch(openCreateOrUpdateIssueModal(issue))}
4041
>{'Edit'}</button>
4142
<button
4243
style={{...style.button, ...{...postProps.status === 'Close' ? style.close_or_reopen_button : style.other_buttons}}}
4344
className='btn btn-primary'
44-
onClick={() => dispatch(closeOrReopenIssueModal(issue))}
45+
onClick={() => dispatch(openCloseOrReopenIssueModal(issue))}
4546
>{postProps.status}</button>
4647
</div>
4748
);

‎webapp/src/components/modals/attach_comment_to_issue/attach_comment_to_issue.jsx‎

Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -40,11 +40,11 @@ export default class AttachIssueModal extends PureComponent {
4040
}
4141

4242
if (!this.state.issueValue) {
43-
const {owner, repo, number} = this.props.messageData ?? {};
43+
const {repo_owner, repo_name, issue_number} = this.props.messageData ?? {};
4444
const issue = {
45-
owner,
46-
repo,
47-
number,
45+
owner: repo_owner,
46+
repo: repo_name,
47+
number: issue_number,
4848
comment: this.state.comment,
4949
post_id: this.props.post.id,
5050
show_attached_message: false,
@@ -72,7 +72,7 @@ export default class AttachIssueModal extends PureComponent {
7272
owner,
7373
repo,
7474
number,
75-
comment: this.props.post.message,
75+
comment: this.state.comment,
7676
post_id: this.props.post.id,
7777
show_attached_message: true,
7878
};
@@ -106,17 +106,23 @@ export default class AttachIssueModal extends PureComponent {
106106
});
107107
};
108108

109+
componentDidUpdate(prevProps) {
110+
if (this.props.post && !this.props.messageData && !prevProps.post) {
111+
this.setState({comment: this.props.post.message}); // eslint-disable-line react/no-did-update-set-state
112+
}
113+
}
114+
109115
render() {
110116
const {error, submitting, comment, issueValue} = this.state;
111-
const {visible, theme, messageData, post} = this.props;
117+
const {visible, theme, messageData} = this.props;
112118
const style = getStyle(theme);
113119
if (!visible) {
114120
return null;
115121
}
116122

117-
const {number} = messageData ?? {};
118-
const modalTitle = number ? 'Create a comment to GitHub Issue' : 'Attach Message to GitHub Issue';
119-
const component = number ? (
123+
const {issue_number} = messageData ?? {};
124+
const modalTitle = issue_number ? 'Create a comment to GitHub Issue' : 'Attach Message to GitHub Issue';
125+
const component = issue_number ? (
120126
<div>
121127
<Input
122128
label='Create a comment'
@@ -138,10 +144,9 @@ export default class AttachIssueModal extends PureComponent {
138144
<Input
139145
label='Message Attached to GitHub Issue'
140146
type='textarea'
141-
isDisabled={true}
142-
value={post?.message}
147+
value={comment}
143148
disabled={false}
144-
readOnly={true}
149+
onChange={this.handleIssueCommentChange}
145150
/>
146151
</div>
147152
);

‎webapp/src/components/modals/close_reopen_issue/index.tsx‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,9 @@ const CloseOrReopenIssueModal = ({theme}: {theme: Theme}) => {
3636
channel_id: messageData.channel_id,
3737
issue_comment: comment,
3838
status_reason: currentStatus,
39-
repo: messageData.repo,
40-
number: messageData.number,
41-
owner: messageData.owner,
39+
repo: messageData.repo_name,
40+
number: messageData.issue_number,
41+
owner: messageData.repo_owner,
4242
status: messageData.status,
4343
postId: messageData.postId,
4444
};

0 commit comments

Comments
 (0)