-
Notifications
You must be signed in to change notification settings - Fork 48
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
PCHR-3411: Remove unnecesary relationship types #2554
PCHR-3411: Remove unnecesary relationship types #2554
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks very nice. However I'm going to leave a comment about this below
@reneolivo I'm a bit concerned about deleting these relationship types. Running
On a vanilla CiviCRM site will return 3 rows
And all 3 of them would be deleted by this PR. Maybe that might be fine and they're just reserved for fun, but looking through the code I see places like this, this and this. I tested this out by deleting the relationship types from your PR and then trying to create a new contact in CiviCRM with "Current Employer" set it gave me this: There are also places where this error isn't thrown but it does depend on one of these relationship types. For example this code seems to depend on two relationship types. And indeed after deleting these types, going to "Advanced search", selecting some contacts and choosing "print mailing label" from the actions, and checking "Merge labels for contacts belonging to the same household" I got I'm guessing if I can find these problems in a few minutes there are plenty more that I've missed. If we really don't want to show these in CiviHR it might be a better option to try to hide them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just two codestyle comments
/** | ||
* Returns the Ids of the the relationship types to be disabled. | ||
* | ||
* @return Array[int] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the IDE not complain about this? I thought the standard way was to use int[]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The IDE even makes it pretty (Atom IDE).... I'll change it anyways!
'id' => $relationship['id'] | ||
]); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unforgivable, no empty line before closing bracked 😁 Did you run civilint
on this file?
/** | ||
* Returns all the relationships associated with a given type. | ||
* | ||
* @param int[] $relationshipTypeId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tiny mistake, this is int
and not int[]
c348166
to
b3125ec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good 🏎️
Overview
This PR removes relationships type that are not needed by CiviHR. The relationships types are:
Before
After
Technical Details
A new upgrader was implemented for HRCore since this is a change that affects all extensions: