-
Notifications
You must be signed in to change notification settings - Fork 669
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
SMQ-2706 - Add domain route to message topic #2765
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2765 +/- ##
==========================================
+ Coverage 27.54% 30.56% +3.02%
==========================================
Files 351 207 -144
Lines 55347 37371 -17976
==========================================
- Hits 15243 11424 -3819
+ Misses 39346 25328 -14018
+ Partials 758 619 -139 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
All looks good, except i have one comment which need testing, I will create issue for this comment
channels/private/service.go
Outdated
d, err := svc.domains.RetrieveEntity(ctx, req.DomainID) | ||
if err != nil { | ||
return errors.Wrap(svcerr.ErrAuthorization, err) | ||
} | ||
if d.Status != dom.EnabledStatus { | ||
return errors.Wrap(svcerr.ErrAuthorization, errDisabledDomain) | ||
} |
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.
If req.ClientType
is policies.UserType
, then we might need to check user is present in domain or not.
I'm not sure about this , but i need to test this case.
Steps to replicate :
- We need to add member to domain role
- Add access to channel
- Remove member from domain role
- then test reading value of message via reader.
On removal of member from domain role and if member is not removed from other entities and removed only from domains, then even after removal of member will still have access to channels.
User removal PR will solve this case.
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.
I have created ticket here #2767
81565c6
to
45d192d
Compare
a5d6ec4
to
4160a8f
Compare
Signed-off-by: Felix Gateru <felix.gateru@gmail.com>
Signed-off-by: Felix Gateru <felix.gateru@gmail.com>
Signed-off-by: Felix Gateru <felix.gateru@gmail.com>
Signed-off-by: Felix Gateru <felix.gateru@gmail.com>
Signed-off-by: Felix Gateru <felix.gateru@gmail.com>
Signed-off-by: Felix Gateru <felix.gateru@gmail.com>
4160a8f
to
ff19935
Compare
What type of PR is this?
This is a feature because it adds domain route to message topic.
What does this do?
Which issue(s) does this PR fix/relate to?
Have you included tests for your changes?
Yes, tests have been updated.
Did you document any new/modified feature?
Yes, in code documentation has been updated.
Notes
None