-
Notifications
You must be signed in to change notification settings - Fork 5
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
Update records with datastore.update
when advancing rotations
#10
base: main
Are you sure you want to change the base?
Conversation
FYI: To appease the type checker, I had to merge the changes from #8 into this branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested this and it's looking great! 🎉 (Also thanks for including a link trigger to test the advancing of the rotations! 🙌)
A few thoughts before merging:
- The base of this PR is in a strange state due to the
deno-slack-sdk
version bump down we had to do in this PR, with Fil’s PR (Bump deno SDK version to latest, wrap format_rotation property with new SDK DefineProperty #8) bumping that version back up to1.6.0
, which will no longer be the latest version after Beta Release 28 is completed this week. I could see this causing some sort of merge conflict depending on when this is merged in, specifically with thedeno-slack-sdk
version being1.6.1
in the pre-release branch. If this PR is merged beforepre-release-0316
is, we’ll definitely want to rebase that branch againstmain
to avoid any conflicts. If this PR is merged afterpre-release-0316
is merged in, then we’ll need to rebase this branch to avoid conflicts. - On a somewhat related note, I also wanted to double check with @filmaj that the proposed changes in Bump deno SDK version to latest, wrap format_rotation property with new SDK DefineProperty #8 were good to be incorporated into this PR and merged in. If it’s preferred to keep those changes separate, we can also merge that PR first and then rebase this on top. If we do end up merging Fil's PR into
main
, that’ll be another moving part that may require updatingpre-release-0316
depending on merging timing, so that might be another thing to consider with regards to consolidating these changes or not.
Let me know if there are any questions with this!
Type of change
Summary
To showcase a new datastore operation, this PR replaces the
datastore.put
call in theadvance_rotation
function withdatastore.update
. With this call, only the fields passed into this function will be updated with the primary keychannel
, while those unspecified are not modified.The versions of Deno modules were also bumped to support this new function.
Testing
To verify the changes of this PR manually, install this app to a workspace and create a rotation with more than one user.
Then create a trigger with the following definition to force a rotation, and trip this trigger:
Requirements