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

Add project to PSCID generation #5285

Merged
merged 3 commits into from Oct 31, 2019

Conversation

ridz1208
Copy link
Collaborator

@ridz1208 ridz1208 commented Oct 7, 2019

Brief summary of changes

Added option to generate PSCIDs using the project alias instead of the site alias. this is needed by several projects where permissions will be more project oriented then site.

to test:

  • in your config.xml change between static, siteAbbrev and projectAbbrev make sure the correct PSCID gets generated

@ridz1208 ridz1208 added Needs Rebase PR contains conflicts with the current target branch or was issued to the wrong branch Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix labels Oct 7, 2019
@@ -191,7 +191,7 @@ class Candidate
* configured to be true.
* @param string $sex sex, either 'Male' or 'Female'
* @param string|null $PSCID PSCID specified by the user, if available
* @param int|null $projectID The project ID, if available
* @param int $projectID The project ID, if available
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be nice to change this signature to actually pass a Project object instead of an ID here, if possible. We'd benefit from built-in validation this way.

Up to you though - I'm not sure how complicated this would be.

@@ -306,7 +307,8 @@ class Utility
*/
static function structureToPCRE(
array $structure,
?string $siteAbbrev = null
?string $siteAbbrev = null,
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider changing this to $prefix and only using one variable since the validation for $projAbbrev and $siteAbbrev do the same thing anyway.

This also allows calling code for project abbreviations to be Utility::structureToPCRE(array, ?string) instead of Utility::structureToPCRE(array, ?string, ?string) meaning that it doesn't need to always use null for the second argument.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@johnsaigle I'd like to keep both finally. I would like to be able to one day generate Ids with more than one prefix.

I know it's not the case right now but I think users should be able to generate PROJ-SITE-0000 IDs and that would not be possible with passing around a single prefix value.

@ridz1208 ridz1208 changed the base branch from minor to major October 30, 2019 16:13
@ridz1208 ridz1208 added Feature PR or issue introducing/requiring at least one new feature Needs Documentation PR awaiting proper documentation of the changes and removed Needs Rebase PR contains conflicts with the current target branch or was issued to the wrong branch Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix labels Oct 30, 2019
@ridz1208
Copy link
Collaborator Author

Documentation will be done in a different PR to avoid another rebase

ridz1208 and others added 2 commits October 31, 2019 13:26
Code

Update SQL/New_patches/2019-10-05-Add_alias_to_projects.sql

Co-Authored-By: PapillonMcGill <34311645+PapillonMcGill@users.noreply.github.com>
RB

fix tests

checkstatic

more checkstatic
@ridz1208 ridz1208 changed the base branch from major to 22.0-release October 31, 2019 17:27
@PapillonMcGill PapillonMcGill added the Passed Manual Tests PR has undergone proper testing by at least one peer label Oct 31, 2019
@driusan driusan merged commit dc71f60 into aces:22.0-release Oct 31, 2019
@ridz1208 ridz1208 added this to the 22.0.0 milestone Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature PR or issue introducing/requiring at least one new feature Needs Documentation PR awaiting proper documentation of the changes Passed Manual Tests PR has undergone proper testing by at least one peer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants