Skip to content
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.

Sign up for GitHub

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

Jump to bottom

MM-28733 : Admin Advisor v2 #15515

Merged
merged 34 commits into from
Sep 25, 2020
Merged

MM-28733 : Admin Advisor v2 #15515

merged 34 commits into from
Sep 25, 2020

Conversation

catalintomai
Copy link
Contributor

@catalintomai catalintomai commented Sep 16, 2020 •
edited
Loading

@catalintomai catalintomai mentioned this pull request Sep 16, 2020
Closed
@mattermod
Copy link
Contributor

Failed running enterprise tests. @mattermost/core-build-engineers have been notified. Error:
workflow for pip be0a330a-db39-47ab-a811-93640eaa0c94 not found

@catalintomai catalintomai added this to the v5.28.0 milestone Sep 16, 2020
@mattermod
Copy link
Contributor

Failed running enterprise tests. @mattermost/core-build-engineers have been notified. Error:
workflow for pip 7717e90a-6d11-4724-8475-56efba1420a2 not found

@catalintomai catalintomai added 2: Dev Review Requires review by a developer 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review labels Sep 17, 2020
@srkgupta
Copy link
Contributor

@catalintomai since the messages are still not ready and finalized by @esadur, I will only be able to test the PRs once they are finalised.

@jasonblais
Copy link
Contributor

@srkgupta are the messages at https://github.com/mattermost/mattermost-webapp/pull/6461/files#diff-8aed887c744b7eb6f40c0a268ae06e7b?

@srkgupta
Copy link
Contributor

@jasonblais Catalin has just added a placeholder for messages. The actual messages are still not finalized by Eric.

hahmadia
hahmadia reviewed Sep 17, 2020
View reviewed changes
Copy link
Contributor

@hahmadia hahmadia left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Some other general comments (not specific to this PR) but maybe tickets can be made for them?
I noticed that BuildEnterpriseReady was a string declared four years ago and I think it's kinda weird how a boolean variable was initialized a string (maybe a reason for this that I'm not sure) but I feel like this should be a boolean and not a string since throughout the code we have many places checking to see if the value is 'true'.

I think maybe not changing the variable type to boolean but making some sort of function like "func BuildEnterpriseReadyAsBoolean" where then we can just do "model.BuildEnterpriseReadAsBoolean" rather than always needing to do a "model.BuildEnterpriseReady == 'true'". Just my 2 cents and I feel that it could help alleviate the constant "== true" part.

Otherwise all looks good! Thanks

app/app.go Outdated Show resolved Hide resolved
app/integration_action.go Outdated Show resolved Hide resolved
app/integration_action.go Show resolved Hide resolved
app/server.go Outdated Show resolved Hide resolved
app/server.go Outdated Show resolved Hide resolved
app/server.go Outdated Show resolved Hide resolved
app/server.go Outdated Show resolved Hide resolved
store/sqlstore/user_store.go Outdated Show resolved Hide resolved
hahmadia
hahmadia approved these changes Sep 18, 2020
View reviewed changes
Copy link
Contributor

@hahmadia hahmadia left a comment

Choose a reason for hiding this comment

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

I guess my last comment would be using squirrel instead of plain sql due to our move to using squirrel. Otheriwse, with respect to above comments, everything else looks good.

srkgupta
srkgupta approved these changes Sep 24, 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.

Tested the updated PR along with it's dependent PRs and all the issues are now fixed and are working fine. Approving the PR.

@srkgupta srkgupta removed the 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review label Sep 24, 2020
@amyblais
Copy link
Member

@mkraft Kind note to review for v5.28.

@amyblais amyblais added CherryPick/Approved Meant for the quality or patch release tracked in the milestone and removed 2: Dev Review Requires review by a developer labels Sep 24, 2020
@catalintomai catalintomai merged commit f74b86a into mattermost:master Sep 25, 2020
@mattermod
Copy link
Contributor

Cherry pick is scheduled.

mattermost-build pushed a commit to mattermost-build/mattermost that referenced this pull request Sep 25, 2020
@catalintomai @mattermost-build
MM-28733 : Admin Advisor v2 (mattermost#15515)
2eeec1e 
(cherry picked from commit f74b86a)
@mattermost-build mattermost-build mentioned this pull request Sep 25, 2020
Closed
@mattermod mattermod added CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone and removed CherryPick/Approved Meant for the quality or patch release tracked in the milestone labels Sep 25, 2020
@amyblais amyblais added the Docs/Needed Requires documentation label Sep 25, 2020
catalintomai added a commit that referenced this pull request Sep 25, 2020
@catalintomai
MM-28733 : Admin Advisor v2 (#15515)
16e8066
@amyblais amyblais added the Changelog/Done Required changelog entry has been written label Sep 28, 2020
@justinegeffen justinegeffen mentioned this pull request Oct 9, 2020
Merged
@justinegeffen justinegeffen added Docs/Done Required documentation has been written and removed Docs/Needed Requires documentation labels Oct 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@esadur esadur esadur left review comments

@mkraft mkraft mkraft approved these changes

@srkgupta srkgupta srkgupta approved these changes

@hahmadia hahmadia hahmadia approved these changes

Assignees

@esadur esadur

Labels
Changelog/Done Required changelog entry has been written CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone Docs/Done Required documentation has been written
Projects
None yet
Milestone
v5.28.0
Development

Successfully merging this pull request may close these issues.
9 participants
@catalintomai @mattermod @srkgupta @jasonblais @amyblais @mkraft @hahmadia @esadur @justinegeffen