Skip to content
This repository has been archived by the owner on Jul 23, 2024. It is now read-only.

FLPATH-237: notification-service: add User or Group validation to NotificationMessage creation #325

Merged
merged 2 commits into from
May 10, 2023

Conversation

nirarg
Copy link
Contributor

@nirarg nirarg commented May 9, 2023

What this PR does / why we need it:

When NotificationMessage is created, user must be assigned to it, this can be done by indicating
GroupName (all users under this group are assigned to the Notification)
Therefore, create API must include validation that there are users or groups added to the request

There is still open issue when creating NotificationMessage with GroupName
Opened another jira ticket for this (FLPATH-353)

Which issue(s) this PR fixes:

Fixes #FLPATH-237

Change type

  • New feature
  • Bug fix
  • Unit tests
  • Integration tests
  • CI
  • Documentation
  • Auto generated SDK code

Impacted services

  • Workflow Service
  • Notifivcation Service

Checklist

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.

@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented May 9, 2023

@nirarg: This pull request references FLPATH-237 which is a valid jira issue.

In response to this:

What this PR does / why we need it:

When NotificationMessage is created, user must be assigned to it, this can be done by indicating
GroupName (all users under this group are assigned to the Notification)
Therefore, create API must include validation that there are users or groups added to the request

There is still open issue when creating NotificationMessage with GroupName
Opened another jira ticket for this (FLPATH-353)

Which issue(s) this PR fixes:

Fixes #FLPATH-237

Change type

  • New feature
  • Bug fix
  • Unit tests
  • Integration tests
  • CI
  • Documentation
  • Auto generated SDK code

Impacted services

  • Workflow Service
  • Notifivcation Service

Checklist

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci openshift-ci bot requested review from pkliczewski and rgolangh May 9, 2023 06:29
@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented May 9, 2023

@nirarg: This pull request references FLPATH-237 which is a valid jira issue.

In response to this:

What this PR does / why we need it:

When NotificationMessage is created, user must be assigned to it, this can be done by indicating
GroupName (all users under this group are assigned to the Notification)
Therefore, create API must include validation that there are users or groups added to the request

There is still open issue when creating NotificationMessage with GroupName
Opened another jira ticket for this (FLPATH-353)

Which issue(s) this PR fixes:

Fixes #FLPATH-237

Change type

  • New feature
  • Bug fix
  • Unit tests
  • Integration tests
  • CI
  • Documentation
  • Auto generated SDK code

Impacted services

  • Workflow Service
  • Notifivcation Service

Checklist

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented May 9, 2023

@nirarg: This pull request references FLPATH-237 which is a valid jira issue.

In response to this:

What this PR does / why we need it:

When NotificationMessage is created, user must be assigned to it, this can be done by indicating
GroupName (all users under this group are assigned to the Notification)
Therefore, create API must include validation that there are users or groups added to the request

There is still open issue when creating NotificationMessage with GroupName
Opened another jira ticket for this (FLPATH-353)

Which issue(s) this PR fixes:

Fixes #FLPATH-237

Change type

  • New feature
  • Bug fix
  • Unit tests
  • Integration tests
  • CI
  • Documentation
  • Auto generated SDK code

Impacted services

  • Workflow Service
  • Notifivcation Service

Checklist

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented May 9, 2023

@nirarg: This pull request references FLPATH-237 which is a valid jira issue.

In response to this:

What this PR does / why we need it:

When NotificationMessage is created, user must be assigned to it, this can be done by indicating
GroupName (all users under this group are assigned to the Notification)
Therefore, create API must include validation that there are users or groups added to the request

There is still open issue when creating NotificationMessage with GroupName
Opened another jira ticket for this (FLPATH-353)

Which issue(s) this PR fixes:

Fixes #FLPATH-237

Change type

  • New feature
  • Bug fix
  • Unit tests
  • Integration tests
  • CI
  • Documentation
  • Auto generated SDK code

Impacted services

  • Workflow Service
  • Notifivcation Service

Checklist

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented May 9, 2023

@nirarg: This pull request references FLPATH-237 which is a valid jira issue.

In response to this:

What this PR does / why we need it:

When NotificationMessage is created, user must be assigned to it, this can be done by indicating
GroupName (all users under this group are assigned to the Notification)
Therefore, create API must include validation that there are users or groups added to the request

There is still open issue when creating NotificationMessage with GroupName
Opened another jira ticket for this (FLPATH-353)

Which issue(s) this PR fixes:

Fixes #FLPATH-237

Change type

  • New feature
  • Bug fix
  • Unit tests
  • Integration tests
  • CI
  • Documentation
  • Auto generated SDK code

Impacted services

  • Workflow Service
  • Notifivcation Service

Checklist

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented May 9, 2023

@nirarg: This pull request references FLPATH-237 which is a valid jira issue.

In response to this:

What this PR does / why we need it:

When NotificationMessage is created, user must be assigned to it, this can be done by indicating
GroupName (all users under this group are assigned to the Notification)
Therefore, create API must include validation that there are users or groups added to the request

There is still open issue when creating NotificationMessage with GroupName
Opened another jira ticket for this (FLPATH-353)

Which issue(s) this PR fixes:

Fixes #FLPATH-237

Change type

  • New feature
  • Bug fix
  • Unit tests
  • Integration tests
  • CI
  • Documentation
  • Auto generated SDK code

Impacted services

  • Workflow Service
  • Notifivcation Service

Checklist

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented May 9, 2023

@nirarg: This pull request references FLPATH-237 which is a valid jira issue.

In response to this:

What this PR does / why we need it:

When NotificationMessage is created, user must be assigned to it, this can be done by indicating
GroupName (all users under this group are assigned to the Notification)
Therefore, create API must include validation that there are users or groups added to the request

There is still open issue when creating NotificationMessage with GroupName
Opened another jira ticket for this (FLPATH-353)

Which issue(s) this PR fixes:

Fixes #FLPATH-237

Change type

  • New feature
  • Bug fix
  • Unit tests
  • Integration tests
  • CI
  • Documentation
  • Auto generated SDK code

Impacted services

  • Workflow Service
  • Notifivcation Service

Checklist

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

public boolean isValid(NotificationMessageCreateRequestDTO notificationMessage,
ConstraintValidatorContext context) {
if (notificationMessage == null) {
return true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

In this case we do not check whether usernames nor groupnames are empty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to return false

@nirarg nirarg force-pushed the notification-auth branch from 2107d63 to 372ebc9 Compare May 9, 2023 10:52
@pkliczewski
Copy link
Collaborator

/lgtm

@openshift-ci openshift-ci bot added the lgtm label May 9, 2023
@nirarg nirarg force-pushed the notification-auth branch from 372ebc9 to 800a1aa Compare May 9, 2023 14:10
@openshift-ci openshift-ci bot removed the lgtm label May 9, 2023
nirarg added 2 commits May 10, 2023 10:26
…sage creation

When NotificationMessage is created, user must be assigned to it, this can be done by indicating
GroupName (all users under this group are assigned to the Notification)
Therefore, create API must include validation that there are users or groups added to the request
Add more integration tests to validate that:
1. Auth works as expected in the API
2. NotificationMessage User or Group validation in create API
@nirarg nirarg force-pushed the notification-auth branch from 800a1aa to 40084e4 Compare May 10, 2023 08:02
@gciavarrini
Copy link
Contributor

/lgtm

@rgolangh
Copy link
Contributor

/approve

@openshift-ci
Copy link

openshift-ci bot commented May 10, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rgolangh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 4b2999e into rhdhorchestrator:main May 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants