[ACS-3895] ACA - Folder Rules: inherit rule sets toggle #2808
Conversation
projects/aca-folder-rules/src/lib/manage-rules/manage-rules.smart-component.html
Outdated
Show resolved
Hide resolved
projects/aca-folder-rules/src/lib/manage-rules/manage-rules.smart-component.ts
Outdated
Show resolved
Hide resolved
pr-triage
bot
added
PR: partially-approved
PR: unreviewed
and removed
PR: unreviewed
PR: partially-approved
labels
There was a problem hiding this comment.
Good work!
Your method is fine, and I'm sure works well, but I'll admit it's not what I would have done. I'm not going to request changes due to the short time frame we have but see my review for what I think is a better approach.
Feel free to attempt it this afternoon if you're up to it and don't have anything else to do, otherwise I'm happy enough with this and you can merge it before the end of the day so it's hopefully propagated to the remote environment tomorrow.
| @@ -49,6 +50,8 @@ import { RuleSet } from '../model/rule-set.model'; | |||
| }) | |||
| export class ManageRulesSmartComponent implements OnInit, OnDestroy { | |||
| nodeId = ''; | |||
| isInheritanceEnabled = true; | |||
There was a problem hiding this comment.
Could we have this as an observable in FolderRuleSetsService instead?
| this.folderRulesService.getRuleSettings(this.nodeId).then((ruleSettings) => { | ||
| this.isInheritanceEnabled = ruleSettings.value; | ||
| }); |
There was a problem hiding this comment.
We should load this at the same time as loadRuleSets(this.nodeId) in the FolderRuleSetsService. Like that it will be part of the initial loading of the page and use the same mat progress bar for everything.
| @@ -152,6 +152,16 @@ export class FolderRulesService { | |||
| ); | |||
| } | |||
|
|
|||
| async getRuleSettings(nodeId: string, key: string = '-isInheritanceEnabled-'): Promise<RuleSettings> { | |||
There was a problem hiding this comment.
Since the rule set service handles the node id, this should be in FolderRuleSetsService. We can load it at the same time in the loadRuleSets method by adding it in the combineLatest, here:
switchMap(() => combineLatest(
this.getMainRuleSet(nodeId),
loadInheritedRuleSets ? this.getInheritedRuleSets(nodeId) : of([])
))
This could become:
switchMap(() => combineLatest(
this.getMainRuleSet(nodeId),
loadInheritedRuleSets ? this.getInheritedRuleSets(nodeId) : of([]),
this.getFolderInheritance(nodeId)
))
and then using the value in the subscribe to emit to the isFolderInheritanceEnabledSource BehaviourSubject.
There was a problem hiding this comment.
I made getRuleSettings more generic, so it could be used in future with other parameters. Do you suggest to redesign it to getFolderInheritance for now, so it will only load data for isInheritanceEnabled$?
There was a problem hiding this comment.
Considering it's likely the only rules / rule sets related setting for folders that we have, I don't think it's necessary to make it generic like this. Is this endpoint one of the new private ones? Did you check if there a generic js-api method that could allow you to call this endpoint and then use the folder rule (sets) service to be more specific about isInheritanceEnabled?
nikita-web-ua
force-pushed
the
feature/dev-mmaliarchuk-ACS-3895-folder-rules-inherit-rule-sets-toggle
branch
from
731cb33
to
2fd10a0
Compare
nikita-web-ua
deleted the
feature/dev-mmaliarchuk-ACS-3895-folder-rules-inherit-rule-sets-toggle
branch
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (check one with "x")
What is the current behaviour? (You can also link to an open issue here)
Currently in ACA, the user does not have the ability to choose whether a folder should inherit folder rule sets from parent folders or not.
What is the new behaviour?
Each folder has a button to toggle whether it should inherit folder rule sets from its parent folders.
Does this PR introduce a breaking change? (check one with "x")
If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...
Other information: