-
Notifications
You must be signed in to change notification settings - Fork 9
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
[Debt] Move enum localization to backend #10701
Conversation
I need to go through this more carefully but so far I really like it. Regarding the big array in GraphQLServiceProvider, I agree that it isn't great but isn't terrible either. It should be possible to fix this - Laravel itself does a lot of "if a file exists I'll use it" magic. At the same time, not terrible to have to do this manually for a bit. Regarding the fallback for missing strings - I actually prefer the fallback to print the key. I think that's a lot more useful for a user than a generic "not found" string. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10701 +/- ##
============================================
+ Coverage 38.60% 38.91% +0.31%
- Complexity 1654 1723 +69
============================================
Files 1013 1053 +40
Lines 30903 31236 +333
Branches 6439 6564 +125
============================================
+ Hits 11929 12157 +228
- Misses 18809 18912 +103
- Partials 165 167 +2
Flags with carried forward coverage won't be shown. Click here to find out more. β View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am finished with my list of enums.
I did notice this loading behaviour for some slow loading. Is this new with moving enums to the backend or is it something I hadn't noticed before when it was coming from the frontend?
Screen recording
slow_load.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think I've run out of things to try and check, I leave it to the other reviewers now π«‘
Yeah, I was trying to avoid a bunch of prop drilling here and load the needed strings only when they were needed (opening the form). This can defintley be improved but I'm not sure how π€
|
apps/web/src/pages/SearchRequests/ViewSearchRequestPage/components/UpdateSearchRequest.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work Eric π₯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a lot of ποΈ going on here, nice work. the issues i've raised have been addressed. this is not part of the acceptance criteria so i guess i approve π€·.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there! π A few small things and one big thing left.
π€ Resolves #10634
π Introduction
Moves the localization of specific enums to the backend.
π΅οΈ Details
This moves most of the strings found in localized constants to the backend using a few new tools. We have a new concept of a localized enum. This takes any PHP enum in the
App\Enums
namespace and adds a localized string to each case (assuming it exists).In order to create a localized enum, there are a few things you must do:
HasLocalization
trait on the eum (meaning defining a lang file)/lang/{en|fr}.php
matching what you defined in the enum.$localizedEnums
array in theGraphQLServiceProvider
@localizedEnum
directiveLocalized*
typeβοΈ How it works
HasLocalization trait
This trait will force you to implement the
getLangFile
method which needs to return a string that represents the file name of the lang file.It then exposes a static function
localizedString
on the enum that takes a case of the enum and will return a localized string.Lang files
These files are found in the
lang
folder and then organized again by locale. They simply contain associative arrays in a similar fashion to laravel config files. You just need to define a key that matches a lowercase version of an enum case and then the value is the string we want to render.If a key for a speciific case does not exist in the lang file, it will first attempt to fallback to English and then, if that fails, just print out the expected key (i.e
citizenship_status.other
). Ideally we could find a way to provide a fallback message like "Not found" π€Common strings
Of course, we have a few enums that share a common string like "Other". Thankfully, it is fairly simple to share these strings. We have a
common.php
file where you can store these and then access them using theLang
facade.Lang::get('common.key', [], 'locale');
Graphql
We register these enums as a custom type in graphql in the
GraphQLServiceProvider
. Since these are all identical, all you need to do is define it in the array. We then loop through them and register it. The result is the following type (whereEnunName
is the name of the enum):Localized enum directive
Even though the type is registered, in order to actually use it, you will need to apply the
@localizedEnum
directive. This takes the value of the enum from the database and then converts it into the new type. If the key in graphql does not match the attribute on the PHP model, you can supply theattribute
argument and it will behave in a similar way to the@rename
directive.Retrieving all strings for enum
Sometimes (usually in forms) we need to get all the strings of an enum for things like input options. In order to do this we have a simple query that just returns a basic version of the localized enum type. It has validation on the enum name which will fail if the enum does not exist in
App\Enums
or if it does not implement theHasLocalization
trait.Mocking
Since we no longer have the strings for all these enums in the client code, we need to mock them for storybook and jest. We now have a helper in the
@gc-digital-talent/fake-data
package calledtoLocalizedEnum
. This is basic helper that just takes the enum and converts it to the proper tpye signature making the label just the enum value replacing_
with a space and making it lower case.Helpers
Forms
We often use all cases in an enum for input options. We have a helper
enumToOptions
that converts the cases to an options array. However, this will not work with localized enums. Now, we should use the newlocalizedEnumToOptions
helper. Since we do not have strings in the client code, you will need to query them using the new query and pass them in.Others
We have some other helpers found in
@gc-digital-talent/i18n
. I'm not sure if this is the correct location but that is where they ended up. If we want to move them somewhere else, that can be done easily.Sometimes, we have the value of an enum but not the localized version. We have two ways to convert them. One will return the entire localized enum type and the other just returns the localized string in the current locale.
Also, alot of these enums need specific sorting. We have all of these sort helpers there as well as a generic sorter that just takes an array for order and the data from the API.
π Things I don't like
Maintaining the localized enum array
Having to maintain an array in the
GraphQLServiceProvider
is not great and I would like to find a way to "discover" any enum that is localized automatically. However, we do not add them that frequently so I think this could work for some time.Fallbacks
Right now if you do not provide a string it will just print out the key for the string. I would love if we could automatically fallback to either a "not found" string or just null π€
Mocking
See the previous comment about mocking for details. tldr; We no longer have real enum strings in storybook or jest tests.
π§ͺ Testing
make refresh-api
make artisan CMD="cache:clear"
pnpm run dev:fresh
π Review Plan