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] refactoring the user delete code from ocs to graphapi #7020

Merged
merged 4 commits into from
Oct 31, 2023
Merged

[tests-only][full-ci] refactoring the user delete code from ocs to graphapi #7020

merged 4 commits into from
Oct 31, 2023

Conversation

S-Panta
Copy link
Contributor

@S-Panta S-Panta commented Aug 11, 2023 •
edited
Loading

Description

This PR refactors the user deletion test code from provisioning to graphapi

Related Issue

Motivation and Context

How Has This Been Tested?

  • test environment:
  • 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:

@S-Panta S-Panta force-pushed the refactoring-user-deletion-testcode-to-graphapi branch from 6f27302 to 8b78a61 Compare August 14, 2023 06:15
@S-Panta S-Panta self-assigned this Aug 14, 2023
@S-Panta S-Panta marked this pull request as ready for review August 14, 2023 06:59
@S-Panta
Copy link
Contributor Author

S-Panta commented Aug 14, 2023

The scenario on line 13 in moveReceivedShare.feature file could be passed if the line Then the OCS status code of responses on all endpoints should be "100" be removed. Since the usage of GraphAPI is done while implementing this, it makes no sense to ask and check for the OCS status code. Should I then remove this step from the feature file and from the expected failure file to pass the drone? Also, there's a bug report related to this scenario here. #3621
@saw-jan @SwikritiT

@SwikritiT
Copy link
Contributor

The scenario on line 13 in moveReceivedShare.feature file could be passed if the line Then the OCS status code of responses on all endpoints should be "100" be removed. Since the usage of GraphAPI is done while implementing this, it makes no sense to ask and check for the OCS status code. Should I then remove this step from the feature file and from the expected failure file to pass the drone? Also, there's a bug report related to this scenario here. #3621 @saw-jan @SwikritiT

the ocs status code check is correct for these steps and should be there. We use graph API for user/space related operations till now and sharing still uses ocs so the check is relevant.

    When user "Alice" shares folder "TMP" with group "grp1" using the sharing API
    And user "Brian" accepts share "/TMP" offered by user "Alice" using the sharing API
    And user "Carol" accepts share "/TMP" offered by user "Alice" using the sharing API

@SwikritiT
Copy link
Contributor

SwikritiT commented Aug 14, 2023 •
edited
Loading

The scenario on line 13 in moveReceivedShare.feature file could be passed if the line Then the OCS status code of responses on all endpoints should be "100" be removed. Since the usage of GraphAPI is done while implementing this, it makes no sense to ask and check for the OCS status code. Should I then remove this step from the feature file and from the expected failure file to pass the drone? Also, there's a bug report related to this scenario here. #3621 @saw-jan @SwikritiT

the ocs status code check is correct for these steps and should be there. We use graph API for user/space related operations till now and sharing still uses ocs so the check is relevant.

    When user "Alice" shares folder "TMP" with group "grp1" using the sharing API
    And user "Brian" accepts share "/TMP" offered by user "Alice" using the sharing API
    And user "Carol" accepts share "/TMP" offered by user "Alice" using the sharing API

Note soon the ocs sharing will also be deprecated and eventually removed but till then these steps should stay

@S-Panta S-Panta mentioned this pull request Sep 6, 2023
Closed
2 tasks
@S-Panta S-Panta force-pushed the refactoring-user-deletion-testcode-to-graphapi branch 2 times, most recently from df3816d to f39f4c9 Compare September 11, 2023 09:01
@S-Panta S-Panta force-pushed the refactoring-user-deletion-testcode-to-graphapi branch 4 times, most recently from 21f7988 to 1c1dc20 Compare September 25, 2023 10:50
This was referenced Sep 25, 2023
Closed
Closed
@S-Panta S-Panta force-pushed the refactoring-user-deletion-testcode-to-graphapi branch from 1c1dc20 to 11ac129 Compare September 25, 2023 12:14
@S-Panta
Copy link
Contributor Author

S-Panta commented Oct 4, 2023

@S-Panta S-Panta force-pushed the refactoring-user-deletion-testcode-to-graphapi branch from cd0a41c to 6f329dc Compare October 6, 2023 10:54
@S-Panta S-Panta force-pushed the refactoring-user-deletion-testcode-to-graphapi branch 3 times, most recently from 4727227 to b21815c Compare October 9, 2023 06:10
@S-Panta S-Panta force-pushed the refactoring-user-deletion-testcode-to-graphapi branch from b21815c to 00b8450 Compare October 9, 2023 10:22
saw-jan
saw-jan approved these changes Oct 10, 2023
View reviewed changes
Copy link
Member

