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

Format quota email currency values #7605

Merged
merged 3 commits into from Oct 10, 2023

Conversation

JoaoJandre
Copy link
Contributor

Description

Currently, in the Quota emails, the formatting of monetary values does not include the currency symbol defined in the quota.currency.symbol configuration. Additionally, the values are not formatted.

This PR changes the Quota emails to use the quota.currency.symbol configuration to set the currency symbol. Additionally, a new configuration has also been created, quota.currency.locale, which should be set to format the value's punctuation in the quota emails using the wanted locale, en-US or pt-BR for example.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

I tested different combinations to observe the changes in the received quota emails.

Test value = 12863.56

quota.currency.symbol quota.currency.locale How it is received in the emails
R$ pt-BR R$ 12.863,56
test null test 12863.56
null ja-JP $ 12,863.56

@codecov
Copy link

codecov bot commented Jun 7, 2023

Codecov Report

Merging #7605 (3d725f9) into main (819dd7b) will increase coverage by 0.00%.
Report is 1 commits behind head on main.
The diff coverage is 68.75%.

@@            Coverage Diff            @@
##               main    #7605   +/-   ##
=========================================
  Coverage     14.40%   14.40%           
- Complexity    10109    10110    +1     
=========================================
  Files          2748     2748           
  Lines        259390   259401   +11     
  Branches      40381    40382    +1     
=========================================
+ Hits          37353    37361    +8     
- Misses       217207   217209    +2     
- Partials       4830     4831    +1     
Files Changed Coverage Δ
.../org/apache/cloudstack/quota/QuotaServiceImpl.java 35.86% <0.00%> (ø)
...apache/cloudstack/quota/QuotaAlertManagerImpl.java 64.24% <71.42%> (+0.31%) ⬆️
.../apache/cloudstack/quota/constant/QuotaConfig.java 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

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

good functionality, small remarks

@DaanHoogland
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@DaanHoogland a [LL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [LL]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 6110

@DaanHoogland
Copy link
Contributor

@blueorangutan test

@blueorangutan
Copy link

@DaanHoogland a [SF] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@blueorangutan
Copy link

[SF] Trillian test result (tid-6708)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 21396 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7605-t6708-kvm-centos7.zip
Smoke tests completed. 86 look OK, 0 have errors, 24 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
all_test_safe_shutdown Skipped --- test_safe_shutdown.py
all_test_metrics_api Skipped --- test_metrics_api.py
all_test_outofbandmanagement Skipped --- test_outofbandmanagement.py
all_test_outofbandmanagement_nestedplugin Skipped --- test_outofbandmanagement_nestedplugin.py
all_test_routers_iptables_default_policy Skipped --- test_routers_iptables_default_policy.py
all_test_secondary_storage Skipped --- test_secondary_storage.py
all_test_service_offerings Skipped --- test_service_offerings.py
all_test_storage_policy Skipped --- test_storage_policy.py
all_test_templates Skipped --- test_templates.py
all_test_update_security_group Skipped --- test_update_security_group.py
all_test_usage_events Skipped --- test_usage_events.py
all_test_vm_autoscaling Skipped --- test_vm_autoscaling.py
all_test_vm_deployment_planner Skipped --- test_vm_deployment_planner.py
all_test_vm_life_cycle Skipped --- test_vm_life_cycle.py
all_test_vm_lifecycle_unmanage_import Skipped --- test_vm_lifecycle_unmanage_import.py
all_test_vm_snapshot_kvm Skipped --- test_vm_snapshot_kvm.py
all_test_vm_snapshots Skipped --- test_vm_snapshots.py
all_test_volumes Skipped --- test_volumes.py
all_test_vpc_ipv6 Skipped --- test_vpc_ipv6.py
all_test_vpc_redundant Skipped --- test_vpc_redundant.py
all_test_vpc_router_nics Skipped --- test_vpc_router_nics.py
all_test_vpc_vpn Skipped --- test_vpc_vpn.py
all_test_host_maintenance Skipped --- test_host_maintenance.py
all_test_hostha_kvm Skipped --- test_hostha_kvm.py

@DaanHoogland DaanHoogland added this to the 4.19.0.0 milestone Jun 22, 2023
@JoaoJandre
Copy link
Contributor Author

@DaanHoogland could we rerun the tests for this PR?

@JoaoJandre
Copy link
Contributor Author

@blueorangutan package

@blueorangutan
Copy link

@JoaoJandre a [SF] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 6722

@DaanHoogland
Copy link
Contributor

@blueorangutan test

@blueorangutan
Copy link

@DaanHoogland a [SF] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@blueorangutan
Copy link

[SF] Trillian test result (tid-7359)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 42554 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7605-t7359-kvm-centos7.zip
Smoke tests completed. 112 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
test_01_migrate_VM_and_root_volume Error 76.76 test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks Error 53.47 test_vm_life_cycle.py

@github-actions
Copy link

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

Copy link
Contributor

@GutoVeronezi GutoVeronezi left a comment

Choose a reason for hiding this comment

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

CLGTM

* <li>value.toString() if the numberFormat is null;</li>
* <li>the value formatted if both parameters are not null;</li>
*/
public static String formatBigDecimalAccordingToNumberFormat(BigDecimal value, NumberFormat numberFormat) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We could add some unit tests for this method.

@@ -238,19 +248,29 @@ public void sendQuotaAlert(DeferredQuotaEmail emailToBeSent) {
}
}

private NumberFormat getLocaleFormatIfCurrencyLocaleNotNull() {
Copy link
Contributor

Choose a reason for hiding this comment

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

We could add some unit tests for this method.

@DaanHoogland
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@DaanHoogland a [SF] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 7281

@DaanHoogland
Copy link
Contributor

@blueorangutan test

@blueorangutan
Copy link

@DaanHoogland a [SF] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@blueorangutan
Copy link

[SF] Trillian test result (tid-7894)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 44212 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7605-t7894-kvm-centos7.zip
Smoke tests completed. 112 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
test_02_upgrade_kubernetes_cluster Failure 610.23 test_kubernetes_clusters.py
test_08_upgrade_kubernetes_ha_cluster Failure 711.00 test_kubernetes_clusters.py

@DaanHoogland DaanHoogland merged commit 43aed45 into apache:main Oct 10, 2023
25 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants