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

[full-ci][tests-only]Expand tests coverage related to user with different role #5725

Merged
merged 2 commits into from Mar 21, 2023
Merged

[full-ci][tests-only]Expand tests coverage related to user with different role #5725

merged 2 commits into from Mar 21, 2023

Conversation

amrita-shrestha
Copy link
Contributor

@amrita-shrestha amrita-shrestha commented Mar 3, 2023 •
edited

Description

This PR expand tests coverage related to the user with all role to create, edit, delete, get the user
As Viktor Scharf told 403 Forbidden vs 401 Unauthorized. I would be expect 403 code. But we definitely have chaos here and often for developers - 401 ok. in case non-existent - 404 or 403. looking at witch checking is first.
So scenario like below has check for http status 401

- Non-admin user tries to delete his/her own account
- a normal user should not be able to edit another user's display name
- a normal user should not be able to reset the password of another user
- non-admin user tries to get the information of a user
- non-admin user tries to get all users
- non-admin user tries to get the group information of a user
- non-admin user tries to get users with a certain role

As Artur suggested running the authorization and non-existent related scenarios and adding them to the expected failure file

Related Issue

Motivation and Context

How Has This Been Tested?

  • Locally

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:

@amrita-shrestha amrita-shrestha self-assigned this Mar 3, 2023
@amrita-shrestha amrita-shrestha force-pushed the expandTestsForDifferentRoleUser branch 3 times, most recently from ac15d3f to 77e68b2 Compare March 7, 2023 02:47
@amrita-shrestha amrita-shrestha mentioned this pull request Mar 7, 2023
Closed
4 tasks
@amrita-shrestha amrita-shrestha force-pushed the expandTestsForDifferentRoleUser branch 5 times, most recently from e2b57bc to 9a77eea Compare March 15, 2023 06:09
@amrita-shrestha amrita-shrestha marked this pull request as ready for review March 15, 2023 06:10
saw-jan
saw-jan reviewed Mar 15, 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.

We can use username to delete user, so no need to get the user ID.
IMO, not needed for this PR

tests/acceptance/features/apiGraph/deleteUser.feature Outdated Show resolved Hide resolved
tests/acceptance/features/bootstrap/GraphContext.php Show resolved Hide resolved
tests/acceptance/features/bootstrap/GraphContext.php Outdated Show resolved Hide resolved
tests/acceptance/features/bootstrap/GraphContext.php Outdated Show resolved Hide resolved
tests/TestHelpers/GraphHelper.php Show resolved Hide resolved
@amrita-shrestha
Copy link
Contributor Author

amrita-shrestha commented Mar 15, 2023 •
edited

We can use username to delete user, so no need to get the user ID. IMO, not needed fo

i asked Viktor which method should we use he told me to use user id to delete the user. So i implemented deletion of user using user-id.

If you don't want to implement deletion of the user using UUID in this PR then i can make new PR to add scenario where user delete user by UUID

@saw-jan
Copy link
Member

saw-jan commented Mar 15, 2023 •
edited

^
Was just trying to reduce the code changes if we can use the existing helpers. But yeah, using ID seems to be the way to go as the clients use ids

saw-jan
saw-jan reviewed Mar 15, 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.

use setResponse() within the When implementation only.

tests/acceptance/features/bootstrap/GraphContext.php Outdated Show resolved Hide resolved
tests/acceptance/features/bootstrap/GraphContext.php Outdated Show resolved Hide resolved
tests/acceptance/features/bootstrap/GraphContext.php Outdated Show resolved Hide resolved
@amrita-shrestha
Copy link
Contributor Author

amrita-shrestha commented Mar 15, 2023 •
edited

use setResponse() within the When implementation only.

@saw-jan if we have to setResponse() in When step then we can do that by returning response from function why to remove function which help to implement DRY concept?

@saw-jan
Copy link
Member

saw-jan commented Mar 15, 2023

@saw-jan if we have to setResponse() in When step then we can do that by returning response from function why to remove function which help to implement DRY concept?

Yeah, my only suggestion is to use setResponse() within When. Ignore others

saw-jan
saw-jan reviewed Mar 17, 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

SagarGi
SagarGi reviewed Mar 17, 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 👍

@amrita-shrestha amrita-shrestha force-pushed the expandTestsForDifferentRoleUser branch 2 times, most recently from 686ad6a to fa6fea6 Compare March 20, 2023 03:54
@sonarcloud
Copy link

sonarcloud bot commented Mar 20, 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

SwikritiT
SwikritiT approved these changes Mar 20, 2023
View reviewed changes
Copy link
Contributor

@SwikritiT SwikritiT left a comment

Choose a reason for hiding this comment

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

LGTM

@amrita-shrestha amrita-shrestha merged commit 71011fc into master Mar 21, 2023
2 checks passed
@delete-merged-branch delete-merged-branch bot deleted the expandTestsForDifferentRoleUser branch March 21, 2023 04:18
ownclouders pushed a commit that referenced this pull request Mar 21, 2023
@amrita-shrestha
[full-ci][tests-only]Expand tests coverage related to user with diffe…
782bd51 
…rent role (#5725)

* Refactor tests related to different role

* Use setResponse from When step
@SwikritiT
Copy link
Contributor

SwikritiT commented Mar 27, 2023 •
edited

@amrita-shrestha just a reminder that this needs to be backported to stable-2.0 (with skip on stable tag probably?)

TODO

  • Backport to stable branch

amrita-shrestha added a commit that referenced this pull request Mar 28, 2023
@amrita-shrestha
[full-ci][tests-only]Expand tests coverage related to user with diffe…
3e36d16 
…rent role (#5725)

* Refactor tests related to different role

* Use setResponse from When step
@amrita-shrestha amrita-shrestha mentioned this pull request Mar 28, 2023
Closed
9 tasks
amrita-shrestha added a commit that referenced this pull request Mar 29, 2023
@amrita-shrestha
[full-ci][tests-only]Expand tests coverage related to user with diffe…
474d8aa 
…rent role (#5725)

* Refactor tests related to different role

* Use setResponse from When step
amrita-shrestha added a commit that referenced this pull request Mar 29, 2023
@amrita-shrestha
[full-ci][tests-only]Expand tests coverage related to user with diffe…
6ec17ff 
…rent role (#5725)

* Refactor tests related to different role

* Use setResponse from When step
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@individual-it individual-it individual-it left review comments

@SagarGi SagarGi SagarGi left review comments

@SwikritiT SwikritiT SwikritiT approved these changes

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

@ScharfViktor ScharfViktor ScharfViktor approved these changes

@PrajwolAmatya PrajwolAmatya Awaiting requested review from PrajwolAmatya

@grgprarup grgprarup Awaiting requested review from grgprarup

Assignees

@amrita-shrestha amrita-shrestha

Labels
QA:team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@amrita-shrestha @saw-jan @SwikritiT @individual-it @grgprarup @SagarGi @PrajwolAmatya @ScharfViktor