Skip to content
This repository has been archived by the owner on Oct 21, 2019. It is now read-only.

Move is_tech_lead to team_members and misc improvements #85

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bfbachmann
Copy link
Member

Changes

  • Moved is_tech_lead from members to team_members relation table. Now a member can be a lead of multiple teams and teams can have multiple leads.
  • Updated toggle-tech-leads and tech-leads accordingly.
  • Added some handy stuff to the Makefile
  • Updated Dockerfile.db so it now places SQL scripts in the DB container so we can easily run migrations from the outside without needing to copy over the scripts each time.
  • Made a few other minor improvements to Rocket response meesages

@coveralls
Copy link

coveralls commented Jun 23, 2018

Coverage Status

Coverage decreased (-2.2%) to 38.083% when pulling c3bcd84 on move-is_tech_lead into 9b6e70e on master.

Copy link
Member

@bobheadxi bobheadxi left a comment

Choose a reason for hiding this comment

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

I feel like the loss of the tech-lead level permissioning is a bit disappointing - can that still be implemented with this setup?

As an administrative tool, especially re: #84, I think it's pretty important that there is one level of permissions beneath admin.

# e.g: $ make migration MIGRATION=6_add_is_tech_lead
.PHONY: migrate
migrate:
@docker-compose exec postgres bash -c \
Copy link
Member

Choose a reason for hiding this comment

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

🔥

@@ -13,7 +13,7 @@ import (
func NewAddUserCmd(ch cmd.CommandHandler) *cmd.Command {
return &cmd.Command{
Name: "add-user",
HelpText: "Add a user to a team (admins and tech leads only)",
HelpText: "Add a user to a team (admins only)",
Copy link
Member

Choose a reason for hiding this comment

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

are we removing all tech-lead level permissions?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just took it out for now because I didn't want to write the extra code to check if a given member is a tech lead, since that would require an extra DB query each time we check the user's privilege level. But I was just lazy so I'll add it back in.

Copy link
Member

Choose a reason for hiding this comment

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

That would be great!

}
return names, noParams
if len(msg) == 0 {
msg = "There are currently no admins :feelsbadman:"
Copy link
Member

Choose a reason for hiding this comment

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

🤔

@@ -17,17 +16,20 @@ func NewTechLeadsCmd(ch cmd.CommandHandler) *cmd.Command {
}
}

// listAdmins displays Launch Pad admins
// listTechLeads displays Launch Pad tech leads
Copy link
Member

Choose a reason for hiding this comment

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

oops 😛

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants