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

Use constants instead of translations for StoreCreditType names #2157

Merged
merged 4 commits into from
Aug 21, 2017
Merged

Use constants instead of translations for StoreCreditType names #2157

merged 4 commits into from
Aug 21, 2017

Conversation

swcraig
Copy link
Contributor

@swcraig swcraig commented Aug 16, 2017

There doesn't seem to be a great reason for StoreCreditType::DEFAULT_TYPE_NAME to be defined with a translation. The translation isn't being used to construct error messages or sent to the frontend at all.

This PR defines two constants in StoreCreditType, StoreCreditType::EXPIRING and StoreCreditType::NON_EXPIRING, and uses them in the appropriate places.

There's no great reason for `StoreCreditType::DEFAULT_TYPE_NAME` to be
defined with a translation. The translation isn't being used to
construct error messages or sent to the frontend at all.
It makes more sense to use the constants defined in `StoreCreditType`.
The `StoreCreditType::DEFAULT_TYPE_NAME` is the expiring type. Instead
of having a string as the `secondary_credit_type#name` use the constant
defined in `StoreCreditType`.
Copy link
Contributor

@cbrunsdon cbrunsdon left a comment

Choose a reason for hiding this comment

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

Big thumbs up for me, doesn't take store credits as far as they need to go (full expiry support), but its an improvement of loading up consts from translations.

Copy link
Contributor

@mamhoff mamhoff left a comment

Choose a reason for hiding this comment

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

Yeah, nice step in the right direction.

@cbrunsdon cbrunsdon merged commit b55c047 into solidusio:master Aug 21, 2017
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