-
Notifications
You must be signed in to change notification settings - Fork 317
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set username as triggerValue when possible #2382
Conversation
@CodiumAI-Agent /review |
PR Analysis(review updated until commit 4384ef3)
PR Feedback💡 General suggestions: The PR is well-structured and the changes are clear. However, it would be beneficial to include unit tests to verify the new functionality. Also, it would be helpful to provide a bit more context in the PR description about why this change is needed and how it improves the system. ✨ Usage guide:Overview:
With a configuration file, use the following template:
See the review usage page for a comprehensive guide on using this tool. |
Persistent review updated to latest commit 4384ef3 |
1 similar comment
Persistent review updated to latest commit 4384ef3 |
core/class/scenario.class.php
Outdated
@@ -840,6 +840,7 @@ public function launch($_trigger = '', $_message = '', $_forceSyncMode = false, | |||
$this->setTags($tags); | |||
} | |||
$this->setCache(array('startingTime' => strtotime('now'), 'state' => 'starting')); | |||
$this->setCache('lastExecutionUser', (isset($_SESSION) && isset($_SESSION['user']) && $_SESSION['user'] != null) ? $_SESSION['user']->getLogin() : 'none'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider handling the case where $_SESSION['user']
is not set more gracefully. Instead of setting the lastExecutionUser
to 'none', it might be better to leave it unset or set it to a more meaningful value. [medium]
Hello @Hotfirenet & @Sekiro-kost, |
Hello, No problem with this PR just we wait core 4.5 (maybe this week or next week). |
Hello, |
Hello, I think we can do better for this PR, currently it's use cache but now it's better to you use tag like this : Now tag is instanciated for current launch and do not impact other scenario launch |
Using the tag system in the instance, it is way easier to get the user who triggered the scenario. |
Proposed change
As per feature request here: https://community.jeedom.com/t/utilisateur-ayant-execute-la-commande/4488
Last user who executed the scenario is available using
triggerValue()
whentrigger()
is 'user'.(Also fixed a typo in core/ajax/user.ajax.php)
Type of change
Test check
Documentation
N/A