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

fix user search problem happening after user deletion #36431

Merged
merged 1 commit into from
Nov 21, 2019
Merged

Conversation

karakayasemi
Copy link
Contributor

@karakayasemi karakayasemi commented Nov 17, 2019

Description

In the web-UI user management page, When user make a search, if the search result has only one entry and the listed user is deleted from the search result page, the search does not work anymore until refreshing the page.

Reason for the problem, Js search function uses a hidden column to clone new rows from it. In our scenario, after user deletion, this row also being deleted. As a result, search functionality can not add a new row to the user list.

Related Issue

Motivation and Context

Fixing bugs.

How Has This Been Tested?

Manually with the following steps:

  • Open Menu Users
  • Searching for a user (search result should be match with only one entry)
  • Delete a user
  • Make a new search for another user did not work, no result for the search.

This problem now fixed.

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

@karakayasemi karakayasemi self-assigned this Nov 17, 2019
@karakayasemi karakayasemi added this to the development milestone Nov 17, 2019
@codecov
Copy link

codecov bot commented Nov 18, 2019

Codecov Report

Merging #36431 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #36431   +/-   ##
=========================================
  Coverage     65.16%   65.16%           
  Complexity    19802    19802           
=========================================
  Files          1271     1271           
  Lines         74760    74760           
  Branches       1309     1309           
=========================================
  Hits          48719    48719           
  Misses        25655    25655           
  Partials        386      386
Flag Coverage Δ Complexity Δ
#javascript 54% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 66.39% <ø> (ø) 19802 <ø> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 096b4a8...676f993. Read the comment docs.

@jvillafanez
Copy link
Member

I'm worried about the deleted comment... could you check / investigate it?

@karakayasemi
Copy link
Contributor Author

@jvillafanez I agree about your concern too, I spent lots of time to debug and understand the code. But I could not find the purpose of removing <'tr:first'>. As line 180 says, one row should always be left to keep working the functionality of adding. The source of the problem of the related issue is also this remove line. Because when we delete the last row, new search results can not add new items.

I will investigate if this issue is a regression. It may help.

@jvillafanez
Copy link
Member

Just trying to give some ideas:

  • Add + remove + add + remove + add?
  • Small / mobile screens

You could also try with "git blame" to know when the comment was added.

@karakayasemi
Copy link
Contributor Author

@jvillafanez I investigated the code with more details. The deleted comment belongs to the following commit 478393e which looks like merged 6 years ago with the re-write of the user management page. Also, I tried the issue with older oc versions, the issue is not a regression, probably its scenario is reproducible from the beginning.

I made several tests with consecutive adding-removing-searching operations on the PR's branch and could not see any unexpected behavior. In terms of code, if there is a hidden <tr> element for cloning new rows, I see no reason for removing it in add new row method. 🤷‍♂

@jvillafanez
Copy link
Member

One last thing: double-check the generated html code make sense, without any wrong element added (you can use the browser's developer tools)

@karakayasemi
Copy link
Contributor Author

The HTML code looks fine. Also, no error log in the browser console. @enbrnz before merging this one, I would like to get your approval that the problem has been solved and that there are no visible problems.
I will also add a changelog item.

@enbrnz
Copy link
Contributor

enbrnz commented Nov 21, 2019

Hi, sorry for the delay. So I created a patch from this PR and applied it to a docker-compose setup. However it doesn't seem to have fixed the issue, as far as I can tell the behavior hasn't changed.
Sorry, my bad the browser cache got me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants