Skip to content
This repository was archived by the owner on Nov 24, 2025. It is now read-only.

Conversation

@tinfoil-knight
Copy link
Contributor

@tinfoil-knight tinfoil-knight commented Mar 10, 2023

Closes: #7249

Currently, a user updating their email to one that's already in use returns an 500 Internal Server Error. This PR updates the implementation to ensure that a 400 Bad Request error with a description is returned instead.


Which Traffic Control components are affected by this PR?

  • Traffic Ops

What is the best way to verify this PR?

  • Create a user (using the POST /users endpoint)
  • Attempt to modify the user (using the PUT /user/current endpoint)
  • Verify that you're receiving an error with HTTP status code 400 with a message mentioning that the email already exists

If this is a bugfix, which Traffic Control versions contained the bug?

Wasn't able to track down the exact release where this bug originated. This bug is present in master branch & #3996 is the PR where the endpoint was first written for API V3.

PR submission checklist

  • This PR has tests
  • This PR has documentation
    PR doesn't need any separate documentation since only the error handling behaviour has been changed. API docs only document the request & response structure.
  • This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY (see the Apache Software Foundation's security guidelines for details)

@tinfoil-knight
Copy link
Contributor Author

tinfoil-knight commented Mar 10, 2023

Wanted to confirm if a test needs to be added here.

I was looking at the test files & I'm slightly confused as to how/where users are added in the mock database. (I'd need to add 2 users & then attempt to change the email for 1 of the users to an email already in use by the other for a functional test)

One of the relevant file is here: https://github.com/apache/trafficcontrol/blob/master/traffic_ops/testing/api/v5/user_current_test.go

@codecov
Copy link

codecov bot commented Mar 10, 2023

Codecov Report

Merging #7394 (29495cb) into master (34a601f) will not change coverage.
The diff coverage is 0.00%.

@@            Coverage Diff            @@
##             master    #7394   +/-   ##
=========================================
  Coverage     26.12%   26.12%           
  Complexity       98       98           
=========================================
  Files           648      648           
  Lines         76280    76280           
  Branches         90       90           
=========================================
  Hits          19925    19925           
  Misses        54520    54520           
  Partials       1835     1835           
Flag Coverage Δ
golib_unit 52.47% <ø> (ø)
grove_unit 4.60% <ø> (ø)
t3c_unit 5.33% <ø> (ø)
traffic_monitor_unit 20.43% <ø> (ø)
traffic_ops_integration 69.34% <ø> (ø)
traffic_ops_unit 19.70% <0.00%> (ø)
traffic_stats_unit 10.14% <ø> (ø)
unit_tests 22.55% <0.00%> (ø)
v3 57.79% <ø> (ø)
v4 79.09% <ø> (ø)
v5 78.53% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
traffic_ops/traffic_ops_golang/user/current.go 0.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ocket8888 ocket8888 added bug something isn't working as intended Traffic Ops related to Traffic Ops low impact affects only a small portion of a CDN, and cannot itself break one labels Mar 13, 2023
@ocket8888 ocket8888 self-assigned this Mar 13, 2023
Copy link
Contributor

@ocket8888 ocket8888 left a comment

Choose a reason for hiding this comment

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

Looks good, works, fixes the bug

@ocket8888 ocket8888 merged commit acc2297 into apache:master Mar 13, 2023
@tinfoil-knight tinfoil-knight deleted the fix/duplicate-email-err branch March 14, 2023 07:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug something isn't working as intended low impact affects only a small portion of a CDN, and cannot itself break one Traffic Ops related to Traffic Ops

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Setting your own email address to one that's used by another user causes Internal Server Error

2 participants