chiefonboarding / ChiefOnboarding

Free and open-source employee onboarding platform. Onboard new hires through Slack or the web portal.
https://chiefonboarding.com
GNU Affero General Public License v3.0
677 stars 124 forks source link

Fix new_hire_views.py #343

Closed helgio closed 1 year ago

helgio commented 1 year ago

Fix filter for finding sequences that will not be triggered. It should be gte not lte

GDay commented 1 year ago

Thanks for the PR!

Could you elaborate a bit on why it should be gte instead of lte? This test fails now: https://github.com/helgio/ChiefOnboarding/blob/896d9c77c22b16c8ffe45ac51c9015563eac80f5/back/admin/people/tests.py#L183-L226

The "before a new hire starts" items counts descending (so 5 days before the start day, then 2 days before start day... ), while "after the new hire starts" counts ascending.

helgio commented 1 year ago

Hi I hope I am not misunderstanding this, but like I understand the code is behaving, without my alteration:

This is the scope you are getting:

---- past --- Today ----- 1 day before ---- start date ---- 1 day after ___xxxxxxxxxxxxxxxxxxxxxxxx

you want this:

---- past --- Today ----- 1 day before ---- start date ---- 1 day after xxxxxxxxxxxx

Like I understand the test: you are creating a task that will trigger 1 day before the starting day. The starting day is in 7 days, the task in the test will be triggered. But the test and the code are saying it will not.

Time in dates can be so confusing, I am getting a headache writing this :)

GDay commented 1 year ago

@helgio My apologies, you are absolutely right! The test case and the original code are incorrect. Your fix is great!

If it's not too much of a hassle, could you fix the test case? I will merge this once tests pass. If you can't, just let me know and I will fix it.

Thanks a lot!

helgio commented 1 year ago

My pleasure, I will fix the test :)