Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

automated MM-14632 Integration test for removing team icon #4066 #4611

Merged
merged 9 commits into from
Jan 8, 2020
Merged

automated MM-14632 Integration test for removing team icon #4066 #4611

merged 9 commits into from
Jan 8, 2020

Conversation

VishalSwarnkar
Copy link
Contributor

@VishalSwarnkar VishalSwarnkar commented Jan 3, 2020 •
edited
Loading

Summary

Ticket Link (mattermost/mattermost#10470)

Related Pull Requests

Screenshots

@VishalSwarnkar
Copy link
Contributor Author

@jasonblais, kindly review my PR, please.

@jasonblais jasonblais added the Awaiting Submitter Action Blocked on the author label Jan 3, 2020
@jasonblais
Copy link
Contributor

Thanks @VishalSwarnkar! Would you be open to signing the above contributor license agreement, standard in open source projects? If you have any questions, don't hesitate to reach out.

Once signed, we'll queue your PR for reviews. Big thanks for your contribution!

@VishalSwarnkar
Copy link
Contributor Author

Thanks @VishalSwarnkar! Would you be open to signing the above contributor license agreement, standard in open source projects? If you have any questions, don't hesitate to reach out.

Once signed, we'll queue your PR for reviews. Big thanks for your contribution!

@VishalSwarnkar VishalSwarnkar reopened this Jan 3, 2020
@VishalSwarnkar
Copy link
Contributor Author

Thanks, @jasonblais I have signed and submitted CLA

@jasonblais jasonblais added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester and removed Awaiting Submitter Action Blocked on the author labels Jan 3, 2020
@jasonblais
Copy link
Contributor

Thanks @VishalSwarnkar, I've queued it for review. Would you be open to updating the PR description with a summary and ticket link?

reflog
reflog suggested changes Jan 4, 2020
View reviewed changes
Copy link
Contributor

@reflog reflog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @VishalSwarnkar ! Thanks for the PR. Everything looks great, but one request - please use 'data-testid' instead of 'id' (following https://docs.cypress.io/guides/references/best-practices.html)

@VishalSwarnkar
Copy link
Contributor Author

Thanks, @reflog for reviewing the code. As suggested I have replaced newly introduced id with data-testid and updated the PR. Kindly review the changes

@jasonblais
Copy link
Contributor

/check-cla

@jasonblais jasonblais requested a review from reflog January 4, 2020 19:19
reflog
reflog approved these changes Jan 5, 2020
View reviewed changes
Copy link
Contributor

@reflog reflog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!

@VishalSwarnkar
Copy link
Contributor Author

Hi @jasonblais the PR is now approved now but still, I can see the current status as "merge/blocked Expected — Waiting for status to be reported". Is there anything I have to do in regards to the PR?

saturninoabril
saturninoabril suggested changes Jan 6, 2020
View reviewed changes
Copy link
Member

@saturninoabril saturninoabril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@VishalSwarnkar Test flow looks good, except for few comments, thanks!

e2e/cypress/integration/team/remove_team_icon_spec.js Outdated Show resolved Hide resolved
e2e/cypress/integration/team/remove_team_icon_spec.js Outdated Show resolved Hide resolved
e2e/cypress/integration/team/remove_team_icon_spec.js Outdated Show resolved Hide resolved
e2e/cypress/integration/team/remove_team_icon_spec.js Outdated Show resolved Hide resolved
e2e/cypress/integration/team/remove_team_icon_spec.js Outdated Show resolved Hide resolved
@jasonblais
Copy link
Contributor

Hi @jasonblais the PR is now approved now but still, I can see the current status as "merge/blocked Expected — Waiting for status to be reported". Is there anything I have to do in regards to the PR?

@VishalSwarnkar Thanks! The PR actually undergoes two dev reviews and 1 QA review (among the three reviews requested previously). You can learn more about our code review process in our docs. Let me know if that's helpful.

As for the "merge/blocked Expected — Waiting for status to be reported" error, I'm not actually sure what that refers to, @saturninoabril would you know?

@saturninoabril
Copy link
Member

@jasonblais I'm also not sure.
@VishalSwarnkar Once you make changes per comments, please update your branch into master and let's see what'll happen with the "merge/block".

@VishalSwarnkar
Copy link
Contributor Author

Hi @saturninoabril as requested, I have incorporated the review comments and update the pull request. Kindly review

saturninoabril
saturninoabril suggested changes Jan 7, 2020
View reviewed changes
Copy link
Member

@saturninoabril saturninoabril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@VishalSwarnkar Thanks for the update, all looks good except for last request.

components/setting_picture.jsx Show resolved Hide resolved
saturninoabril
saturninoabril approved these changes Jan 7, 2020
View reviewed changes
Copy link
Member

@saturninoabril saturninoabril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@VishalSwarnkar Thanks, LGTM!

@saturninoabril saturninoabril removed the 2: Dev Review Requires review by a core commiter label Jan 7, 2020
srkgupta
srkgupta approved these changes Jan 8, 2020
View reviewed changes
Copy link
Contributor

@srkgupta srkgupta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @VishalSwarnkar. The testcase is looking great. Much appreciated.

@srkgupta srkgupta added 4: Reviews Complete All reviewers have approved the pull request QA Review Done and removed 3: QA Review Requires review by a QA tester labels Jan 8, 2020
@srkgupta srkgupta merged commit 2baaff8 into mattermost:master Jan 8, 2020
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Jan 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@reflog reflog reflog approved these changes

@srkgupta srkgupta srkgupta approved these changes

@saturninoabril saturninoabril saturninoabril approved these changes

Assignees
No one assigned
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation QA Review Done
Projects
None yet
Milestone
No milestone
6 participants
@VishalSwarnkar @jasonblais @saturninoabril @reflog @srkgupta @amyblais