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

Added autocomplete input field for assigning bulk default user #3250

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pandafy
Copy link
Contributor

@pandafy pandafy commented Jun 18, 2023

Closes #2510

@pandafy pandafy force-pushed the default-tester-autocomplete branch from 6b9c235 to 5dfe4be Compare June 18, 2023 15:39
@pandafy
Copy link
Contributor Author

pandafy commented Jun 18, 2023

Testplan.mp4

@pandafy
Copy link
Contributor Author

pandafy commented Jun 24, 2023

@atodorov did you get time to look into this PoC?

@atodorov
Copy link
Member

@atodorov did you get time to look into this PoC?

Sorry, not yet. I will try to look into it by the end of this week.

Copy link
Member

@atodorov atodorov left a comment

Choose a reason for hiding this comment

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

Functionally looks good but needs changes in order for us to be able to use this widget elsewhere and for the user experience to be better.

tcms/testplans/templates/testplans/get.html Outdated Show resolved Hide resolved
tcms/static/js/index.js Outdated Show resolved Hide resolved
return element.username
},
source: function (query, processSync, processAsync) {
jsonRPC('User.filter', { username__icontains: query }, function (data) {
Copy link
Member

Choose a reason for hiding this comment

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

Note: User.filter depends on having the auth.view_user permission which currently silently prints errors in the console if the permission is not granted.

I'm not sure if/how we want to inform users that their auto-complete searches may not work because they are missing the permission.

Maybe we should default to the old implementation if permission isn't granted and allow auto-complete only when it is.

@pandafy
Copy link
Contributor Author

pandafy commented Jul 10, 2023

Thank you for your feedback @atodorov! I will make those changes ASAP.

@pandafy pandafy requested a review from atodorov July 24, 2023 09:16
@pandafy pandafy marked this pull request as ready for review August 7, 2023 11:29
Copy link
Member

@atodorov atodorov left a comment

Choose a reason for hiding this comment

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

Looks better, some changes needed to address issues with the current implementation.

<button type="button" class="close" data-dismiss="modal" aria-hidden="true" aria-label="Close">
<span class="pficon pficon-close"></span>
</button>
<h4 class="modal-title" id="default-tester-modal-title">{% trans "Default Tester" %}</h4>
Copy link
Member

Choose a reason for hiding this comment

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

Default Tester -> Default tester to minimize variations in translation strings

Suggested change
<h4 class="modal-title" id="default-tester-modal-title">{% trans "Default Tester" %}</h4>
<h4 class="modal-title" id="default-tester-modal-title">{% trans "Default tester" %}</h4>

@@ -22,6 +22,7 @@ <h1 class="col-md-12" style="margin-top: 0">
data-perm-add-testcase="{{ perms.testcases.add_testcase }}"
data-perm-add-comment="{{ perms.django_comments.add_comment }}"
data-perm-delete-comment="{{ perms.django_comments.delete_comment }}"
data-perm-view-user="{{ perms.auth.view_user }}"
data-trans-username-email-prompt="{% trans 'Enter username, email or user ID:' %}"
Copy link
Member

Choose a reason for hiding this comment

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

Unused, needs to be removed:

Suggested change
data-trans-username-email-prompt="{% trans 'Enter username, email or user ID:' %}"

})
}
})
}
Copy link
Member

Choose a reason for hiding this comment

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

A few issues with how this function/modal behaves:

Issue #1

  1. Use it once to assign a default tester. I chose "alice" then
  2. Mark some test cases and trigger the default tester modal again -> the text "alice" is still present inside the input field, it should be empty field instead

Issue #2

Every time the modal opens the typeahead handler gets initialized again. Using the modal 5-6 times without reloading the page leads to a chain of User.filter RPC calls instead of having only 1 call. This visibly slows down the page. This is similar to #3281 .

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.

RFE: Show drop down w/ auto-complete for user selection in Test Run Page
2 participants