-
Notifications
You must be signed in to change notification settings - Fork 11
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
Bendyworks/deduplicate starting salary #267
base: main
Are you sure you want to change the base?
Conversation
@@ -12,8 +12,10 @@ def new | |||
def edit; end | |||
|
|||
def create | |||
@employee = Employee.new(employee_params) | |||
salary_params = employee_params.dig(:salaries_attributes, "0") |
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.
I don't love this but I was having trouble making the employee model validate on create.
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.
I wonder if this validation could be handled in a before_action
method?
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.
Could adding :inverse_of
to the salary
model fix this?
If you change the start date, the starting salary date doesn't change with it, and can end up before the start date. |
@aterrype You are so right. Thanks for pointing that out. It should be fixed now. |
I'm merging in the rails bump on main now. Pull that in so our CI/CD pipeline is fixed |
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.
Aside from the comment that Amy left here, this looks good to me 👍
@aterrype Thanks for testing. The UI has changed so I don't think the error you were seeing is happening any more. |
Salaries are moved to relate to tenures and a new salary record is created on hire with a start date matching the first tenure. There are no data entry changes related to this but there may be a few more interstitial data points in the salary charts because of the new salary records.