Skip to content

Draft: Validate email presence across codebase#22

Open
yzhoubk wants to merge 1 commit intomainfrom
AP-580
Open

Draft: Validate email presence across codebase#22
yzhoubk wants to merge 1 commit intomainfrom
AP-580

Conversation

@yzhoubk
Copy link
Contributor

@yzhoubk yzhoubk commented Feb 13, 2026

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.

Copy link
Member

@awilfox awilfox left a comment

Choose a reason for hiding this comment

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

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

# list all keys except duo keys
def list_auth_extra_keys(auth_extra)
keys = auth_extra.keys.reject { |k| k.start_with?('duo') }.sort
Rails.logger.info("CalNet auth_extra keys: #{keys.join(', ')}")
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@awilfox awilfox left a comment

Choose a reason for hiding this comment

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

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.

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.

2 participants