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

add support for datetime on final_shifts API parameters #3103

Merged

Conversation

njohnstone2
Copy link
Contributor

@njohnstone2 njohnstone2 commented Oct 3, 2023

What this PR does

  • Changes the data type from DateField to DateTimeField on the final_shifts API endpoint
  • Accepts either a date or a datetime for the start_date and end_date parameters (e.g. 2021-01-01 or 2021-01-01T01:00)
  • Datetime granularity is in line with what is configurable on the schedule i.e. YYYY-MM-DD hh:mm
  • removes the rounding logic that modifies the query sent on the database

Which issue(s) this PR fixes

#3086

Checklist

  • Unit, integration, and e2e (if applicable) tests updated
  • Documentation added (or pr:no public docs PR label added if not required)
  • CHANGELOG.md updated (or pr:no changelog PR label added if not required)

@njohnstone2 njohnstone2 requested a review from a team October 3, 2023 07:55
Copy link
Contributor

@joeyorlando joeyorlando left a comment

Choose a reason for hiding this comment

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

overall LGTM. Thanks for the contribution! Once you update those tests I'll go ahead and approve/merge

@@ -1095,7 +1102,7 @@ def test_oncall_shifts_export_truncate_events(

# request shifts on a Tu (ie. 00:00 - 09:00)
url = reverse("api-public:schedules-final-shifts", kwargs={"pk": schedule.public_primary_key})
response = client.get(f"{url}?start_date=2023-01-03&end_date=2023-01-03", format="json", HTTP_AUTHORIZATION=token)
response = client.get(f"{url}?start_date=2023-01-03T09:00&end_date=2023-01-03T09:00", format="json", HTTP_AUTHORIZATION=token)
assert response.status_code == status.HTTP_200_OK

expected_on_call_times = {user1_public_primary_key: 9}
Copy link
Contributor

Choose a reason for hiding this comment

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

for these tests, do you mind parameterizing them to test both formats? 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added separate tests for validating the parameter formats. Let me know if i missed the mark on what you were asking.

This change essentially means that specifying a date only will result in a report that starts and ends at 00:00 for the time.

@joeyorlando joeyorlando added the pr:no public docs Added to a PR that does not require public documentation updates label Oct 3, 2023
@joeyorlando
Copy link
Contributor

@njohnstone2 also do you mind adding a small note about this to the changelog? thank you!

@njohnstone2
Copy link
Contributor Author

@joeyorlando Is there any update on this PR? Just making sure you are not waiting on anything from me

@joeyorlando
Copy link
Contributor

@njohnstone2 apologies, I was out for a bit. Merging this now. Thanks again for the contribution 🙌

@joeyorlando joeyorlando enabled auto-merge October 26, 2023 20:40
auto-merge was automatically disabled October 27, 2023 07:57

Head branch was pushed to by a user without write access

@joeyorlando joeyorlando enabled auto-merge October 27, 2023 10:18
auto-merge was automatically disabled October 28, 2023 00:05

Head branch was pushed to by a user without write access

@joeyorlando joeyorlando enabled auto-merge October 28, 2023 14:01
@joeyorlando joeyorlando added this pull request to the merge queue Oct 28, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 28, 2023
@joeyorlando joeyorlando added this pull request to the merge queue Oct 30, 2023
Merged via the queue into grafana:dev with commit c281484 Oct 30, 2023
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:no public docs Added to a PR that does not require public documentation updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants