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.

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

Adding attribute by team name #40

Merged
merged 4 commits into from
Jul 29, 2020
Merged

Adding attribute by team name #40

merged 4 commits into from
Jul 29, 2020

Conversation

nathanaelhoun
Copy link
Contributor

@nathanaelhoun nathanaelhoun commented Jun 15, 2020

Summary

  • Add attributes by team membership
  • Customise settings from UI with a team picker
  • Update README.md

I didn't tested if it doesn't break the GroupID version, because I don't have access to an E20 server, but I tested the others cases.

Inspired by #15

Ticket Link

No ticket

@hanzei hanzei added 1: PM Review Requires review by a product manager 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Jun 16, 2020
@hanzei
Copy link
Contributor

hanzei commented Jun 16, 2020

@aaronrothschild I would like to hear you thoughts on this feature proposal.

@marianunez
Copy link
Contributor

@nathanaelhoun Thank you for your contribution! Just to confirm, this new feature will allow custom attributes to be added to all user that belongs to the configured team(s)?

Update README.md (I don't know how to update the second screenshot here)

To update the image, you can either update it directly in Github and upload the image that you would want (you can drag and drop) or you can create a docs folder to add your image and then reference it as a relative path in url markdown added in the Readme - you can see an example of that here

@nathanaelhoun
Copy link
Contributor Author

Thanks for the tip @marianunez !

Yes exactly: every member of the team will have the team attribute. We use it because we don't have the AD groups in our Mattermost instance.

@aaronrothschild
Copy link
Contributor

How does this work when you belong to 20+ teams @nathanaelhoun ? Could you post some screenshots here?

@nathanaelhoun
Copy link
Contributor Author

nathanaelhoun commented Jun 16, 2020

@aaronrothschild This looks like this with one team for each letter in the alphabet. Looks pretty messy I guess.
image

But it looks normal if a reasonable number of custom attributes are set up (only for two teams and one personal attribute here)

image

A little clarification: the custom attribute only shows when it's configured for the specified team (it is not automatic for all the teams)
image

Copy link
Contributor

@marianunez marianunez left a comment

Choose a reason for hiding this comment

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

@nathanaelhoun overall changes look good. However, I would suggest we rely on saving the teamId in the configuration instead of team name in order for the check to be more robust and to be consistent with how we handle user and groups saved by id.

On the UI side, they could still select by team name if you implement a selector for team like we have there for user_input.

@nathanaelhoun
Copy link
Contributor Author

nathanaelhoun commented Jun 21, 2020

Thanks for the feeback @marianunez! It took me some time (React is totally new to me, I had to understand a few things), and I force-pushed as I rewrote everything I had changed previously.

Now there is a team selector in the UI, and teams are compared with their ids.
image

Let me know if it theses changes seems better to you 😄

// TeamsInput searches and selects teams displayed by display_name.
// Teams prop can handle the team object or strings directly if the team object is not available.
// Returns the selected team ids in the `OnChange` value parameter.
export default class TeamsInput extends React.Component {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not in scope with this PR, but it would be interesting to see if we can generalize this input component. Beside formatOptionLabel and getOptionValue, everything else is identical. It would be great if we would pass those as props and it can be reused for both teams and users. It could also possibly solve #28

Copy link
Contributor

@marianunez marianunez Jul 2, 2020

Choose a reason for hiding this comment

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

@nathanaelhoun would you be interested in giving #28 a shot in a separate PR and possibly exploring the approach above that would apply for users, groups and teams?

Copy link
Contributor

@marianunez marianunez left a comment

Choose a reason for hiding this comment

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

Great work @nathanaelhoun! The team selector looks great! 👍

Copy link
Contributor

@mickmister mickmister left a comment

Choose a reason for hiding this comment

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

Thanks @nathanaelhoun! I have some requests, mostly regarding style in the javascript side.

@nathanaelhoun
Copy link
Contributor Author

Thanks for the review @mickmister 😄

Copy link
Contributor

@mickmister mickmister left a comment

Choose a reason for hiding this comment

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

LGTM 👍 One non-blocking request on error string formatting

Co-authored-by: Michael Kochell <6913320+mickmister@users.noreply.github.com>
@marianunez
Copy link
Contributor

marianunez commented Jul 2, 2020

@aaronrothschild any concerns product-wise with moving forward with this feature?

@marianunez marianunez removed the 2: Dev Review Requires review by a core committer label Jul 2, 2020
@nathanaelhoun
Copy link
Contributor Author

Hi! Is there any news on this?

@marianunez
Copy link
Contributor

@nathanaelhoun apologies on the delay on this.

@aaronrothschild gentle ping to PM Review so we can go ahead with QA Review.

Copy link
Contributor

@aaronrothschild aaronrothschild left a comment

Choose a reason for hiding this comment

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

@nathanaelhoun Sorry for the delay, catching up on holiday relatyed backlog. Looks good, I am worried about too many Teams when there is a bunch setup, but that can be conquered later.

Thanks for your contribution!

@marianunez marianunez removed the 1: PM Review Requires review by a product manager label Jul 14, 2020
Copy link

@DHaussermann DHaussermann left a comment

Choose a reason for hiding this comment

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

Tested and passed

  • Confirmed members of the team specified are given the custom attributed as expected
  • Tested adding / removing teams as well as assigning custom attribute to members of multiple teams
  • Users team membership updated custom attribute as expected
  • Tested various combination of Team with the existing user grouping.
    No issues found
    LGTM!

@nathanaelhoun sorry for the delay in testing. Huge thanks for this enhancement!

@DHaussermann DHaussermann added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Jul 29, 2020
@mickmister
Copy link
Contributor

Looks good, I am worried about too many Teams when there is a bunch setup, but that can be conquered later.

@aaronrothschild Do you think this needs its own ticket?

Thanks so much @nathanaelhoun!

@mickmister mickmister merged commit 3c06086 into mattermost-community:master Jul 29, 2020
@nathanaelhoun nathanaelhoun deleted the adding-attribute-by-team branch August 2, 2020 12:17
@hanzei hanzei added this to the v1.3.0 milestone Aug 7, 2020
@larkox larkox mentioned this pull request Aug 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants