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 disambiguate all incorrectly affectly records #4777

Merged
merged 4 commits into from
Apr 26, 2024

Conversation

realVinayak
Copy link
Collaborator

@realVinayak realVinayak commented Apr 11, 2024

Fixes # My issue when doing conversion work

The problem in the previous code is you could have something like

image

Here, test1 has duplicates. So, if you click on the test1 on the top row and click disambiguate and perform "Apply All", it incorrectly chose rows just based on the key. Then, it applied the disambiguation to the same mapping path as the one currently active. So, it will apply a disambiguation on the second row with the mapping path of the first one, which is incorrect as test2 will be incorrectly disambiguated as test1.

This leads to backend always choosing test1 for test2. Seems like backend should have logic for not looking at disambiguation when a single match. I don't know use cases for multiple disambiguations for the same record in a single row -- I just tried making the code as semantically same as before. Could get rid of that to just do on all disambiguations in a single row.

I'll defer this PR to 7.9.5 when workbench is in react but opening it right now because this was annoying bug for conversion work. In my case, it led to uniqueness rule exception (a very confusing one) because everything seemed fine on the surface.

Checklist

  • Self-review the PR after opening it to make sure the changes look good
    and self-explanatory (or properly documented)
  • Add automated tests
  • Add relevant issue to release milestone

Testing instructions

@realVinayak realVinayak added this to the 7.9.5 milestone Apr 11, 2024
Copy link
Member

@maxpatiiuk maxpatiiuk left a comment

Choose a reason for hiding this comment

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

This looks good, but pinging @sharadsw as he is doing a major workbench refactor this now in #4637

any changes we make to the workbench source code in the meanwhile may cause merge conflicts, and would have to be incorporated into the new refactored code

@grantfitzsimmons
Copy link
Member

Seems like an incredibly urgent issue based on the description. @CarolineDenis What do you think?

@sharadsw
Copy link
Contributor

sharadsw commented Apr 23, 2024

This will cause a merge conflict with #4637 no matter what order we merge both PRs in. I think since this only changes one file and it's urgent, we can go ahead and merge this first. Shouldn't be difficult to resolve the conflict when it happens

@CarolineDenis CarolineDenis modified the milestones: 7.9.5, 7.9.4 Apr 23, 2024
@grantfitzsimmons grantfitzsimmons self-assigned this Apr 23, 2024
@realVinayak
Copy link
Collaborator Author

wouldn't be suprised if there is bad data out there bc of this. we caught it now just bc of business rules (other cases where no business rules will not have been caught)

@grantfitzsimmons
Copy link
Member

@CarolineDenis failing tests 😢

@CarolineDenis CarolineDenis merged commit 0cfa2ec into production Apr 26, 2024
9 checks passed
@CarolineDenis CarolineDenis deleted the wb-disambiguate-all-fix branch April 26, 2024 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants