mattermost-webapp
mattermost-webapp copied to clipboard
mattermost
MM-45494: Edit message history
Summary
This PR introduces a new feature for users to
- see the edit history of their edited messages
- restore an old version
Ticket Link
Fixes https://github.com/mattermost/mattermost-server/issues/20864 JIRA: https://mattermost.atlassian.net/browse/MM-45494
Related Pull Requests
- Has server changes (https://github.com/mattermost/mattermost-server/pull/20945)
Screenshots
https://user-images.githubusercontent.com/37421564/189528414-5afc2363-a227-4cb9-9221-fc19a0ac6631.mov
Release Note
Give ability to users to see the history of edited messages and restore an old version with current version.
Hello @sinansonmez,
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.
E2E tests not automatically triggered, because PR has no approval yet. Please ask a developer to review and then try again to attach the QA label.
@hmhealey @michelengelen @jfrerich This PR is ready for review. I just need to add test for server PR which I am planning to add on the upcoming days. In the meantime, I kindly ask you to start reviewing both webapp and server PRs.
The message history RHS doesn't seem to load for me on the test server. It's just an infinite loading state.
Same for me @sinansonmez ^. Super excited about getting this in, thanks for taking it!
Test server might not be getting a correct server PR. @hmhealey any idea how can I check this?
@sinansonmez great to meet you today! As discussed, let's stick with only the post creator to be able to see the edit history and restore. We can always expand the permissions in the future based on feedback and usage of the feature.
Test server might not be getting a correct server PR. @hmhealey any idea how can I check this?
If you look at the About Mattermost modal in the product switcher menu in the top left, it shows you the exact commit hashes of both the server and web app code being use, so you can cross-reference them with the git history.
In this case though, I was looking at the CI stuff for test servers a few weeks back, and I think that test servers created from a web app PR using a fork of the mattermost-webapp repo will never use the matching mattermost-server branch. It might work if you create a test server from the server PR though even if that logic is a bit unreliable
Thanks for the guidance @hmhealey I already tried adding Test Cloud Server label to server PR. Unfortunately, that didn't work as well. @isacikgoz even suggested creating a webapp branch with exactly the same name as the server branch. Again no luck. Please see the discussion here
The question is that how can PM and UX can test these changes easily?
@phoinixgrr Do you have any idea why spinwick couldn't create the instance with the webapp branch? Was this feature only for the upstream branches? Context: https://github.com/mattermost/mattermost-server/pull/20945#issuecomment-1263498545
@phoinixgrr Do you have any idea why spinwick couldn't create the instance with the webapp branch? Was this feature only for the upstream branches? Context: mattermost/mattermost-server#20945 (comment)
@isacikgoz 🤷 SRE team can better provide support on this. cc: @angeloskyratzakos @d-wierdsma @Mshahidtaj @spirosoik
kindly pinging for support @angeloskyratzakos @d-wierdsma @Mshahidtaj @spirosoik
I asked around a bit on this, and while the test servers are rather flaky with whether or not they use the correct commit, it looks like it's actually impossible to get an automatic test server with the corresponding server and web app changes on it when the PR is coming from a forked repo (like all community PRs would be). For that to work, the branches would also need to have the same name as well (these ones are different), but that doesn't matter because they're also from a fork.
@isacikgoz @jfrerich Would you feel comfortable reviewing and merging the PR before we merge this, or should we look at manually setting up a test server with both of these branches on it?
Would you feel comfortable reviewing and merging the PR before we merge this, or should we look at manually setting up a test server with both of these branches on it?
@hmhealey I think UX and QA would need a test server with both branches on it
Would you feel comfortable reviewing and merging the PR before we merge this, or should we look at manually setting up a test server with both of these branches on it?
@hmhealey I think UX and QA would need a test server with both branches on it
I wouldn't want to merge without UX and QA approval. Whether we can get a test server or they setup locally on their machines doesn't matter. I suppose one solution to the test server is to create have a MM user create a parallel PR for server and webapp (only for creating the test instance)
@jfrerich Creating a parallel set of PRs seems like it'd work well enough. Would you mind trying that out?
@hmhealey @jfrerich as discussed with @AshishDhama , he will create parallel branches copying from my fork branches in webapp and server repos. Then we will have the test server up and ready
Cool. I must've missed that discussion since I was worried this had been forgotten about. Thanks for the update!
@matthewbirtch @hmhealey @michelengelen finally test server is working correctly. You can check here https://github.com/mattermost/mattermost-server/pull/21601#issuecomment-1307655892
This PR has been automatically labelled "stale" because it hasn't had recent activity. A core team member will check in on the status of the PR to help with questions. Thank you for your contribution!
Mattermost test server created! :tada:
Access here: https://mattermost-webapp-pr-11069.test.mattermost.cloud
| Account Type | Username | Password |
|---|---|---|
| Admin | sysadmin | Sys@dmin123 |
| User | user-1 | User-1@123 |