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

Fix language code validation (PP-422) #1362

Merged
merged 2 commits into from
Sep 8, 2023

Conversation

jonathangreen
Copy link
Member

@jonathangreen jonathangreen commented Sep 8, 2023

Description

The previous validation code was both checking that the language was valid and transforming the language code that gets stored in the database to the three letter version of the code.

When I updated the validation code in #1281, I missed the second part. So we were storing and using the full name of the language. This was causing the libraries lanes not to populate correctly.

Motivation and Context

Support issue PP-422.

In order to fully fix this for libraries that were created after the bug was deployed, but before the fix was deployed we would need a data migration to update the libraries settings and the lane configuration. In a discussion on slack, we decided that since this is only impacting one or two libraries those libraries can be fixed via manual intervention.

For those libraries we will need to:

  1. Re-save the library settings
  2. Regenerate the libraries lanes

How Has This Been Tested?

  • Reproduced the issue locally
  • Applied fix
  • Did manual remediation steps
  • Saw lanes properly generating

Checklist

  • I have updated the documentation accordingly.
  • All new and existing tests passed.

@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (f7f8e03) 89.91% compared to head (0ae0742) 89.92%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1362   +/-   ##
=======================================
  Coverage   89.91%   89.92%           
=======================================
  Files         211      211           
  Lines       28470    28475    +5     
  Branches     6498     6499    +1     
=======================================
+ Hits        25599    25605    +6     
  Misses       1880     1880           
+ Partials      991      990    -1     
Files Changed Coverage Δ
core/configuration/library.py 100.00% <100.00%> (+0.88%) ⬆️

☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.

Copy link
Contributor

@tdilauro tdilauro left a comment

Choose a reason for hiding this comment

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

Looks good! 🛠️ One minor comment. 🤔

@@ -638,12 +638,17 @@ def validate_language_codes(
) -> Optional[List[str]]:
"""Verify that collection languages are valid."""
if value is not None:
languages = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: You could switch to a set here to avoid checking before adding a language at l. 652.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm using a list because I want to avoid re-ordering the languages, and sets don't provide order.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking about using one of the suggestions from stack overflow for an ordered set, but in the end with this little data, using dict keys to approximate an ordered set seemed to negatively impact readability with very little gain.

@jonathangreen jonathangreen merged commit daabfad into main Sep 8, 2023
@jonathangreen jonathangreen deleted the bugfix/language-code-validation branch September 8, 2023 17:47
@tdilauro tdilauro added the bug Something isn't working label Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants