DASSL / ClassDB

An open-source system to let students experiment with relational data
https://dassl.github.io/ClassDB/
Other
8 stars 2 forks source link

Add Team Views #239

Closed KevinKelly25 closed 6 years ago

KevinKelly25 commented 6 years ago

Adds the team views: ClassDB.Team and ClassDB.TeamMember. Fixes #236

The view TeamMember displays all roles that are added to a team and their respective team(s).

The view Team displays all teams with their attributes and an aggregate MemberCount.

In order to test this properly the ClassDB.addToTeam function from branch add-teams-membership will need to be added.

I added it manually to my local ClassDB and everything seems to working properly.

smurthys commented 6 years ago

@KevinKelly25 I like the idea of using the view TeamMember to define the view Team.

A minor thing: The comment on L19 should include something about teams.

I think the query to populate TeamMember can be simplified just a bit as follows to not use a derived table:

SELECT Team.RoleName Team, Member.RoleName Member
FROM Classdb.RoleBase Member 
JOIN ClassDB.RoleBase Team ON ClassDB.isMember(Member.RoleName, Team.RoleName)
WHERE NOT Member.IsTeam AND Team.IsTeam

Likewise, the definition of Team could more simply use a scalar-value sub-query to compute MemberCount instead of using a join query.

wildtayne commented 6 years ago

The code looks good overall. ClassDB.Team appears to not display teams that have no members, however. For example:

SELECT ClassDB.createTeam('MyTeam');
SELECT * FROM ClassDB.Team;

yields:

 teamname | fullname | schemaname | extrainfo | membercount
----------+----------+------------+-----------+-------------
(0 rows)

At a glance, I think the second change suggested by @smurthys would fix this by removing the inner join.

KevinKelly25 commented 6 years ago

@smurthys Thanks for the optimization idea, I used that method for both of the views in the new commit.

@srrollo Thanks for spotting that. As you said, with the new view the error does not persist.

smurthys commented 6 years ago

These changes compile and run correctly.

A tiny correction is possible on L138: change Classdb to ClassDB. Other than that, this PR seems ready to merge.

KevinKelly25 commented 6 years ago

Thanks @smurthys for finding the capitalization error.

I fixed that in the latest commit and also changed a comment to better fit the updated view structure.

KevinKelly25 commented 6 years ago

Thank you all for the reviews. I fixed the spacing in last commit @afig. Thanks for pointing that out.