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

Render shipping cost nanos in emailservice #567

Merged
merged 1 commit into from
Nov 10, 2022

Conversation

jjatria
Copy link
Contributor

@jjatria jjatria commented Nov 9, 2022

Changes

The template used for the confirmation email in the emailservice did not render the nanos of the shipping cost. This commit adds them, so the rendering of shipping cost and item cost (lower in the template) is equivalent.

This is a minor fix.

@jjatria jjatria requested a review from a team November 9, 2022 17:05
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 9, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: jjatria / name: José Joaquín Atria (bd70d8b)

The template used for the confirmation email in the emailservice did not
render the nanos of the shipping cost. This commit adds them, so the
rendering of shipping cost and item cost (lower in the template) is
equivalent.
@julianocosta89
Copy link
Member

Hello @jjatria, thanks for the contribution.
Is this change actually visible somewhere?
I've rebuilt the service locally, but I couldn't find any difference from the main branch and this PR.

@jjatria
Copy link
Contributor Author

jjatria commented Nov 10, 2022

I don't think it is: the email is being sent through a test transport which as far as I understand does not actually send the email anywhere, it simply stores it for later inspection... which I don't think we do.

I spotted the bug in the code rather than in the output, and if you look back at bc0cd67 to when the original Python version was ported to Ruby, you can see that the original was actually using the nanos in this section. So it just looks like that bit got lost in the port.

Looking at that commit now, it looks like the Ruby version is much less careful about how those numbers are printed, but I chose to keep the diff minimal and simply keep the two uses consistent.

@julianocosta89
Copy link
Member

Ah great!
It makes sense.
I've asked because we do have an opened issue in the frontend (#558), where the nanos are also ignored.

@ahayworth
Copy link
Contributor

@jjatria Thanks! I think this was just an oversight from when I initially added the ruby bits (but never noticed because as you pointed out: we don't really send any emails 😆 )

@jjatria
Copy link
Contributor Author

jjatria commented Nov 10, 2022

@ahayworth Makes sense! I only noticed this when I was porting this service to Perl so I could play around locally, and even then only when I noticed I was stringifying a hash instead of a number

@cartersocha cartersocha merged commit 0c1a627 into open-telemetry:main Nov 10, 2022
@jjatria jjatria deleted the ruby-nanos-render branch November 10, 2022 16:14
jmichalak9 pushed a commit to jmichalak9/opentelemetry-demo that referenced this pull request Mar 22, 2024
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.

4 participants