Skip to content

Conversation

@sharonpatta
Copy link
Contributor

Gordon ID types have been changed from string to int across the api to prepare for standardizing the id types in the database. Reference: #727

@sharonpatta
Copy link
Contributor Author

There are a lot of file changes, but I wanted to check with you specifically about the changes to ProfileServices, StateYourBusiness, and CCTContextProcedures/ICCTContextProcedures to make sure what I did there was correct.

Copy link
Contributor

@EjPlatzer EjPlatzer left a comment

Choose a reason for hiding this comment

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

@sharonpatta for the changes in the Models/CCT folder, did you change those manually? Or were those changes automated by Entity Framework Core?

I ask because all of those files are auto-generated, and we re-generate them from the database definition regularly.

@bennettforkner
Copy link
Contributor

@sharonpatta for the changes in the Models/CCT folder, did you change those manually? Or were those changes automated by Entity Framework Core?

I ask because all of those files are auto-generated, and we re-generate them from the database definition regularly.

@EjPlatzer I advised @sharonpatta to change them manually if it was an easy process because there is otherwise no great way to get the type checks working for the rest of the code. When we actually update the database and are ready to merge this, we will have to refresh the database models and do a final pass-through to make sure it all works.

@EjPlatzer
Copy link
Contributor

@EjPlatzer I advised @sharonpatta to change them manually if it was an easy process because there is otherwise no great way to get the type checks working for the rest of the code. When we actually update the database and are ready to merge this, we will have to refresh the database models and do a final pass-through to make sure it all works.

Oh ok, I see. When we first discussed, I believe the plan was to make the database changes temporarily, just for long enough to regenerate the models, and then revert them until everything is tested and we're ready to merge it. I think that would be the cleaner way to do this, but I suppose at this point there's not a big difference either way.

@bennettforkner bennettforkner marked this pull request as draft May 24, 2023 13:34
Copy link
Contributor

@bennettforkner bennettforkner left a comment

Choose a reason for hiding this comment

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

I am just reviewing this PR so that Teams does not notify me every day that it is waiting for my review.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants