ui: fix form data double fetch/reset form data by ownership selection

[shwstppr]

Description

Fixes #10832

Do not call form fetchData/resetData while OwnershipSelection is still initializing.
To prevent race conditions in fetch-owner signal emitted by OwnershipSelection, a request token is tracked.

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

Feature/Enhancement Scale

• Major
• Minor

Bug Severity

• BLOCKER
• Critical
• Major
• Minor
• Trivial

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 16.17%. Comparing base (db5b6a5) to head (e0570f6).
⚠️ Report is 41 commits behind head on 4.20.

Additional details and impacted files

@@            Coverage Diff             @@
##               4.20   #11705    +/-   ##
==========================================
  Coverage     16.17%   16.17%            
- Complexity    13286    13297    +11     
==========================================
  Files          5656     5656            
  Lines        498015   498230   +215     
  Branches      60406    60456    +50     
==========================================
+ Hits          80538    80579    +41     
- Misses       408515   408681   +166     
- Partials       8962     8970     +8     

Flag	Coverage Δ
uitests	4.00% <ø> (-0.01%)	⬇️
unittests	17.02% <ø> (+<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.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[shwstppr]

@blueorangutan ui

[blueorangutan]

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

[blueorangutan]

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

[DaanHoogland]

clgtm, just one question; is there a possibility that during loading the ownership type changes from Accounts to Projects or vice versa? If so this could lead to an untimely this.initialized = true.

[shwstppr]

@DaanHoogland I tried to look into this scenario. While in normal cases it shouldn't happen but it could happen when first listAccounts/listProjects is taking time, the user changes the owner type in the meantime and the new listAccounts/listProjects call finishes before the first call. Parent component won't reset/fetchData in this case. Maybe this.initialized = true can be added to the changeDomain method as well.
I think to handle such a scenario, we should also add handling to not emit fetch-owner by the first listAccounts/listProjects API call when it finishes, to prevent wrong details being updated in the parent.
Let me know your thoughts

[shwstppr]

@blueorangutan ui

[blueorangutan]

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

[blueorangutan]

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

[DaanHoogland]

should this be

Suggested change

	this.initialized = true
	this.fetchOwnerData()
	this.fetchOwnerData()
	this.initialized = true

or

Suggested change

	this.initialized = true
	this.fetchOwnerData()
	this.initialized = false
	this.fetchOwnerData()

?

[shwstppr]

I think it is fine as it is. The method changeAccountTypeOrDomain will be called only when the user changes the value in the select boxes

[DaanHoogland]

@DaanHoogland I tried to look into this scenario. While in normal cases it shouldn't happen but it could happen when first listAccounts/listProjects is taking time, the user changes the owner type in the meantime and the new listAccounts/listProjects call finishes before the first call. Parent component won't reset/fetchData in this case. Maybe this.initialized = true can be added to the changeDomain method as well. I think to handle such a scenario, we should also add handling to not emit fetch-owner by the first listAccounts/listProjects API call when it finishes, to prevent wrong details being updated in the parent. Let me know your thoughts

this is quite an edge case and I am not sure how likely it is, but on busier systems it becomes more likely. I don’t think change domain should add the initialized =true, but check for it and set a flag that signifies that the data retrieved can be discarded. Now we can make the edge case more complex; what if there are multiple (more than three) attempts. How will we keep track of which is the actual data that matches the last request. this is /me regretting I asked the question…

maybe we should use two separate mutexes?

[shwstppr]

I don't think we need mutexes. Current changes will allow emitting fetch-owner signal only for the latest request which should cover those edge cases. Also, adding initialized=true on select box value change by user should allow updating the parent even in cases where initial request is still running

[shwstppr]

@blueorangutan ui

[blueorangutan]

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

[blueorangutan]

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

[DaanHoogland]

ok, maybe I am seeing imaginary bears on the road. Let’s fix if a problem turns up.

[DaanHoogland]

clgtm