feat(configurable-actions): add userinfo in configurable-actions#2415
Closed
belfhi wants to merge 6 commits into
Closed
feat(configurable-actions): add userinfo in configurable-actions#2415belfhi wants to merge 6 commits into
belfhi wants to merge 6 commits into
Conversation
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Consider replacing the
userObject: anyparameter inprocessSelectorwith a strongly-typed user profile interface so that misuse and typos on fields likeusername/emailare caught at compile time. - It might be safer to guard against
this.userProfilebeing undefined/null before passing it intoprocessSelector, and define a clear fallback behavior when user information is not available. - For consistency with existing selectors like
#datasetOwnerand#userIsAdmin, consider using a consistent naming convention (e.g.#userName,#userEmail) for the new user-related selectors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider replacing the `userObject: any` parameter in `processSelector` with a strongly-typed user profile interface so that misuse and typos on fields like `username`/`email` are caught at compile time.
- It might be safer to guard against `this.userProfile` being undefined/null before passing it into `processSelector`, and define a clear fallback behavior when user information is not available.
- For consistency with existing selectors like `#datasetOwner` and `#userIsAdmin`, consider using a consistent naming convention (e.g. `#userName`, `#userEmail`) for the new user-related selectors.
## Individual Comments
### Comment 1
<location path="src/app/shared/modules/configurable-actions/configurable-action.component.ts" line_range="130" />
<code_context>
private processSelector(
jsonObject: ActionItems,
+ userObject: any,
selector: string,
): string | string[] | number | number[] {
</code_context>
<issue_to_address>
**suggestion:** Avoid `any` for `userObject` and describe its shape explicitly.
Given we access specific fields like `username` and `email` on `userObject`, please introduce a typed interface (e.g. `UserProfile`) and use that instead of `any` to document expected properties and gain type safety.
Suggested implementation:
```typescript
interface UserProfile {
username: string;
email: string;
}
export class ConfigurableActionComponent
```
```typescript
private processSelector(
jsonObject: ActionItems,
userObject: UserProfile,
selector: string,
): string | string[] | number | number[] {
```
If the class name in this file is not `ConfigurableActionComponent`, adjust the first SEARCH/REPLACE block to match the actual `export class ...` line so the `UserProfile` interface is inserted in the appropriate place.
You may also want to expand `UserProfile` with any additional fields accessed elsewhere in the component to keep type usage consistent.
</issue_to_address>
### Comment 2
<location path="docs/contributors/configurable_actions_technical.md" line_range="153" />
<code_context>
| #uuid | string | A v4 uuid generated on the fly |
+| #Username | string | The currently logged in user's username |
+| #UserEmail | string | email address of the user |
| @variable | any | replace the string with the valu eof the variable defined in the action variable |
## How to Configure
</code_context>
<issue_to_address>
**issue (typo):** Fix the typo "valu eof" to "value of".
```suggestion
| @variable | any | replace the string with the value of the variable defined in the action variable |
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Contributor
Author
|
Hi Carlo, I wasn't aware that you had proposed those changes. We can resolve your PR, that's ok to me. |
Contributor
Author
|
closing since this functionality was added in #2350 |
This file contains hidden or 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
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.
Description
We want to create Jobs via configurable-actions and the email field needs to be populated by the current user's email address.
This PR adds this functionality.
Documentation
official documentation info
If you have updated the official documentation, please provide PR # and URL of the pages where the updates are included
Summary by Sourcery
Expose current user information as variables in configurable actions and document the new placeholders.
New Features:
Documentation: