Generally this makes sense for me, I think it would be nice if it was eventually a separate setting in your usersettings (in case companies have differing committer / sourcegraph primary email setups) instead of magically selecting some email address in your user account, but that seems alright the way it is.

Also, I think you found all the right spots here. I would move it out of the for loop in the workspace creator though.

re: is this something that should be in our store:
I don't think it should, it's not touching our tables and it also contains logic beyond simple fetching / query building.
BUT: if we cant work it out otherwise, I would not cry out too loud.
I think the worker store should actually not live in there but in the worker package instead, it contains a bunch of logic and the word store doesn't really map to the same thing here. If we did that, it might be easier to move it out of the store!

Nothing more additional I'd add to that except to back up that this fallback behavior seems reasonable to me. Let's make sure we document it in the same places as we document the default author/email behavior for src-cli, though. I agree allowing users to configure this with a setting eventually seems useful as well, but I wouldn't rush to implement that as I expect that the majority of the time, a user's Sourcegraph email would be the same as they'd want to use for committing code to their company's repos.

Also, I think you found all the right spots here. I would move it out of the for loop in the workspace creator though.

@eseliger @LawnGnome Is this the only part that still needs work here? Along with updating docs?

re: is this something that should be in our store:
I don't think it should, it's not touching our tables and it also contains logic beyond simple fetching / query building.
BUT: if we cant work it out otherwise, I would not cry out too loud.
I think the worker store should actually not live in there but in the worker package instead, it contains a bunch of logic and the word store doesn't really map to the same thing here. If we did that, it might be easier to move it out of the store!

Would be great if we could investigate this as well!

@eseliger @LawnGnome Is this the only part that still needs work here? Along with updating docs?

In no particular order, I think this needs:

• Move that GetChangesetAuthorForUser call out of the for loop.
• Move GetChangesetAuthorForUser out of the store package. (I'm not 100% sure where the best place for it is, but maybe a new subpackage of store for now just to avoid import complications while we figure out what to do with the worker store stuff.)
  • This will imply passing the store in as an argument, rather than the function taking the Store as a receiver.
• Implement basic unit tests for GetChangesetAuthorForUser. (Nothing complicated; basically just covering the branches in the function — user ID doesn't exist, user exists but doesn't have an e-mail, user exists and has an e-mail but doesn't have a display name, user exists and is "normal".)
• Check if there are test cases that should be added for batchSpecWorkspaceCreator.process or batchSpecWorkspaceExecutionWorkerStore.MarkComplete. (My guess is no, but you can use your best judgement there.)
• A changelog entry.

I also think we should investigate moving the worker store out of enterprise/internal/batches/store, but I wouldn't do that in this PR. Sounds like a good sustaining ticket to me!

@LawnGnome

• Move GetChangesetAuthorForUser out of the store package. (I'm not 100% sure where the best place for it is, but maybe a new subpackage of store for now just to avoid import complications while we figure out what to do with the worker store stuff.)

what do you think the subpackage should be called? And should anything else be apart of that package for now? or just this one method?

what do you think the subpackage should be called? And should anything else be apart of that package for now? or just this one method?

Let's call it enterprise/internal/batches/store/author for now, unless you or someone else has a better idea.

And yep, I think it'll literally just be that one function for now. 🤷 Good thing storage is cheap and directories are small!

• Move GetChangesetAuthorForUser out of the store package. (I'm not 100% sure where the best place for it is, but maybe a new subpackage of store for now just to avoid import complications

hey @LawnGnome @eseliger in working on this i actually did encounter import implications leaving me unable to import store.Store type into author.go because I also need to import author.go into the store. I pushed the changes so you can see where I've left off. I did try to replicate the store in author.go but was not able to access the methods, like database, on the store so im not exactly sure where to go with this

Codenotify: Notifying subscribers in CODENOTIFY files for diff 5dec121...fb485e0.

Oop, well I guess it's a good thing we did that, it looks like the user actually isn't quite getting created correctly. 😬

I think you'll just need to give the database.NewUser an EmailIsVerified: true or a dummy EmailVerificationCode.