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

Don't duplicate starting salary data #206

Open
aterrype opened this issue Dec 15, 2020 · 6 comments
Open

Don't duplicate starting salary data #206

aterrype opened this issue Dec 15, 2020 · 6 comments
Assignees

Comments

@aterrype
Copy link
Contributor

The starting_salary attribute on the employee model duplicates the salary information we already store in the salary table. We should remove the employee attribute so we aren't storing the same info twice.

@rogerroelofs
Copy link

This looks like a good starter for me. @aterrype, what do you think?

@rogerroelofs
Copy link

First question: If an employee is rehired, should the starting_salary be the original starting salary or the salary at rehire?

@aterrype
Copy link
Contributor Author

Yeah, I think this would be a good starter. As for your question, I think the idea is that if we get rid of starting_salary it wouldn't matter - we'd just get the first salary in the salary db instead. (Which would be the original starting salary, but that's how it works now so I think that's fine.)

@rogerroelofs
Copy link

@aterrype and @Code47X Now that I've been poking around a bit I'm starting to wonder if salaries should be related to tenures rather than the employee. Doing so would allow for having a starting and ending salary per tenure but feels like it is going to make getting data for charts more complicated. Any thoughts?

@aterrype
Copy link
Contributor Author

It would make sense for start/end dates, but I wonder how that would work for raises, since tenure doesn't care about raise dates. Another possibility might be to relate salaries and tenures through employees - I know you can do that in Rails, though I'm not super clear on the specifics so I can't say for sure that it would work here.

@Code47X
Copy link
Contributor

Code47X commented Mar 15, 2021

I haven't looked too deeply into the data structure so far, but it makes sense to me to have salaries related to tenures. As @aterrype mentioned, maybe we just set it up as Employee has_many :salaries, through: :tenures.

rogerroelofs pushed a commit that referenced this issue Mar 24, 2021
rogerroelofs added a commit that referenced this issue Mar 26, 2021
rogerroelofs added a commit that referenced this issue Mar 26, 2021
rogerroelofs added a commit that referenced this issue Mar 27, 2021
rogerroelofs added a commit that referenced this issue Mar 31, 2021
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

No branches or pull requests

3 participants