Draft: Validate email presence across codebase

[yzhoubk]

During Rails upgrade code review testing, we discovered users were missing email addresses. Investigation showed that a CalNet schema attribute had been renamed, though we were not notified when the change occurred. This ticket aims to address unexpected attribute changes from CalNet and improve our handling of schema updates.

[awilfox]

This looks great and should help with debugging AP-583 as well. r+ after tiny question.

[awilfox]

Since this value is already being logged above (line 76), does it need to be logged here as well?

[awilfox]

Ah, sorry, I didn't see the test failures. Looks like Rubocop has some suggestions, and we need to fix up the mock user objects in the other tests to have the required attributes. Still, I approve of the overall work - just need to fix those issues before merging.

[anarchivist]

agreed with @awilfox - this is looking good, but let's address the test failures before moving forward.

[yzhoubk]

@anarchivist, @awilfox

I added berkeleyEduAlternateId as a fallback attribute when retrieving the email from CalNet. I also moved the CalNet attribute name mappings into a config file so they can be shared by both the User model and CalNet-related tests, without modifying the CalNet data fixtures.

For the student ID value, since there is no berkeleyEduStuID attribute in the response, I temporarily fell back to using berkeleyEduCSID, as the fixture data and comments suggest that these two attributes contain the same value.

However, after further review, I think we should not introduce a fallback for the berkeleyEduStuID attribute. In my local CalNet response, the berkeleyEduStuID attribute is missing simply because I am not a student. Previously, the User model did not validate CalNet attributes, so we did not notice the absence of berkeleyEduStuID. - That's my guess.

If this is the case, a student might also not have an employeeNumber if he/she is not a student employee.

It seems we need a sample CalNet response for an actual student account to clarify the expected attributes. Based on that, we can decide how to properly update the current validation logic for studentID and employeeID, or we just eliminate to validate related attributes: berkeleyEduStuID and employeeNumber .

I will rebase locally to fix the conflict tomorrow.

[awilfox]

@yzhoubk I agree that we should gather a real CalNet student response, anonymise it, and then use it as part of our testing. It would be beneficial to have both a student employee and a student non-employee to see how employeeNumber behaves.

The overall direction of this MR looks good. I will review it in-depth when you are ready.

[yzhoubk]

The PR isn’t ready for review yet — I’m planning to refactor and restructure some of the code .