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

Update subscriber.py #53

Merged
merged 1 commit into from
Oct 4, 2023
Merged

Update subscriber.py #53

merged 1 commit into from
Oct 4, 2023

Conversation

prathapbelli
Copy link
Contributor

added channels field to SubscriberDto so that we can get credentials information in subscriber get api.

novu/dto/subscriber.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@ryshu ryshu left a comment

Choose a reason for hiding this comment

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

Their isn't any test, we don't known if it's work or not

@prathapbelli
Copy link
Contributor Author

Their isn't any test, we don't known if it's work or not

I have tested it locally with my self host novu it worked but I haven't updated test cases.

@unicodeveloper
Copy link
Contributor

Their isn't any test, we don't known if it's work or not

I have tested it locally with my self host novu it worked but I haven't updated test cases.

Please update test cases @prathapbelli

@prathapbelli
Copy link
Contributor Author

@ryshu @unicodeveloper I have added test case for the above, please take a look

tests/api/test_subscriber.py Outdated Show resolved Hide resolved
@ryshu
Copy link
Collaborator

ryshu commented Sep 5, 2023

@unicodeveloper Would it be possible to put me back among the bookstore's employees? I lost access when the use of 2FA became mandatory, but that blocks a certain number of features on reviews as a result.

@unicodeveloper
Copy link
Contributor

@unicodeveloper Would it be possible to put me back among the bookstore's employees? I lost access when the use of 2FA became mandatory, but that blocks a certain number of features on reviews as a result.

@ryshu Bookstore's employees? Please can you clarify what you mean?

@ryshu
Copy link
Collaborator

ryshu commented Sep 5, 2023

@unicodeveloper Would it be possible to put me back among the bookstore's employees? I lost access when the use of 2FA became mandatory, but that blocks a certain number of features on reviews as a result.

@ryshu Bookstore's employees? Please can you clarify what you mean?

Sry, I meant "Collaborator" on the Github repository

@unicodeveloper
Copy link
Contributor

@unicodeveloper Would it be possible to put me back among the bookstore's employees? I lost access when the use of 2FA became mandatory, but that blocks a certain number of features on reviews as a result.

@ryshu Bookstore's employees? Please can you clarify what you mean?

Sry, I meant "Collaborator" on the Github repository

Okay sure, doing that now...

@unicodeveloper
Copy link
Contributor

unicodeveloper commented Sep 5, 2023

Wait, do you still have 2FA activated? because we need collaborators to have 2FA turned on @ryshu

@ryshu
Copy link
Collaborator

ryshu commented Sep 5, 2023

Wait, do you still have 2FA activated? because we need collaborators to have 2FA turned on @ryshu

I looked and yes normally, my 2FA on a Microsoft authenticator is well configured

@unicodeveloper
Copy link
Contributor

Wait, do you still have 2FA activated? because we need collaborators to have 2FA turned on @ryshu

I looked and yes normally, my 2FA on a Microsoft authenticator is well configured

Okay on it.

@unicodeveloper
Copy link
Contributor

I just added you @ryshu. Please confirm.

@ryshu
Copy link
Collaborator

ryshu commented Sep 5, 2023

@prathapbelli FYI, your commits do not respect the naming convention, and the pipeline will not pass.

See https://github.com/novuhq/novu-python/blob/main/CONTRIBUTING.rst for more details.

Also, the code doesn't work in Python 3.8 (list doesn't exist, might be imported from typing)

from typing import List

@unicodeveloper
Copy link
Contributor

Any updates here? @prathapbelli

@unicodeveloper
Copy link
Contributor

@ryshu Please can you help edit this PR so we can close this out too?

@ryshu
Copy link
Collaborator

ryshu commented Sep 13, 2023

@ryshu Please can you help edit this PR so we can close this out too?

@unicodeveloper

I won't have time today, but I can.

We deliver the alpha without it to unlock the other topics?

@unicodeveloper
Copy link
Contributor

Alright, that also works @ryshu

@ryshu ryshu changed the base branch from main to alpha October 4, 2023 20:53
@ryshu
Copy link
Collaborator

ryshu commented Oct 4, 2023

@unicodeveloper @Cliftonz Should be ok for a review

@ryshu ryshu self-assigned this Oct 4, 2023
@Cliftonz Cliftonz merged commit ce7ad38 into novuhq:alpha Oct 4, 2023
unicodeveloper pushed a commit that referenced this pull request Oct 14, 2023
# [1.5.0](v1.4.0...v1.5.0) (2023-10-14)

### Bug Fixes

* **change:** promoted path parms is required for listing ([#86](#86)) ([9758660](9758660))
* **notification:** list notifications should return a paginated response ([#87](#87)) ([1b47e10](1b47e10)), closes [#88](#88)

### Features

* added missing methods in notification group class ([#83](#83)) ([951d893](951d893))
* **subscriber:** add channels fields on DTO ([#53](#53)) ([b8809b5](b8809b5))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants