Add device group feature flag in TeamType #3235
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## 2989-assign-device-group-to-pipeline-stage #3235 +/- ##
==============================================================================
- Coverage 40.70% 40.68% -0.03%
==============================================================================
Files 598 598
Lines 22311 22327 +16
Branches 5374 5383 +9
==============================================================================
+ Hits 9082 9084 +2
- Misses 13229 13243 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Pezmc
reviewed
Dec 20, 2023
Pezmc
reviewed
Dec 20, 2023
Steve-Mcl
approved these changes
Dec 20, 2023
|
@Steve-Mcl can you have a look at the last set of commits?
|
Steve-Mcl
reviewed
Dec 20, 2023
Comment on lines
+15
to
+19
| <ff-loading | ||
| v-if="loading" | ||
| message="Loading Device Groups..." | ||
| /> | ||
| <div v-else-if="deviceGroups?.length > 0" class="pt-4 space-y-6" data-el="pipelines-list"> |
There was a problem hiding this comment.
Instead of show/hide whole portions of the page, perhaps we should simply utelise the features of the data table like we do for "Loading Teams" and "Loading Users"?
https://flowfuse.github.io/forge-ui-components/#ff-data-table
Property Default Description loadingfalse Shows a placeholder Loading message in place of the data. loading-message'Loading Data...' If loading=true, then this message will be shown.
Pezmc
approved these changes
Dec 20, 2023
knolleary
merged commit 390291b
into
2989-assign-device-group-to-pipeline-stage
9 of 10 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds a
deviceGroupfeature flag to TeamType so the feature can be limited to certain team types.As part of this, I found that if you open an application in a team that isn't your default team directly (ie paste the url in, or Ctrl-R on the page), we would load the user's default team first, then load the team of the application you were accessing.
This would cause the view to first load data associated with your default team, before refreshing with data for the team you were accessing - in some cases, it wouldn't refresh because there was no watch happening.
To fix this, I've modified the account load phase to skip loading the user's team if the url is on either
/application,/deviceor/instance- as all of these views do a lookup of the team and callsetTeamanyway. This avoids the double load.However, I then hit the problem with the order the
applicationpage does things. It loads the application information, devices, deviceGroups etc, then it ensures the loaded team is the one for the application. This leads to a chicken-vs-egg situation as we now need to check the team's feature flags to decide whether to load the device groups or not.... but we may not have loaded the team yet.As a workaround, I've put a try/catch around the loading of deviceGroups, which will 404 if the team isn't allowed to use device groups (we at least have the platform-level guard in there). I then pass a flag down through all of the Pipeline components to say if deviceGroups is enabled or not. I dislike this model - I find it much easier to reason over if a component usesmapStateto access the team locally, rather than all of this passing down of properties. I'm sure there is a Vue reason it's being done the way it is...Updated via 2ced020 - I've modified the order the Application loads things, having realised we can await the loading of the application team (if needed). This ensure we have the Team object so can check the device group feature flag. As a consequence, I've also removed the
deviceGroupEnabledproperty I had added - cleaner (IMHO) to check the team directly from the store (ie viamapState...).