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.

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

Improve the assign input on Alerts View. #8002

Merged

Conversation

florinbilt
Copy link
Contributor

No description provided.

@florinbilt florinbilt force-pushed the distinguish-retriggers-in-Graph branch from 7d332f7 to abc058f Compare March 29, 2024 15:14
@florinbilt florinbilt force-pushed the distinguish-retriggers-in-Graph branch from abc058f to 9f8548f Compare March 29, 2024 15:17
@codecov-commenter
Copy link

codecov-commenter commented Mar 29, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 77.07%. Comparing base (931e8bb) to head (9f8548f).

Files Patch % Lines
ui/perfherder/alerts/Assignee.jsx 85.71% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #8002   +/-   ##
=======================================
  Coverage   77.07%   77.07%           
=======================================
  Files         545      545           
  Lines       26962    26967    +5     
  Branches     3383     3389    +6     
=======================================
+ Hits        20782    20786    +4     
- Misses       6013     6014    +1     
  Partials      167      167           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

if (event.key === 'Enter') {
event.preventDefault();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you quickly explain why did you move it?

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 moved this line (event.preventDefault();) inside the if statement because it was also preventing the onchange event on the input field, and that's why the input field was not editable. However, we still need that line because inside the if statement, 'const { failureStatus } = await updateAssignee(newAssigneeUsername);' is called. This line at the end of the function will try to redirect you to '/alerts' regardless of whether the assignment was successful or not.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the explanation

@@ -33,7 +33,9 @@ export default class Assignee extends React.Component {
inEditMode: true,
// input prefills with this field, so
// we must have it prepared
newAssigneeUsername: assigneeUsername,
newAssigneeUsername: assigneeUsername
? assigneeUsername.split('/')[1]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you need to do that because on line 70 you already set the username correctly when using the "Take" functionality. For the case where the user fills in the information themselves this is not needed because they are required to only fill in the email. Let me know what you think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line is necessary when we want to reassign an alert. If that line doesn't exist, when enter in edit mode on an already assigned alert, we will see 'mozilla-ldap/[email protected]' instead of '[email protected]'

@beatrice-acasandrei beatrice-acasandrei self-requested a review April 16, 2024 20:18
@beatrice-acasandrei beatrice-acasandrei merged commit 0291f4e into mozilla:master Apr 16, 2024
5 checks passed
Archaeopteryx pushed a commit to Archaeopteryx/treeherder that referenced this pull request Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants