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

Send abo magazin confirmation email (#1129) #1211

Merged
merged 2 commits into from
Nov 6, 2024

Conversation

amaierhofer
Copy link
Contributor

No description provided.

@amaierhofer amaierhofer force-pushed the feature/1129-abo-magazin-confirmation-mailer branch from 7123bbf to 846353e Compare November 4, 2024 16:26
@amaierhofer amaierhofer linked an issue Nov 4, 2024 that may be closed by this pull request
5 tasks
Copy link
Contributor

@codez codez left a comment

Choose a reason for hiding this comment

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

LGTM

end

def placeholder_language
Event::Course.language_labels.stringify_keys[@person.language]
Copy link
Contributor

Choose a reason for hiding this comment

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

Das sollte wohl nicht vom Kurs kommen, eher von der Person. (Auch wenn die Werte jetzt gleich sein mögen, sind die unabhängig voneinander).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ja, das macht viel mehr Sinn, hab ich angepasst.

magazin: [
AboCost.new(amount: 60, country: :switzerland),
AboCost.new(amount: 76, country: :international)
],
Copy link
Contributor

Choose a reason for hiding this comment

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

Anstelle eines Arrays wäre ein Hash mit dem country als Key effizienter. Dann braucht es im aufrufenden Code kein find, es kann direkt mit [country] auf den richtigen Wert zugegriffen werden.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ist nicht ganz so easy umzusetzen (locales, fallback für die verschiedenen internationalen Länder, partial wird auch von einem Abo ohne country verwendet), ich lasse das vorerst so

@amaierhofer amaierhofer force-pushed the feature/1129-abo-magazin-confirmation-mailer branch from a1f49d5 to a73d345 Compare November 6, 2024 17:12
@amaierhofer amaierhofer enabled auto-merge (squash) November 6, 2024 17:12
@amaierhofer amaierhofer merged commit 19ee640 into master Nov 6, 2024
6 checks passed
@amaierhofer amaierhofer deleted the feature/1129-abo-magazin-confirmation-mailer branch November 6, 2024 17:21
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.

MAILS: Bestellbestätigung Magazin
2 participants