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

[Feature:System] Don't set given names to empty string #37

Merged
merged 5 commits into from
Jan 24, 2025

Conversation

lavalleeale
Copy link
Contributor

Currently this script is setting all users preferred names to an empty string upon running, which causes many database queries to become substantially longer to account for both values indicating no desired preferred name.

@lavalleeale lavalleeale changed the title [Feature:System] Prevent setting given names to empty string [Feature:System] Don't setting given names to empty string Jan 21, 2025
@lavalleeale lavalleeale changed the title [Feature:System] Don't setting given names to empty string [Feature:System] Don't set given names to empty string Jan 21, 2025
@lavalleeale lavalleeale requested a review from pbailie January 21, 2025 21:14
Copy link
Contributor

@pbailie pbailie left a comment

Choose a reason for hiding this comment

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

In my testing, the very first run for an academic term will insert a mixture of null and empty strings for user_preferred_givenname. Subsequent runs will upsert the intended results, but as there is an edge case of failure, I am requesting changes.

@lavalleeale
Copy link
Contributor Author

Thank you very much, I placed the NULLIF in the wrong place so it was only working on conflict but now it will work on the first run too.

@lavalleeale lavalleeale requested a review from pbailie January 22, 2025 22:34
Copy link
Contributor

@pbailie pbailie left a comment

Choose a reason for hiding this comment

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

ERROR:  syntax error at or near "("
LINE 6:  NULLIF(user_preferred_givenname, ''),
               ^ 

SQL state: 42601
Character: 91

NULLIF is a function and cannot be used in that context.

@lavalleeale
Copy link
Contributor Author

Sorry, now it really should be in the right place. Also, is there an example csv that can be used for local testing?

@lavalleeale lavalleeale requested a review from pbailie January 23, 2025 18:07
Copy link
Contributor

@pbailie pbailie left a comment

Choose a reason for hiding this comment

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

This change is showing OK in pgAdmin.

This line is intended to "write protect" user_preferred_givenname after a value has been inserted. That is, the CSV can change user_preferred_givenname only when it is blank. Please update this line to reflect that a blank value is now null.

AND COALESCE(users.user_preferred_givenname, '')=''

I will do a more thorough test when this requested change is done.

@pbailie
Copy link
Contributor

pbailie commented Jan 23, 2025

Also, is there an example csv that can be used for local testing?

I built my own testing tools, and they are not for public use.

pgAdmin is very useful for testing queries and transactions, and it is free and open source.
https://www.pgadmin.org/

@lavalleeale
Copy link
Contributor Author

I can make that change if needed, but that means that if there is lag between this PR being deployed and the migration being run on main there will be a period where preferred names will not automatically be fetched for existing students. I can make a second PR once the migration is run, but for now main expects both null and empty strings to mean no preferred name. If you would still like the changes to be made here first I can also do that however, just let me know how you would like to proceed.

@pbailie
Copy link
Contributor

pbailie commented Jan 23, 2025

I am the guy who manages the student auto feed in production. The auto feed system is entirely separate from Submitty. No migration is needed for the auto feed.

As for the requested changes... once they are finished, I can upload the changes to production ASAP or delay or schedule for any changes being planned for Submitty itself. The code patch process for (auto feed) production takes almost no time at all.

Updating the write-protect line should be my last change request, assuming that things work well when I do more thorough testing.

Copy link
Contributor

@pbailie pbailie left a comment

Choose a reason for hiding this comment

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

Seems to be working well. Thank you for helping.

@pbailie
Copy link
Contributor

pbailie commented Jan 24, 2025

Should this change be uploaded to production ASAP, or is there a wait period for work on Submitty?

@bmcutler bmcutler merged commit 31710c5 into main Jan 24, 2025
1 check passed
@bmcutler bmcutler deleted the no-empty-names branch January 24, 2025 21:35
bmcutler pushed a commit to Submitty/Submitty that referenced this pull request Feb 5, 2025
### Please check if the PR fulfills these requirements:

* [ ] Tests for the changes have been added/updated (if possible)
* [ ] Documentation has been updated/added if relevant
* [ ] Screenshots are attached to Github PR if visual/UI changes were
made

### What is the current behavior?
<!-- List issue if it fixes/closes/implements one using the "Fixes
#<number>" or "Closes #<number>" syntax -->
Preferred names can both be empty strings or null to indicate the user
does not have a desired preferred name

### What is the new behavior?
All existing empty preferred names will be set to null and constraints
are added to the database to ensure that empty preferred names cannot be
added to the database in the future.

### Other information?
<!-- Is this a breaking change? -->
<!-- How did you test -->
This is part of a series of changes starting with
Submitty/SysadminTools#37 and will be followed
up by more PRs to remove the handling of empty strings in the database
to be able to further simplify the code to not have to handle 2 cases
that both mean the user does not have a preferred name.
lllinging pushed a commit to lllinging/Submitty that referenced this pull request Feb 6, 2025
### Please check if the PR fulfills these requirements:

* [ ] Tests for the changes have been added/updated (if possible)
* [ ] Documentation has been updated/added if relevant
* [ ] Screenshots are attached to Github PR if visual/UI changes were
made

### What is the current behavior?
<!-- List issue if it fixes/closes/implements one using the "Fixes
#<number>" or "Closes #<number>" syntax -->
Preferred names can both be empty strings or null to indicate the user
does not have a desired preferred name

### What is the new behavior?
All existing empty preferred names will be set to null and constraints
are added to the database to ensure that empty preferred names cannot be
added to the database in the future.

### Other information?
<!-- Is this a breaking change? -->
<!-- How did you test -->
This is part of a series of changes starting with
Submitty/SysadminTools#37 and will be followed
up by more PRs to remove the handling of empty strings in the database
to be able to further simplify the code to not have to handle 2 cases
that both mean the user does not have a preferred name.
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.

3 participants