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

fix(2989) Return users field data for web overriden shifts via public… #3303

Merged
merged 5 commits into from
Nov 16, 2023

Conversation

ravishankar15
Copy link
Contributor

What this PR does

The rolling_users field for the shift of type override created from web UI is populated in the users field of the public GET Shift API

Which issue(s) this PR fixes

#2989

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)

@ravishankar15 ravishankar15 requested a review from a team November 8, 2023 16:49
@CLAassistant
Copy link

CLAassistant commented Nov 8, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@matiasb matiasb left a comment

Choose a reason for hiding this comment

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

Changes make sense, added a few suggestions. Thanks!

**data,
)
on_call_shift.add_rolling_users([[other_user]])
on_call_shift.users.add(user)
Copy link
Contributor

Choose a reason for hiding this comment

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

When creating override shifts, the users field should be empty. We only expect rolling_users to be set up in this case.

and result["rolling_users"] is not None
):
result["users"] += [u for r in result["rolling_users"] for u in r]
result["users"] = list(set(result["users"])) # return unique users
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this could be simplified a bit?
result["users"] = list({u for r in result["rolling_users"] for u in r})

Copy link
Contributor

@matiasb matiasb left a comment

Choose a reason for hiding this comment

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

LGTM, one last comment.
And also, would you add an entry to the changelog about this update? Thanks!

"start": on_call_shift.start.strftime("%Y-%m-%dT%H:%M:%S"),
"rotation_start": on_call_shift.start.strftime("%Y-%m-%dT%H:%M:%S"),
"duration": int(on_call_shift.duration.total_seconds()),
"users": list({user.public_primary_key, other_user.public_primary_key}),
Copy link
Contributor

Choose a reason for hiding this comment

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

Note this may return users in any order, and then the test may randomly fail. It will be nice to make the test stable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have made it to use one user to avoid adding unnecessary sort function in the actual code.

@matiasb matiasb added the pr:no public docs Added to a PR that does not require public documentation updates label Nov 15, 2023
CHANGELOG.md Show resolved Hide resolved
@matiasb matiasb force-pushed the 2989-override-shift-fill-users branch from 42fa929 to d21a02a Compare November 16, 2023 13:04
@matiasb matiasb merged commit 1331857 into grafana:dev Nov 16, 2023
17 of 18 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.

3 participants