Skip to content

TICKET 8321: add default role config option for test projects#170

Open
ukirst wants to merge 5 commits intoTestLinkOpenSourceTRMS:testlink_1_9from
ukirst:testlink_1_9
Open

TICKET 8321: add default role config option for test projects#170
ukirst wants to merge 5 commits intoTestLinkOpenSourceTRMS:testlink_1_9from
ukirst:testlink_1_9

Conversation

@ukirst
Copy link
Copy Markdown
Contributor

@ukirst ukirst commented Jul 3, 2018

@fmancardi Please review changes for "default test project role" feature and merge, if ok.
Thank you.

@fmancardi
Copy link
Copy Markdown
Contributor

function name => setUserRoleIDs($tproject_id, $role_id), has to be changed to setAllUsersToRole()

@fmancardi
Copy link
Copy Markdown
Contributor

function setUserRoleIDs($tproject_id, $role_id), need to be refactored to avoid multiple exit points

@fmancardi
Copy link
Copy Markdown
Contributor

$args->allRoles = $role_mgr->getAll($tprojectMgr->db, "WHERE id != 1 AND id != 2

the MAGIC NUMBERS 1 and 2 need to be replaced with the corresponding constants

@fmancardi
Copy link
Copy Markdown
Contributor

It seems that the default role on test project logic is only applied to PUBLIC projects, can you confirm this ? If answer is Yes then indication need to be provided to user who is configuring test project.
Also I do not think is OK, to delete all users roles on test project and then apply the default.
IMHO this is not a good approach.
Default can be applied when creating new users, and then a new feature update all roles to is needed (but IMHO already exists)

@ukirst
Copy link
Copy Markdown
Contributor Author

ukirst commented Jul 10, 2018

@fmancardi
I've implemented you review comments accordingly.
Users will also get notified now if they create a non-public project that the "default role" feature is not applicable in this case.

Regarding your concern about deleting user roles and applying the default:
I think when creating a project, it is ok to set the default role as no deletion is performed because no project roles exist yet for the new project.
In case an existing project is updated I've added a checkbox that the user must check manually in order to confirm that he/she wants to overwrite the existing project roles with the new default value.
If the checkbox is not checked the default role is not changed.
This way it is possible to update other project-related information/options without setting the default role again.
On the other hand, it seems logical to me to delete all user roles on the project and applying the new default value if the user selects a new test project default role and confirms the change explicitly (via the new checkbox).
I admit that this feature is similar to the bulk feature on the "Assign Test Project roles" page. But I don't think that this is a bad idea because on the "Test Project Management" page where you edit your project settings it makes sense to offer this option as well for the chosen project.

@fmancardi
Copy link
Copy Markdown
Contributor

Because the feature to set all users to a role for a test project exists, I will need to understand if this part of your implementation can be added.
If will be added both features need to be refactored to use common code if possible. This will be my task

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants