Skip to content

Conversation

@BryanMLima
Copy link
Contributor

@BryanMLima BryanMLima commented Dec 5, 2024

Description

This PR addresses an issue when trying to assign an instance to another account/project when using ACS in another language besides English. To accomplish this fix, the OwnershipSelection component was standardized to use logic independent of the language used by the user.

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)
  • build/CI
  • test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

How Has This Been Tested?

Before this patch, the message The new account is the same as the old account. was shown when trying to assign the VM to another account/project.

With this patch, assigning an instance works as intended in different languages, as expected.

I also test the creation of VMs, Isolated networks, L2 networks, shared FS and volumes; all resources were created accordingly to the owner specified.

@BryanMLima BryanMLima added this to the 4.19.2 milestone Dec 5, 2024
@BryanMLima
Copy link
Contributor Author

@blueorangutan ui

@blueorangutan
Copy link

@BryanMLima a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

@BryanMLima BryanMLima changed the title Fix assign instance translation issue UI: Fix assign instance translation issue Dec 5, 2024
@codecov
Copy link

codecov bot commented Dec 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 16.03%. Comparing base (e57a82a) to head (7e062ea).
Report is 17 commits behind head on 4.20.

Additional details and impacted files
@@            Coverage Diff            @@
##               4.20   #10052   +/-   ##
=========================================
  Coverage     16.03%   16.03%           
  Complexity    12814    12814           
=========================================
  Files          5637     5637           
  Lines        493506   493507    +1     
  Branches      59831    59831           
=========================================
+ Hits          79129    79130    +1     
+ Misses       405601   405600    -1     
- Partials       8776     8777    +1     
Flag Coverage Δ
uitests 4.02% <ø> (ø)
unittests 16.87% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@blueorangutan
Copy link

UI build: ✔️
Live QA URL: https://qa.cloudstack.cloud/simulator/pr/10052 (QA-JID-495)

Copy link
Member

@bernardodemarco bernardodemarco left a comment

Choose a reason for hiding this comment

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

LGTM, tested in the QA environment

@blueorangutan
Copy link

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

@DaanHoogland
Copy link
Contributor

@BryanMLima this works in Dutch, so far for the good news. Now I have my doubts about the implementation. As the account type is a logic element of the system I would expect it to be the symbol 'Account' or the symbol 'Project' and not the english or other language words. I would expect the selectedAccountType to be compared to the symbols. What is the reason you choose for this implementation?

@BryanMLima
Copy link
Contributor Author

@BryanMLima this works in Dutch, so far for the good news. Now I have my doubts about the implementation. As the account type is a logic element of the system I would expect it to be the symbol 'Account' or the symbol 'Project' and not the english or other language words. I would expect the selectedAccountType to be compared to the symbols. What is the reason you choose for this implementation?

@DaanHoogland, I agree with you, we should not use translation of a label to the logic of the code. I made some changes to the components that are using the OwnershipSelection.

With these changes, I tested the creating VMs, Isolated networks, L2 networks, shared FS and volumes; all resources were created accordingly to the owner specified.

@BryanMLima
Copy link
Contributor Author

@blueorangutan ui

@blueorangutan
Copy link

@BryanMLima a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

@BryanMLima BryanMLima changed the title UI: Fix assign instance translation issue UI: Standardize OwnershipSelection to use logic independent of language Dec 9, 2024
@blueorangutan
Copy link

UI build: ✔️
Live QA URL: https://qa.cloudstack.cloud/simulator/pr/10052 (QA-JID-498)

Copy link
Contributor

@shwstppr shwstppr left a comment

Choose a reason for hiding this comment

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

code lgtm

@DaanHoogland DaanHoogland merged commit 32af4a2 into apache:4.20 Dec 27, 2024
26 checks passed
DaanHoogland added a commit that referenced this pull request Dec 30, 2024
* 4.20:
  VR: fix site-2-site VPN if split connections is enabled (#10067)
  UI: fix cannot open 'Edit tags' modal for static routes (#10065)
  Update ownership selection component to be language independent (#10052)
  Support to enable/disable VM High Availability manager and related alerts (#10118)
dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Jan 10, 2025
@Pearl1594 Pearl1594 moved this to Done in ACS 4.20.1 Mar 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants