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

[tests-only][full-ci] add test to disable email notifications by federated user #10996

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

prashant-gurung899
Copy link
Contributor

Description

This PR adds test for disabling mail notification by federated user.

Scenario: federated user disables email notification

Related Issue

Motivation and Context

How Has This Been Tested?

  • Locally
  • CI

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

nirajacharya2
nirajacharya2 previously approved these changes Feb 19, 2025
Copy link
Contributor

@amrita-shrestha amrita-shrestha left a comment

Choose a reason for hiding this comment

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

current ocm setup doesn't throw any notification related to federated share. So i don't see any sense on adding step related to federated share. Did we get confirmation from developer how they going to implement federated share notification? developer can make different event for federated share and internal share.
If developer make different event then our scenario doesn't work like it should work.
So until developer implement feature or confirm us about concept and event type we should not add such scenario in assumption basis

@prashant-gurung899
Copy link
Contributor Author

current ocm setup doesn't throw any notification related to federated share. So i don't see any sense on adding step related to federated share. Did we get confirmation from developer how they going to implement federated share notification? developer can make different event for federated share and internal share. If developer make different event then our scenario doesn't work like it should work. So until developer implement feature or confirm us about concept and event type we should not add such scenario in assumption basis

Alright then! If that's the case, should we halt this PR and other OCM notification related PRs for now?
CC @nirajacharya2 @saw-jan @amrita-shrestha

@amrita-shrestha
Copy link
Contributor

current ocm setup doesn't throw any notification related to federated share. So i don't see any sense on adding step related to federated share. Did we get confirmation from developer how they going to implement federated share notification? developer can make different event for federated share and internal share. If developer make different event then our scenario doesn't work like it should work. So until developer implement feature or confirm us about concept and event type we should not add such scenario in assumption basis

Alright then! If that's the case, should we halt this PR and other OCM notification related PRs for now? CC @nirajacharya2 @saw-jan @amrita-shrestha

@prashant-gurung899 i think you can get confirmation from developer about event type.
@nirajacharya2 @saw-jan you guys want to discuss in standup ?

@prashant-gurung899
Copy link
Contributor Author

prashant-gurung899 commented Feb 21, 2025

@prashant-gurung899 i think you can get confirmation from developer about event type. @nirajacharya2 @saw-jan you guys want to discuss in standup ?

I have confirmed with the developer that the event name for OCM shares will include the term OCM.
I think that won't be much of a problem in future to refactor in out tests.

CC @saw-jan @nirajacharya2 @amrita-shrestha

@amrita-shrestha
Copy link
Contributor

@prashant-gurung899 i think you can get confirmation from developer about event type. @nirajacharya2 @saw-jan you guys want to discuss in standup ?

I have confirmed with the developer that the event name for OCM shares will include the term OCM. I think that won't be much of a problem in future to refactor in out tests.

CC @saw-jan @nirajacharya2 @amrita-shrestha

so disabling email notification will disable all email (internal email and federated email) ?

@prashant-gurung899
Copy link
Contributor Author

so disabling email notification will disable all email (internal email and federated email) ?

That's what the expected behaviour is. We aren't sure as OCM notifications aren't implemented yet.

@prashant-gurung899 prashant-gurung899 force-pushed the test/disable-ocm-email-notifications branch from f755bf3 to eb7163e Compare February 25, 2025 09:03
@saw-jan
Copy link
Member

saw-jan commented Feb 26, 2025

If the OCM notification events are not implemented yet then IMO we should wait for the implementation before adding the tests. Because we don't know what the behaviour and expectations will be. If everything follows our test expectations, then that's good. We can hold this until the implementation.

@prashant-gurung899
Copy link
Contributor Author

If the OCM notification events are not implemented yet then IMO we should wait for the implementation before adding the tests. Because we don't know what the behaviour and expectations will be. If everything follows our test expectations, then that's good. We can hold this until the implementation.

Okay! I am holding my OCM notification PRs until further implementation.

@saw-jan
Copy link
Member

saw-jan commented Feb 26, 2025

Please, move it to the backlog blocked

@prashant-gurung899 prashant-gurung899 marked this pull request as draft February 26, 2025 04:52
@prashant-gurung899
Copy link
Contributor Author

Moving this PR to backlog blocked because OCM notification events are not implemented yet. See comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants