search

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

Full names of teams are optional, but createRole asserts that full name is not NULL or empty (E) #223

Closed afig closed 6 years ago

afig commented 6 years ago

During a meeting earlier today (6/12) it was decided that the fullName parameter for the future createTeam() function should be optional. However, createTeam() uses createRole() to register the role, and createRole() raises an EXCEPTION if fullName is NULL or empty.

A few possible solutions are:

  1. Have createRole() not enforce a non-empty fullName if isTeam is true. This will require it to be ensured that allowing a NULL or empty name does not lead to other issues. The ClassDB Role-Creation Process Diagram will also have to be updated.

  2. Make fullName for teams required. Note that it may be slightly confusing for users to be required to enter two names for a team.

  3. Leave fullName "optional", but coalesce with teamName before calling createRole().

Edit: The first solution is ideal, and is likely not be a huge effort to implement (there would not be any API changes since the behavior only impacts teams, which were not officially supported in 2.0). Solutions 2 and 3 are left as "backups" in the event of time constraints or issues with the first solution. Current development of teams is using solution 3 as a temporary measure so as to not slow down development

smurthys commented 6 years ago

I feel it is incorrect to classify this an issue in the sense of a "bug" because the M2 version of function createRole works exactly as expected and meets all requirements of M2.

It is better to capture this changed requirement in Issue #218 and update the issue text there which says "no change anticipated to createRole" (note that that text says "no change anticipated").

I propose re-classifying this issue to "not an issue" and closing it.

afig commented 6 years ago

I agree that this issue is not a bug. I logged this as an issue in the sense that there was a conflict between desired functionality and current implementation that would require making unrelated changes. However, I see how it does not fit in with other issues that have been logged in this repository, and agree that it would better fit in as additional text in 218.