@saw-jan saw-jan left a comment

Choose a reason for hiding this comment

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

LGTM

grgprarup
grgprarup reviewed Oct 11, 2023
View reviewed changes
tests/acceptance/features/coreApiShareManagementToShares/moveReceivedShare.feature
@@ -21,7 +21,7 @@ Feature: sharing
And user "Brian" moves folder "/Shares/TMP" to "/Shares/new" using the WebDAV API
And the administrator deletes user "Carol" using the provisioning API
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
And the administrator deletes user "Carol" using the provisioning API
And the administrator deletes user "Carol" using the provisioning API

Why is this step needed and why aren't we refactoring the step from provisioning to graph?

Copy link
Member

Choose a reason for hiding this comment

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

internally should use graph. but step cleanup can be in upcoming PRs. But it's hard to come with great naming.

Copy link
Contributor

Choose a reason for hiding this comment

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

@saw-jan I might be wrong. But can't we only use the When step for the move (rename) step, and sharing and accepting share can be done using the Given step? IMHO it's better if we can do small fixes in the same PR where we encounter the problem.
Also, why are we deleting the user Carol?

Copy link
Member

Choose a reason for hiding this comment

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

why are we deleting the user Carol?

I really have no idea. what the scenario suggests?

MHO it's better if we can do small fixes in the same PR where we encounter the problem.

Yeah, right. this type of PRs are the opportunity to fix the test if required but I don't have any guess about how much the file changes would be if we do RELATED minor refactor. Depends on @S-Panta

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suggest that we do these things in another PR. Some scenario seems to be refactored and added in this file. There are already more changes in this PR,

Copy link
Member

Choose a reason for hiding this comment

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

Then please, follow up with it after this is merged

@S-Panta S-Panta force-pushed the refactoring-user-deletion-testcode-to-graphapi branch 2 times, most recently from b3c2ae5 to 2628851 Compare October 30, 2023 08:44
@S-Panta S-Panta force-pushed the refactoring-user-deletion-testcode-to-graphapi branch from 2628851 to 6868988 Compare October 30, 2023 11:56
@sonarcloud
Copy link

sonarcloud bot commented Oct 30, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

SagarGi
SagarGi reviewed Oct 31, 2023
View reviewed changes
Copy link
Member

@SagarGi SagarGi left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@saw-jan saw-jan merged commit cc1f93e into master Oct 31, 2023
2 checks passed
@delete-merged-branch delete-merged-branch bot deleted the refactoring-user-deletion-testcode-to-graphapi branch October 31, 2023 06:16
ownclouders pushed a commit that referenced this pull request Oct 31, 2023
@S-Panta
[tests-only][full-ci] refactoring the user delete code from ocs to gr…
ef36a30 
…aphapi (#7020)

* addressing the reviews

* addressing the review

* refactored test code

* updated expected failures file
S-Panta added a commit that referenced this pull request Nov 8, 2023
@S-Panta
[tests-only][full-ci] refactoring the user delete code from ocs to gr…
ac03df1 
…aphapi (#7020)

* addressing the reviews

* addressing the review

* refactored test code

* updated expected failures file
@S-Panta S-Panta mentioned this pull request Nov 8, 2023
Merged
S-Panta added a commit that referenced this pull request Nov 9, 2023
@S-Panta
[tests-only][full-ci] refactoring the user delete code from ocs to gr…
a412db9 
…aphapi (#7020)

* addressing the reviews

* addressing the review

* refactored test code

* updated expected failures file
S-Panta added a commit that referenced this pull request Nov 9, 2023
@S-Panta
[tests-only][full-ci] refactoring the user delete code from ocs to gr…
b555d51 
…aphapi (#7020)

* addressing the reviews

* addressing the review

* refactored test code

* updated expected failures file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grgprarup grgprarup grgprarup left review comments

@SagarGi SagarGi SagarGi left review comments

@saw-jan saw-jan saw-jan approved these changes

@amrita-shrestha amrita-shrestha amrita-shrestha approved these changes

@PrajwolAmatya PrajwolAmatya Awaiting requested review from PrajwolAmatya

@SwikritiT SwikritiT Awaiting requested review from SwikritiT

@KarunAtreya KarunAtreya Awaiting requested review from KarunAtreya

@nabim777 nabim777 Awaiting requested review from nabim777

@nirajacharya2 nirajacharya2 Awaiting requested review from nirajacharya2

Assignees

@S-Panta S-Panta

Labels
QA:team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@S-Panta @SwikritiT @grgprarup @SagarGi @saw-jan @amrita-shrestha