Feature: Cisco Spark action new (#5090) #5092

tdeckers · 2017-02-14T13:17:48Z

Adding new action for Cisco Spark.

Supports messages to room and person
Optional default room config
Basic input validation

Signed-off-by: Tom Deckers tom@ducbase.com

* Supports messages to room and person * Optional default room config * Basic input validation Signed-off-by: Tom Deckers <tom@ducbase.com> Closes openhab#5090

9037568

In addition to the inline comments, please also create a wiki page for the new action and link it into the Actions wiki page.

9037568 · 2017-02-15T00:43:02Z

bundles/action/org.openhab.action.ciscospark/README.md

+
+### Configure openhab
+
+Configure openhab with the access token by adding the following to the file `services/ciscospark.cfg`:


openHAB in text for human use should always be presented as in line 4, as "openHAB".

I also believe this configuration instruction is incorrect for OH1. The text should not reference specific files.

@9037568 : The README.md is not concerned with OH1 runtime users, and should reference the .cfg file used in openHAB 2. If there is an expectation that this action will be used by OH1 runtime users (and why not), the body of README.md can be copied to a wiki page and adjustments can be made for OH1 users. Or, OH1 runtime users can just figure it out.

9037568 · 2017-02-15T00:45:55Z

bundles/action/org.openhab.action.ciscospark/build.properties

+bin.includes = META-INF/,\
+               .,\
+               OSGI-INF/,\
+               lib/ciscospark-client-1.0-SNAPSHOT.jar,\


~~What licence is used for this?~~

MIT, yes? I don't think that's a problem. Perhaps @watou can confirm?

MIT license is OK. It's good practice to add an about.html file explaining use of third-party content (like this), but much of this repo didn't.

9037568 · 2017-02-15T00:51:27Z

...enhab.action.ciscospark/src/main/java/org/openhab/action/ciscospark/internal/CiscoSpark.java

+
+        // Validate message
+        if (msgTxt == null || "".equals(msgTxt)) {
+            logger.error("Message can't be empty");


This should be a WARN or lower. (see the guidelines)

Please review other logging messages and update them accordingly.

9037568 · 2017-02-15T01:05:39Z

...ciscospark/src/main/java/org/openhab/action/ciscospark/internal/CiscoSparkActionService.java

+            CiscoSpark.spark = createSpark();
+            logger.debug("Retrieving user...");
+            Person person = CiscoSpark.spark.people().path("/me").get();
+            logger.info("Cisco Spark logged in as " + person.getDisplayName());


Should use parameterized logging instead of string concatenation.

9037568 · 2017-02-15T01:08:16Z

...ciscospark/src/main/java/org/openhab/action/ciscospark/internal/CiscoSparkActionService.java

+    public void updated(Dictionary<String, ?> config) throws ConfigurationException {
+        if (config != null) {
+
+            String accessTokenString = (String) config.get("accessToken");


For compatibility with OH2, should use Objects.toString instead. eg.

... = Objects.toString(config.get("accessToken"), null);

9037568 · 2017-02-15T01:23:02Z

...enhab.action.ciscospark/src/main/java/org/openhab/action/ciscospark/internal/CiscoSpark.java

+ */
+public class CiscoSpark {
+
+    private static final Logger logger = LoggerFactory.getLogger(CiscoSpark.class);


Logger should not be static.

@9037568 Loggers in nearly all other OH1 add-ons are static, so I think it's OK for this to be as well. If it ever comes time to run through this repo and change all loggers to non-static, it's the difference between changing 1,000 and 1,001 cases. Code in openhab2-addons, by contrast, started out using non-static loggers so it's required in that repo.

Left this one static for now. Couldn't easily change the action methods to non-static. I used the Twitter action as example and didn't find an example with non-static action methods.

9037568 · 2017-02-15T01:24:26Z

...ciscospark/src/main/java/org/openhab/action/ciscospark/internal/CiscoSparkActionService.java

@@ -0,0 +1,129 @@
+/**
+ * Copyright (c) 2010-2016 by the respective copyright holders.


9037568 · 2017-02-15T01:25:01Z

...ciscospark/src/main/java/org/openhab/action/ciscospark/internal/CiscoSparkActionService.java

+ */
+public class CiscoSparkActionService implements ActionService, ManagedService {
+
+    private static final Logger logger = LoggerFactory.getLogger(CiscoSparkActionService.class);


Logger should not be static.

9037568 · 2017-02-15T01:26:02Z

...ion.ciscospark/src/main/java/org/openhab/action/ciscospark/internal/CiscoSparkActivator.java

@@ -0,0 +1,55 @@
+/**
+ * Copyright (c) 2010-2016 by the respective copyright holders.


9037568 · 2017-02-15T01:26:23Z

...ion.ciscospark/src/main/java/org/openhab/action/ciscospark/internal/CiscoSparkActivator.java

+ */
+public final class CiscoSparkActivator implements BundleActivator {
+
+    private static Logger logger = LoggerFactory.getLogger(CiscoSparkActivator.class);


Logger should not be static.

tdeckers · 2017-02-15T09:10:58Z

Updated PR... I'll add the wiki page shortly.

watou · 2017-02-15T09:16:34Z

bundles/action/org.openhab.action.ciscospark/.project

+		<nature>org.eclipse.jdt.core.javanature</nature>
+		<nature>org.eclipse.pde.PluginNature</nature>
+	</natures>
+</projectDescription>


Remove maven2Builder and maven2Nature.

watou · 2017-02-15T09:33:16Z

bundles/action/org.openhab.action.ciscospark/OSGI-INF/action.xml

+    http://www.eclipse.org/legal/epl-v10.html
+
+-->
+<scr:component xmlns:scr="http://www.osgi.org/xmlns/scr/v1.1.0" activate="activate" configuration-policy="require" deactivate="deactivate" immediate="true" name="org.openhab.action.ciscospark.action">


I do not know if specifying configuration-policy without also specifying configuration-pid works. I think (but am not sure) that the absence of configuration-pid causes it to default to the value of the name attribute, which in this case is incorrect. configuration-pid should be the same as the value of the service-pid property. Together, configuration-policy="require" and configuration-pid="org.openhab.ciscospark" should mean that the component is only activated if there is a configuration under the persistence ID org.openhab.ciscospark.

I'm not sure what this means... Admittedly, I'm no osgi/karaf expert. I've been digging into the docs here, but am not seeing any reference to an actual configuration.pid property. The text mentions the configuration pid, but appears to refer to service.pid as the actual property.
I'm testing this by manually adding this plugin to the addon directory and the plugin does get loaded and is functional.
If there are changes needed, can give me few pointers/examples of how to proceed? Thanks for the feedback so far!

I was referring to a file like this one, but since your testing shows that the service.pid property is working with the "require" configuration policy, then I would say that no change is needed.

watou · 2017-02-15T09:59:54Z

...ciscospark/src/main/java/org/openhab/action/ciscospark/internal/CiscoSparkActionService.java

+
+            if (isBlank(accessToken)) {
+                throw new ConfigurationException("ciscospark",
+                        "The parameters 'accessToken' is missing! Please refer to your config file.");


watou · 2017-02-15T10:03:20Z

bundles/action/org.openhab.action.ciscospark/build.properties

@@ -1,5 +1,4 @@
-source.. = src/main/java/,\
-           src/main/resources/


src/main/resources isn't empty, so this entry should be restored

tdeckers · 2017-02-15T22:49:07Z

Wiki page created: https://github.com/openhab/openhab1-addons/wiki/Cisco-Spark-Action
Haven't linked it yet. I assume we need to get this into a release first?

watou · 2017-02-16T02:15:14Z

bundles/action/org.openhab.action.ciscospark/README.md

+
+## Introduction
+
+[Cisco Spark](https://www.ciscospark.com) is a simple and secure way for collaborating with individuals and groups across the cloud.  This Action plugin allows you to send messages to _rooms_ and individuals when certain events take place in openHAB. Messages in the Cisco Spark plugin support Markdown text.


please remove lines 3-7 inclusive. This file is processed to appear on docs.openhab.org so a particular set of conventions are expected.

watou · 2017-02-16T02:17:13Z

...ciscospark/src/main/java/org/openhab/action/ciscospark/internal/CiscoSparkActionService.java

+ * This class registers an OSGi service for the Cisco Spark action.
+ *
+ * @author Tom Deckers
+ * @since 1.9.0


watou · 2017-02-16T02:17:26Z

...ion.ciscospark/src/main/java/org/openhab/action/ciscospark/internal/CiscoSparkActivator.java

+ * Extension of the default OSGi bundle activator
+ *
+ * @author Tom Deckers
+ * @since 1.9.0


watou · 2017-02-16T02:17:45Z

...enhab.action.ciscospark/src/main/java/org/openhab/action/ciscospark/internal/CiscoSpark.java

+ * sending Messages via Cisco Spark.
+ *
+ * @author Tom Deckers
+ * @since 1.9.0


watou · 2017-02-16T02:20:04Z

Change occurrences @since 1.9.0 to @since 1.10.0 and remove noted lines in README.md, and I think it will be good to merge!

Wiki page created: https://github.com/openhab/openhab1-addons/wiki/Cisco-Spark-Action
Haven't linked it yet. I assume we need to get this into a release first?

Since it's a working add-on at this point and soon to be in a nightly build, I think it's OK to add the action to the proper place in the sidebar and linked properly in the Actions wiki page. For all OH2 users, however, the wiki should not matter for add-ons.

* Changed to non-static logger where possible, error -> warn * Changed year * Added about and detail license for dependency * Remove maven nature from project

tdeckers · 2017-02-16T09:55:36Z

I've made all updates as requested. I only backtracked on the settings for: configuration-policy="require" configuration-pid="org.openhab.ciscospark". To support the change, I also changed the namespace to http://www.osgi.org/xmlns/scr/v1.2.0.
With these settings, the sparkMessage command wasn't recognized by the rules I created to test. Even though the bundle in karaf is listed as Active.

2017-02-16 09:14:25.770 [ERROR] [.script.engine.ScriptExecutionThread] - Rule 'test': An error occured during the script execution: The name 'sparkPerson(<XStringLiteralImpl>,<XStringLiteralImpl>)' cannot be resolved to an item or type.

With the current version of the code in this PR the plugin loads properly.

watou · 2017-02-16T10:03:40Z

LGTM

Feature: Cisco Spark action (openhab#5090)

8b932dd

* Supports messages to room and person * Optional default room config * Basic input validation Signed-off-by: Tom Deckers <tom@ducbase.com> Closes openhab#5090

tdeckers force-pushed the feature/5090-ciscospark branch from 1f508cc to 8b932dd Compare February 14, 2017 14:16

9037568 added the review-in-progress label Feb 15, 2017

9037568 suggested changes Feb 15, 2017

View reviewed changes

9037568 added awaiting-feedback and removed review-in-progress labels Feb 15, 2017

watou added the new-binding-action-service label Feb 15, 2017

watou added this to the 1.10.0 milestone Feb 15, 2017

watou reviewed Feb 15, 2017

View reviewed changes

tdeckers force-pushed the feature/5090-ciscospark branch from e5c382d to 0e4de24 Compare February 15, 2017 09:32

watou reviewed Feb 15, 2017

View reviewed changes

tdeckers force-pushed the feature/5090-ciscospark branch from 0e4de24 to ca8b832 Compare February 15, 2017 22:20

tdeckers force-pushed the feature/5090-ciscospark branch from ca8b832 to 59b15d2 Compare February 15, 2017 22:49

watou reviewed Feb 16, 2017

View reviewed changes

Fix: address PR comments

3fc0ece

* Changed to non-static logger where possible, error -> warn * Changed year * Added about and detail license for dependency * Remove maven nature from project

tdeckers force-pushed the feature/5090-ciscospark branch from 59b15d2 to 3fc0ece Compare February 16, 2017 09:50

9037568 removed the awaiting-feedback label Feb 16, 2017

9037568 merged commit dd83de0 into openhab:master Feb 16, 2017

tdeckers deleted the feature/5090-ciscospark branch July 7, 2017 08:35


		### Configure openhab

		Configure openhab with the access token by adding the following to the file `services/ciscospark.cfg`:

		@@ -0,0 +1,129 @@
		/**
		* Copyright (c) 2010-2016 by the respective copyright holders.

		@@ -0,0 +1,55 @@
		/**
		* Copyright (c) 2010-2016 by the respective copyright holders.

		@@ -1,5 +1,4 @@
		source.. = src/main/java/,\
		src/main/resources/


		## Introduction

		[Cisco Spark](https://www.ciscospark.com) is a simple and secure way for collaborating with individuals and groups across the cloud. This Action plugin allows you to send messages to _rooms_ and individuals when certain events take place in openHAB. Messages in the Cisco Spark plugin support Markdown text.

Uh oh!

Feature: Cisco Spark action *new* (#5090) #5092

Feature: Cisco Spark action *new* (#5090) #5092

Uh oh!

Conversation

tdeckers commented Feb 14, 2017

Uh oh!

9037568 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tdeckers commented Feb 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tdeckers commented Feb 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

watou commented Feb 16, 2017

Uh oh!

tdeckers commented Feb 16, 2017

Uh oh!

watou commented Feb 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Feature: Cisco Spark action new (#5090) #5092

Feature: Cisco Spark action new (#5090) #5092