-
Notifications
You must be signed in to change notification settings - Fork 64
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
[MM-380] Fix issue of todo permalink opening in new tab #251
Conversation
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.
Great work 👍 Just a few comments
} | ||
|
||
receiverIssue := newIssue(issue.Message, issue.Description, issue.PostID) | ||
receiverIssue := newIssue(issue.Message, issue.PostPermalink, issue.Description, issue.PostID) |
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.
Seems like this could just accept a issue
instead of these 4 fields derived from the issue
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 function https://github.com/mattermost/mattermost-plugin-todo/blob/b8dff94f7bebfcb7f81f36f588f110e08e34ef9f/server/issue.go#L27 just return a new object with passed parameters value. At some places https://github.com/mattermost/mattermost-plugin-todo/blob/b8dff94f7bebfcb7f81f36f588f110e08e34ef9f/server/list.go#L62 we are passing the value directly instead of the case above
"type": "custom_todo", | ||
"message": message, | ||
"todo": todo, | ||
"postPermalink": postPermalink, |
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.
What happens for posts that do not have this prop?
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.
If we don't have this prop, we won't have a permalink in the message in the todo
sent to other user in the post
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 PR was tested and I found an issue in its functionality
Steps to reproduce:-
- Create any todo through a post in a channel.
- Once the Todo is reflected in RHS and is ready to be created, click spacebar against the name of the todo, it will create 2 permalinks.
- Click on the bottom permalink.
The permalink present in the bottom will not redirect you to a new page, instead the user will remain on the same page.
cc: @mickmister
@AayushChaudhary0001 I have fixed the issue reported. Please have a look. |
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.
@raghavaggarwal2308 The above PR has been re-tested and the above issue has been resolved. Approving this PR, LGTM.
Summary
Fixed issue of todo permalink opening in new tab
Demo
screen-capture.3.webm
Ticket Link
Fixes #164
What to test?