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.

Sign up for GitHub

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

Jump to bottom

[tests-only][full-ci] adding test for disabling in-app notifications for space disabled event #10895

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

[tests-only][full-ci] adding test for disabling in-app notifications for space disabled event #10895

wants to merge 1 commit into from
+160 −5

Conversation

nirajacharya2
Copy link
Contributor

Description

This PR adds test for disabling in-app notifications for space disabled event.
enabling or disabling notification for email is not available for this event.

Related Issue

Motivation and Context

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

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 self-assigned this Jan 20, 2025
@nirajacharya2 nirajacharya2 mentioned this pull request Jan 20, 2025
Open
13 tasks
@nirajacharya2 nirajacharya2 marked this pull request as ready for review January 20, 2025 10:48
@nirajacharya2 nirajacharya2 force-pushed the test-notification-space-disabled branch from 48dd824 to 4e99c78 Compare January 21, 2025 04:11
amrita-shrestha
amrita-shrestha reviewed Jan 21, 2025
View reviewed changes
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.

could you address this missing review from your before PR #10828 (comment)

tests/acceptance/features/apiSettings/notificationSetting.feature Outdated Show resolved Hide resolved
amrita-shrestha
amrita-shrestha requested changes Jan 21, 2025
View reviewed changes
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.

you are saying email is not possible for space disabling event but your scenario include that so confusing

@nirajacharya2 nirajacharya2 force-pushed the test-notification-space-disabled branch 2 times, most recently from cea5b7a to f94ed59 Compare January 21, 2025 04:53
amrita-shrestha
amrita-shrestha reviewed Jan 21, 2025
View reviewed changes
Copy link
Contributor

@amrita-shrestha amrita-shrestha left a comment •
edited
Loading

Choose a reason for hiding this comment

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

why is there Alice uploading same file twice ? i know its past PR task but could you plz look over it

Background:                                                                  
    Given these users have been created with default attributes:               
      | username |
      | Alice    |
      | Brian    |
    And user "Alice" has uploaded file with content "some data" to "lorem.txt"

Scenario: disable email notification
Given user "Alice" has uploaded file with content "some data" to "lorem.txt"

@nirajacharya2 nirajacharya2 force-pushed the test-notification-space-disabled branch from f94ed59 to 1259f23 Compare January 21, 2025 06:16
@nirajacharya2
Copy link
Contributor Author

why is there Alice uploading same file twice ? i know its past PR task but could you plz look over i

removed it

@nirajacharya2 nirajacharya2 force-pushed the test-notification-space-disabled branch from 1259f23 to 38a692d Compare January 21, 2025 06:40
@nirajacharya2 nirajacharya2 force-pushed the test-notification-space-disabled branch 3 times, most recently from 7d770e2 to 41910bf Compare January 22, 2025 04:22
nabim777
nabim777 reviewed Jan 22, 2025
View reviewed changes
tests/acceptance/features/apiSettings/notificationSetting.feature Outdated
Then the HTTP status code should be "200"
And there should be "1" notifications
And the JSON response should contain a notification message with the subject "Space shared" and the message-details should match
Copy link
Member

@nabim777 nabim777 Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
And the JSON response should contain a notification message with the subject "Space shared" and the message-details should match

instead of checking json, is it possible to do something like this 👇 ?

https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiSettings/notificationSetting.feature#L443

Copy link
Contributor

@amrita-shrestha amrita-shrestha Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@nirajacharya2 use such step rather than json

    And user "Brian" should get a notification with subject "Resource shared" and message:
      | message                                      |
      | Alice Hansen shared insideSpace.txt with you |
    But user "Brian" should not have a notification related to resource "insideSpace.txt" with subject "Resource unshared"

If you are not clear, than go to this merged PR and look https://github.com/owncloud/ocis/pull/10893/files#diff-28929748f6f2bda8fa8eb4dc0cd3e83ff0eacbcbbd89a5f6205c9683de994d66R322-R328

@nirajacharya2 nirajacharya2 force-pushed the test-notification-space-disabled branch from 41910bf to 9a23ed9 Compare January 23, 2025 04:26
amrita-shrestha
amrita-shrestha approved these changes Jan 23, 2025
View reviewed changes
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.

even though you didn't check share notification but check 1 notification exist so fine for me. lgtm

@amrita-shrestha amrita-shrestha self-requested a review January 23, 2025 05:00
@saw-jan saw-jan mentioned this pull request Jan 23, 2025
Open
9 tasks
@nirajacharya2 nirajacharya2 force-pushed the test-notification-space-disabled branch from 9a23ed9 to 49e3e49 Compare January 23, 2025 09:24
@nirajacharya2 nirajacharya2 requested a review from saw-jan January 23, 2025 09:26
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

prashant-gurung899
prashant-gurung899 reviewed Jan 23, 2025
View reviewed changes
tests/acceptance/features/apiSettings/notificationSetting.feature
Comment on lines +559 to +560
| message |
| Alice Hansen added you to Space new-space |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| message |
| Alice Hansen added you to Space new-space |
| message |
| Alice Hansen added you to Space new-space |

prashant-gurung899
prashant-gurung899 reviewed Jan 23, 2025
View reviewed changes
tests/acceptance/features/apiSettings/notificationSetting.feature
Comment on lines +473 to +478
"extension":{
"const": "ocis-accounts"
},
"bundle":{
"const": "profile"
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly in other places as well.

Suggested change
"extension":{
"const": "ocis-accounts"
},
"bundle":{
"const": "profile"
},
"extension":{ "const": "ocis-accounts" },
"bundle":{ "const": "profile" },

prashant-gurung899
prashant-gurung899 reviewed Jan 23, 2025
View reviewed changes
tests/acceptance/features/apiSettings/notificationSetting.feature
],
"properties":{
"id":{
"pattern":"%user_id_pattern%"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"pattern":"%user_id_pattern%"
"pattern":"%uuidv4_pattern%"

@prashant-gurung899
Copy link
Contributor

LGTM 👍 (except the above comments)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prashant-gurung899 prashant-gurung899 prashant-gurung899 left review comments

@phil-davis phil-davis Awaiting requested review from phil-davis

@anon-pradip anon-pradip Awaiting requested review from anon-pradip

@PrajwolAmatya PrajwolAmatya Awaiting requested review from PrajwolAmatya

@S-Panta S-Panta Awaiting requested review from S-Panta

@nabim777 nabim777 Awaiting requested review from nabim777

@amrita-shrestha amrita-shrestha Awaiting requested review from amrita-shrestha

@saw-jan saw-jan Awaiting requested review from saw-jan

Assignees

@nirajacharya2 nirajacharya2

Labels
QA:team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nirajacharya2 @prashant-gurung899 @saw-jan @amrita-shrestha @nabim777