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

Payroll: remove terminated storage and change exist modifier for activeness check #610

Merged
merged 1 commit into from
Mar 25, 2019

Conversation

sohkai
Copy link
Contributor

@sohkai sohkai commented Dec 14, 2018

cc @lmcorbalan @bingen

Changes the employeeExists modifier to instead check if an employee is still active (their end date hasn't been reached).

This allows the app to:

  • Remove the terminated storage for each employee, as invoking terminateEmployeeNow() twice in the same block will fail (employee is only active if their end date hasn't been reached)
  • Change details about a yet-to-be "terminated" employee (one who has an non-MAX_UINT64 end date but whose end date has not been reached), such as salary, their actual end date, etc.

Note that this does allow you to "terminate" an employee multiple times, if you set their end date in the future. I'd argue this is a terminology problem though (see below).


Further potential changes:

  • We could change the terminology around "terminating": terminateEmployeeNow() makes sense, but terminateEmployee() should be changed to setEmployeeEndDate() so that its intention is more clear

Todo:

  • Add more tests for terminating employees
  • Change terminology to setEmployeeEndDate()

@sohkai sohkai force-pushed the payroll-terminated-employee branch from 1af9b68 to 6f6a701 Compare December 14, 2018 11:15
@luisivan luisivan mentioned this pull request Mar 22, 2019
21 tasks
@bingen bingen requested review from bingen and facuspagnuolo March 22, 2019 15:32
Copy link
Contributor

@bingen bingen left a comment

Choose a reason for hiding this comment

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

This looks good to me, and I agree with the terminology comments.

Copy link
Contributor

@facuspagnuolo facuspagnuolo left a comment

Choose a reason for hiding this comment

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

LGTM as well, lets merge it and I will take care of the tests in another PR

@facuspagnuolo facuspagnuolo force-pushed the payroll-terminated-employee branch from 6f6a701 to 2cbff2e Compare March 22, 2019 19:22
@coveralls
Copy link

Coverage Status

Coverage remained the same at 96.734% when pulling 2cbff2e on payroll-terminated-employee into 93f0ffb on master.

@facuspagnuolo facuspagnuolo merged commit 4723edb into master Mar 25, 2019
@facuspagnuolo facuspagnuolo deleted the payroll-terminated-employee branch March 25, 2019 13:50
@sohkai sohkai changed the title [WIP] Payroll: remove terminated storage and change exist modifier for activeness check Payroll: remove terminated storage and change exist modifier for activeness check Mar 25, 2019
@sohkai sohkai removed the wip label Mar 25, 2019
ramilexe pushed a commit to ConsiderItDone/aragon-apps that referenced this pull request Dec 9, 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

Successfully merging this pull request may close these issues.

4 participants