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.

Sign up for GitHub

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

Jump to bottom

Feat: update scheduled actions [SPA-284] #882

Merged
merged 7 commits into from Jul 14, 2021
Merged

Feat: update scheduled actions [SPA-284] #882

merged 7 commits into from Jul 14, 2021

Conversation

Spring3
Copy link
Contributor

@Spring3 Spring3 commented Jul 8, 2021 •
edited

Summary

Adds support for

plainClient.scheduledActions.update();
spaceClient.updateScheduledAction();
scheduledAction.update();

Description

Motivation and Context

Checklist (check all before merging)

  • Both unit and integration tests are passing
  • There are no breaking changes
  • Changes are reflected in the documentation

When adding a new method:

  • The new method is exported through the default and plain CMA client
  • All new public types are exported from ./lib/export-types.ts
  • Added a unit test for the new method
  • Added an integration test for the new method
  • The new method is added to the documentation

@Spring3 Spring3 marked this pull request as ready for review July 9, 2021 13:57
freakyfelt
freakyfelt reviewed Jul 9, 2021
View reviewed changes
lib/create-space-api.ts Outdated Show resolved Hide resolved
test/integration/scheduled-action-integration.ts Outdated Show resolved Hide resolved
test/integration/scheduled-action-integration.ts
datetime: new Date().toISOString(),
},
}),
testSpace.createScheduledAction({
Copy link
Contributor

Choose a reason for hiding this comment

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

Same with these

test/integration/scheduled-action-integration.ts
expect(fetchedAction.entity).to.eql(createdAction.entity)

// cleanup
await fetchedAction.delete()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to put clean up in a finally {} block?

Copy link
Contributor Author

@Spring3 Spring3 Jul 9, 2021 •
edited

Choose a reason for hiding this comment

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

I guess in case of an error, scheduled action wouldn't be created, so there would be nothing to cleanup, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

depends on where it failed (could also fail on fetch)

test/integration/scheduled-action-integration.ts Outdated Show resolved Hide resolved
@Spring3 Spring3 force-pushed the feat/update-scheduled-actions branch from 7dfc5b2 to 53ebd3e Compare July 13, 2021 00:20
luizfonseca
luizfonseca reviewed Jul 13, 2021
View reviewed changes
test/integration/scheduled-action-integration.ts
// Deletes a Release and all its Release Actions
await scheduledAction.delete()

const response = await testSpace.getScheduledActions({
Copy link
Member

Choose a reason for hiding this comment

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

wondering if wouldn't be better to try to find the specific scheduled action.

What I am thinking is that if for some reason another test is running this might become flakey since you querying for all scheduled actions.

Copy link
Member

Choose a reason for hiding this comment

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

we actually don't have a getScheduledAction added to the SDK? 🤦🏽

Copy link
Contributor Author

@Spring3 Spring3 Jul 13, 2021 •
edited

Choose a reason for hiding this comment

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

yeah, I noticed it as well and I wanted to add it, but then I heard "please not in the same PR", so I cut the ticket instead

luizfonseca
luizfonseca approved these changes Jul 13, 2021
View reviewed changes
Copy link
Member

@luizfonseca luizfonseca left a comment

Choose a reason for hiding this comment

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

Looks good! 👍🏽

Minor nitpicks:

  • Add JSDoc explaining how the use the scheduledAction.update() and that you have to update the object itself before calling it
  • Add a test covering/using the update from entities/scheduled-action.ts

@Spring3 Spring3 force-pushed the feat/update-scheduled-actions branch from 92bdcd4 to 41f91ac Compare July 13, 2021 14:57
@Spring3 Spring3 changed the title [WIP] Feat: update scheduled actions [SPA-284] Feat: update scheduled actions [SPA-284] Jul 13, 2021
@Spring3 Spring3 force-pushed the feat/update-scheduled-actions branch from b9e4e3c to 0489ad0 Compare July 13, 2021 16:44
@Spring3 Spring3 merged commit c504247 into master Jul 14, 2021
@Spring3 Spring3 deleted the feat/update-scheduled-actions branch July 14, 2021 11:53
luizfonseca
luizfonseca reviewed Jul 14, 2021
View reviewed changes
lib/entities/scheduled-action.ts
const params = getParams(this)

// eslint-disable-next-line @typescript-eslint/no-unused-vars
const { sys, ...payload } = this.toPlainObject()
Copy link
Member

Choose a reason for hiding this comment

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

defining it as _sys wouldn't make the eslint-disable disappear?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no

@ghost
Copy link

ghost commented Jul 14, 2021

🎉 This PR is included in version 7.28.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ghost ghost added the released label Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luizfonseca luizfonseca luizfonseca approved these changes

@mshaaban0 mshaaban0 Awaiting requested review from mshaaban0

@ChidinmaOrajiaku ChidinmaOrajiaku Awaiting requested review from ChidinmaOrajiaku

@freakyfelt freakyfelt Awaiting requested review from freakyfelt

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Spring3 @luizfonseca @freakyfelt