Remove references to the Override checkbox for filters #3548
Conversation
aneta-petrova
added
Needs tech review
| The *Unlimited* checkbox is selected by default, which means that the permission is applied on all resources of the selected type. | ||
| When you disable the *Unlimited* checkbox, the *Search* field activates. | ||
| . For certain *Resource type* options, the *Unlimited* checkbox is available. | ||
| If *Unlimited* is selected, the permission is applied on all resources of the selected type. |
There was a problem hiding this comment.
We might want to add that unlimited will be disabled if the role parent has a loc/org selected, alert in foreman says: "Info alert:The filter is scoped to the selected organizations and locations, therefore can't be unlimited"
There was a problem hiding this comment.
I'm inclined to say that we don't need to go into this kind of detail here. I left a comment on the code PR with a minor suggestion to change the wording of the alert. I'd say that if users will know what's happening from looking at the web UI form, then that's great and there is no need for additional words in the docs.
|
Thanks for the reviews, that covers tech review. |
aneta-petrova
added
tech review done
aneta-petrova
added
style review done
|
Hi @MariaAga, as you're reviving theforeman/foreman#10370, can you please also take a look at this documentation PR? Is it still okay like this, or should I make any changes to address the latest state of the code PR? |
|
@aneta-petrova I added another change, and if it will be accepted I'll add details here 🙏 |
|
We have adjusted the developers PR to include more changes: |
aneta-petrova
removed
tech review done
|
@sourcery-ai review |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR removes all references to the filter-level override checkbox from permission-related documentation, updating the relevant procedures to reflect the removal of override functionality. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `guides/common/modules/proc_adding-permissions-to-a-role.adoc:14` </location>
<code_context>
-The *Unlimited* checkbox is selected by default, which means that the permission is applied on all resources of the selected type.
-When you disable the *Unlimited* checkbox, the *Search* field activates.
+. For certain *Resource type* options, the *Unlimited* checkbox is available.
+If *Unlimited* is selected, the permission is applied on all resources of the selected type.
+If *Unlimited* is unselected, the *Search* field activates.
In this field you can specify further filtering with use of the {Project} search syntax.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The sentence uses passive voice ('is applied'), which should be revised to active voice for clarity.
Consider rephrasing to active voice, such as: 'Selecting *Unlimited* applies the permission to all resources of the selected type.'
<details>
<summary>Review instructions:</summary>
**Path patterns:** `guides/common/*.adoc,guides/common/modules/*.adoc`
**Instructions:**
Avoid passive voice in verbs.
</details>
</issue_to_address>
### Comment 2
<location> `guides/common/modules/proc_adding-permissions-to-a-role.adoc:15` </location>
<code_context>
-When you disable the *Unlimited* checkbox, the *Search* field activates.
+. For certain *Resource type* options, the *Unlimited* checkbox is available.
+If *Unlimited* is selected, the permission is applied on all resources of the selected type.
+If *Unlimited* is unselected, the *Search* field activates.
In this field you can specify further filtering with use of the {Project} search syntax.
For more information, see xref:Granular_Permission_Filtering_{context}[].
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The phrase 'is unselected' uses passive voice; prefer active voice for instructions.
You could rephrase as: 'If you clear *Unlimited*, the *Search* field activates.'
<details>
<summary>Review instructions:</summary>
**Path patterns:** `guides/common/*.adoc,guides/common/modules/*.adoc`
**Instructions:**
Avoid passive voice in verbs.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
aneta-petrova
force-pushed
the
orgs-locs-filter-level
branch
from
0e8616e to
73f4b80
Compare
aneta-petrova
added
the
Needs tech review
aneta-petrova
added
Needs style review
aneta-petrova
force-pushed
the
orgs-locs-filter-level
branch
2 times, most recently
from
fdf1c74 to
df8fe0e
Compare
aneta-petrova
force-pushed
the
orgs-locs-filter-level
branch
from
df8fe0e to
9d0a83a
Compare
|
Hi @MariaAga and @pnovotny, can you please re-review this PR? It's been a while since you approved it and the corresponding code PR has changed as @MariaAga explained in #3548 (comment). Based on the current status of the filter edit forms, I updated the docs PR to simply drop references to the Override and Unlimited checkboxes. Does this reflect the current behavior? |
|
The changes look good to me, thanks! |
aneta-petrova
added
tech review done
|
Re-reviewed. Looks good to me 👍 |
aneta-petrova
added
testing done
|
Dear writers, can someone please review? A style ack is all that is missing and it's really just several lines of docs being removed. |
Lennonka
added
style review done
(cherry picked from commit 276d0eb)
4 participants
What changes are you introducing?
Removing references to Override and Unlimited checkboxes.
Why are you introducing these changes? (Explanation, links to references, issues, etc.)
The filter web UI forms no longer include Override and Unlimited checkboxes. It is no longer possible to specify orgs and locs when editing a filter because these are always inherited from the role.
theforeman/foreman#10370
#3548 (comment)
Anything else to add? (Considerations, potential downsides, alternative solutions you have explored, etc.)
Checklists
Please cherry-pick my commits into: