X Tutup
Skip to content

[tests-only] [full-ci] tests config to allow and not allow user to change their email and display name#39321

Closed
sakshamgurung wants to merge 1 commit intomasterfrom
api-test-user-change-display-name-email
Closed

[tests-only] [full-ci] tests config to allow and not allow user to change their email and display name#39321
sakshamgurung wants to merge 1 commit intomasterfrom
api-test-user-change-display-name-email

Conversation

@sakshamgurung
Copy link
Contributor

@sakshamgurung sakshamgurung commented Oct 5, 2021 •
edited
Loading

Description

Adds api tests for config: allow_user_to_change_display_name and allow_user_to_change_mail_address. Which allow or disallow user to change their display name and email respectively.

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • 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:
  • Changelog item, see TEMPLATE

@sakshamgurung sakshamgurung self-assigned this Oct 5, 2021
@sakshamgurung sakshamgurung force-pushed the api-test-user-change-display-name-email branch from 4e0b017 to 40f14f2 Compare October 5, 2021 06:05
sakshamgurung
sakshamgurung commented Oct 5, 2021
View reviewed changes
@sakshamgurung sakshamgurung force-pushed the api-test-user-change-display-name-email branch from 40f14f2 to 64988ba Compare October 7, 2021 09:38
phil-davis
phil-davis reviewed Oct 7, 2021
View reviewed changes
phil-davis
phil-davis reviewed Oct 7, 2021
View reviewed changes
phil-davis
phil-davis reviewed Oct 7, 2021
View reviewed changes
phil-davis
phil-davis reviewed Oct 7, 2021
View reviewed changes
@sakshamgurung sakshamgurung force-pushed the api-test-user-change-display-name-email branch from 64988ba to b1e7de7 Compare October 7, 2021 09:59
@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 7, 2021

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

@phil-davis
Copy link
Contributor

phil-davis commented Oct 7, 2021

@janackermann these scenarios demonstrate that the server does not seem to properly respect the allow_user_to_change_display_name and allow_user_to_change_mail_address at the API level.

A user seems to be able to change their email address with an API call, even when allow_user_to_change_mail_address is set to false.

Related issues are #39331 and #39332

Can you have a look please?

Maybe it can be fixed easily, and push the fix to this branch - proper BDD/TDD.

I am not 100% sure whether the forbidden change attempts should return 403 "forbidden" or 401 "not authorised". IMO 403 is better, but I see that existing Scenario: "a normal user should not be able to change their quota" already returns 401

@phil-davis phil-davis self-requested a review October 7, 2021 10:34
kiranparajuli589
kiranparajuli589 reviewed Oct 8, 2021
View reviewed changes
Copy link
Contributor

@kiranparajuli589 kiranparajuli589 left a comment

Choose a reason for hiding this comment

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

tests look good to me. please assign bug demonstration scenarios with issue tags.

@phil-davis phil-davis mentioned this pull request Oct 13, 2021
Closed
@phil-davis
Copy link
Contributor

phil-davis commented Oct 13, 2021

These tests have been cherry-picked into #39353

@phil-davis phil-davis closed this Oct 13, 2021
@phil-davis phil-davis deleted the api-test-user-change-display-name-email branch October 13, 2021 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@swoichha swoichha Awaiting requested review from swoichha

@Talank Talank Awaiting requested review from Talank

@SwikritiT SwikritiT Awaiting requested review from SwikritiT

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

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

1 more reviewer

@kiranparajuli589 kiranparajuli589 kiranparajuli589 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@sakshamgurung sakshamgurung

Labels

QA:team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sakshamgurung @phil-davis @kiranparajuli589
X Tutup