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

Feat add nodejs sdk methods for workflow overrides api #5020

Conversation

Prashant-dot1
Copy link
Contributor

@Prashant-dot1 Prashant-dot1 commented Dec 21, 2023

What change does this PR introduce?

This changes is to add a feature in node packages which is - adding all the methods supporting this API.

Tasks done till now and needs input -

  • Added methods for all the APIs in node packages.
  • Creating tests and readme -> still pending

Why was this change needed?

Closes #5005

@Prashant-dot1 Prashant-dot1 marked this pull request as draft December 21, 2023 19:51
@Prashant-dot1
Copy link
Contributor Author

@jainpawan21 @unicodeveloper , please go through the changes once - need your input on this .

I am still left with adding test and updating the readme , will continue on that

@jainpawan21
Copy link
Member

@jainpawan21 @unicodeveloper , please go through the changes once - need your input on this .

I am still left with adding test and updating the readme , will continue on that

Hi @Prashant-dot1

Let us know where you need help!

@Prashant-dot1 Prashant-dot1 marked this pull request as ready for review December 22, 2023 21:46
@Prashant-dot1
Copy link
Contributor Author

Prashant-dot1 commented Dec 22, 2023

Update : @jainpawan21

  • Added the test cases for workflow override API
  • Updated the README.md file as well
  • Attached ss of the passed test cases

Screenshot 2023-12-25 at 10 33 25 PM

PR can be considered for review

@VishalMCF
Copy link

@jainpawan21 I need help actually. I am writing the java, golang and rust sdk. I have finished java sdk but stuck on the delete response for workflow-override handler. Can you please get it added in the documentation because no information is available there. I am attaching a screenshot. Also from nodejs sdk i cannot infer the response body.
Screenshot 2023-12-30 at 5 03 52 AM

@Prashant-dot1
Copy link
Contributor Author

Prashant-dot1 commented Dec 31, 2023

Any update on this PR @jainpawan21 @unicodeveloper ?

@jainpawan21
Copy link
Member

@Prashant-dot1

As these methods are part of WorkflowOverrides class, so we don't need to use workflowOverrides again in methods name.

Please replace methods name as follows :-
updateWorkflowOverrideById ==> updateOneById
getworkflowOverrideById ==> getOneById
getWorkflowOverrides ==> list
getWorkflowOverrideByTenant ==> getOneByTenantIdandWorkflowId
updateWorkflowOverride ==> updateOneByTenantIdandWorkflowId

@jainpawan21
Copy link
Member

@Prashant-dot1

Take idea from topics methods
https://github.com/novuhq/novu/blob/next/packages/node/src/lib/topics/topics.spec.ts#L114

We need to include response as well in testing

expect(mockedAxios.post).toHaveBeenCalled();
expect(mockedAxios.post).toHaveBeenCalledWith('/workflow-overrides', {
workflowId: '8329rufivdsnvs9u334',
tenantId: 'wvnq340i2jfwqv392',
Copy link
Member

Choose a reason for hiding this comment

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

Add active and preferecensettings field fileds also in this test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Surely will do that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this change in the latest commit

@Prashant-dot1
Copy link
Contributor Author

@Prashant-dot1

As these methods are part of WorkflowOverrides class, so we don't need to use workflowOverrides again in methods name.

Please replace methods name as follows :- updateWorkflowOverrideById ==> updateOneById getworkflowOverrideById ==> getOneById getWorkflowOverrides ==> list getWorkflowOverrideByTenant ==> getOneByTenantIdandWorkflowId updateWorkflowOverride ==> updateOneByTenantIdandWorkflowId

Will make this change

Prashant-dot1 and others added 5 commits January 3, 2024 16:37
overrideID -> workflowId

Co-authored-by: Pawan Jain <jainpawan211199@gmail.com>
overrideID -> workflowId

Co-authored-by: Pawan Jain <jainpawan211199@gmail.com>
…c.ts


overrideID -> workflowId

Co-authored-by: Pawan Jain <jainpawan211199@gmail.com>
…c.ts


overrideID -> workflowId

Co-authored-by: Pawan Jain <jainpawan211199@gmail.com>
…c.ts


overrideID -> workflowId

Co-authored-by: Pawan Jain <jainpawan211199@gmail.com>
@jainpawan21
Copy link
Member

@jainpawan21 I need help actually. I am writing the java, golang and rust sdk. I have finished java sdk but stuck on the delete response for workflow-override handler. Can you please get it added in the documentation because no information is available there. I am attaching a screenshot. Also from nodejs sdk i cannot infer the response body. Screenshot 2023-12-30 at 5 03 52 AM

@VishalMCF
I fixed this in this PR
novuhq/docs#445
it will be available in a few days

@Prashant-dot1
Copy link
Contributor Author

@jainpawan21 made the required changes in the latest commit

@Prashant-dot1
Copy link
Contributor Author

Can this pull request be merged now? @jainpawan21

packages/node/README.md Outdated Show resolved Hide resolved
await novu.workflowoverrides.getWorkflowOverrides();
```

- #### get all workflow overrides from the 3rd page and with a limit of 20
Copy link
Member

Choose a reason for hiding this comment

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

remove this one and add params in above methods

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 1333 to 1338
'overrideId_123',
'tenantId_123',
{
active: false,
}
);
Copy link
Member

Choose a reason for hiding this comment

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

Make sure code formatting is correct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

corrected it

Comment on lines 1321 to 1323
'workflowId_123',
'tenantId_123'
);
Copy link
Member

Choose a reason for hiding this comment

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

Make sure code formatting is correct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

corrected it

);
```

- #### Update workflow override by tenantId
Copy link
Member

Choose a reason for hiding this comment

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

Update these headings as per methods name change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Prashant-dot1 and others added 6 commits January 8, 2024 09:32
Co-authored-by: Pawan Jain <jainpawan211199@gmail.com>
…rface.ts


overrideId -> workflowID

Co-authored-by: Pawan Jain <jainpawan211199@gmail.com>
…of github.com:Prashant-dot1/novu into featAdd-Nodejs-SDK-methods-for-Workflow-Overrides-API
@Prashant-dot1
Copy link
Contributor Author

@jainpawan21 made the necessary changes to README.md

packages/node/README.md Outdated Show resolved Hide resolved
@Prashant-dot1
Copy link
Contributor Author

@jainpawan21 why did the spell check test failed , do I have to make some changes for this ?

@jainpawan21 jainpawan21 merged commit b8c0c33 into novuhq:next Jan 10, 2024
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🚀 Feature: Add Nodejs SDK methods for Workflow Overrides API
3 participants