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

Convert the type on the UserJob entity to be a string #23888

Merged
merged 3 commits into from
Jun 29, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Jun 28, 2022

Overview

Currently we are storing a numeric ID - however if we
permit non-core classes to register types we find numeric ids
quickly become hard to manage as uninstalling an extension could
change the id. This switches to using a string type

Before

civicrm_user_job uses type_id to track the job 'type' - but this can't be extended because an integer varies

After

civicrm_user_job now users job_type - a var char

Technical Details

Comments

@civibot
Copy link

civibot bot commented Jun 28, 2022

(Standard links)

@civibot civibot bot added the 5.51 label Jun 28, 2022
@eileenmcnaughton eileenmcnaughton force-pushed the user_import branch 4 times, most recently from c9a5979 to e2ed102 Compare June 28, 2022 11:14
Currently we are storing a numeric ID - however if we
permit non-core classes to register types we find numeric ids
quickly become hard to manage as uninstalling an extension could
change the id. This switches to using a string type
@totten
Copy link
Member

totten commented Jun 28, 2022

Scope looks fairly sensible. I'll give it an r-run.

@totten
Copy link
Member

totten commented Jun 29, 2022

r-run: DB Upgrade: I did some imports on 5.51.beta1, ran the upgrade to 5.51.beta2 (patch added), and inspected civicrm_user_job. The resulting schema matched, and the job_type's appeared to correlate to the previous type_ids. 👍

r-run: Import UI: I ran "Import Contacts", "Import Activities", and "Import Custom Data" (mostly using testdata, though I twiddled the activities.csv to set source_contact_id). The results appeared to be the same, but I think this is a new warning on the "Import Contacts" and "Import Activities":

Screenshot from 2022-06-29 00-25-48

Seems like a relevant warning?

@eileenmcnaughton
Copy link
Contributor Author

Ah - that needs to be changed to job_type:label

@eileenmcnaughton
Copy link
Contributor Author

@totten if it is just that e-notice are you OK to merge this & then I will do a follow up for it? It's a bit easier for me that way

eg When running "Import Activities", the page-title for the summary is incorrect.
@totten
Copy link
Member

totten commented Jun 29, 2022

Well, I hadn't dug into see if the warning was meaningful. So I took a look now - and, in fact, page-title was wrong when using "Import Activities". Added a commit to fix.

@eileenmcnaughton
Copy link
Contributor Author

lol yep - I knew that was it - I just didn't want to play the rebase game :-)

@totten
Copy link
Member

totten commented Jun 29, 2022

Grepped around for other references to type_id. There are some comments that can be updated like:

* Get the name of the type to be stored in civicrm_user_job.type_id.

But that has no functional impact. That can be follow-up.

Aside for the record - as eileen noted elsewhere, this field is pretty new and only becomes meaningful with 5.51. So we don't expect that contrib has any r-tech reliance on this (yet). And it's a lot easier to adjust if we can get this into 5.51 before there is more meaningful downstream reliance.

Merge on pass.

@totten
Copy link
Member

totten commented Jun 29, 2022

AFAICS, #23893 doesn't have any changes affecting Summary.php or FiveFiftyOne.php, so 🤞 you won't have to play the rebase game 🧺 🏀 ⛹️ .

@eileenmcnaughton
Copy link
Contributor Author

@totten I would have had to to update this PR without overwriting your patch

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.

2 participants