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

[Import] Rename dedupe rule id field from dedupe to dedupe_rule_id field #23263

Merged
merged 3 commits into from
Apr 21, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

[Import] Rename dedupe rule id field from dedupe to dedupe_rule_id field

Builds on other open prs - if they are merged this can be rebased down....

Before

The field name for 'dedupe_rule_id' is 'dedupe;

After

Renamed to 'dedupe_rule_id' in the Contact flow - It is still dedupe internally as those places will 'age out'

Technical Details

A universe search showed that @mlutfy's advanced import extension refers to the form value (which is unchanged in this PR) - however, I think it's probably best that once this piece of work is all merged I work with @mlutfy to update advanced import as there are a bunch of things it does where core code could be leveraged - e.g the 'dedupe' value is saved to civicrm_advimport - but that will be redundant as submitted_values are being saved to the new civicrm_user_job once the open PRs are merged.so advimport can access from there.

Comments

Builds on #23245 and #23260 - can be rebased when that is merged (or that will close when this is is merged)

@civibot
Copy link

civibot bot commented Apr 20, 2022

(Standard links)

@colemanw
Copy link
Member

This PR is well-documented and includes test coverage. I think we should just merge it as it's part of ongoing refactoring and we want it all within the same release cycle.

@colemanw colemanw added the merge ready PR will be merged after a few days if there are no objections label Apr 21, 2022
@eileenmcnaughton
Copy link
Contributor Author

I will be continuing to follow up on this & extend test cover - so getting this merged opens that up & the r-run will be ongoing over the top of this - failure seems still related to general fails but try again jenkins - test this please

@eileenmcnaughton
Copy link
Contributor Author

Between all the test mahem this got a fail into it - which is in the PR it builds on - based on the merge-ready on this PR/ discussion I merged the PR that is passing & am rebasing & fixing the middle PR to get this back to passing - note the test failures relate to the tests rather than issues from this change

@eileenmcnaughton eileenmcnaughton force-pushed the import_wippier branch 2 times, most recently from 4df19b3 to 91e41fc Compare April 21, 2022 09:19
…import

This provides a bit of sanity to the MapTable.tpl which is included from
MapField and Preview - the focus being around getting
rid of all the section tags which rely on Count being assigned
and getting back to for-eaching the relevant arrays
It is still dedupe internally as those places will 'age out'
@eileenmcnaughton
Copy link
Contributor Author

test this please

@colemanw colemanw merged commit 5bab835 into civicrm:master Apr 21, 2022
@colemanw colemanw deleted the import_wippier branch April 21, 2022 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants