Lucid migration

[5iver]

Round 3 of migration. This should wrap up the bulk of the changes, and we are ready for testing. ideAlarm and weatherStationUpdloader are now in my fork. The lucid-migration branch in this repo is behind, so please use my fork for testing. We will also need to discuss/resolve everything below.

Things removed during lucid migration: 1) randomization of cron expressions (prefer manual configuration) 2) did not include pronounce (could be added with a Community TTS script) 3) did not include hasReloadFinished, since the StartupDelay script takes care of this 4) removed core.utils.curDateText since core.date.py has similar function 5) In rules.py, the globals injected into the function are not necessary, since they are already in the default scope. Log is already being added as an attribute to the action functions. I also do not see a need for the cron expressions, but I could be convinced that these could go into configuration.py. The event object concerns me. I don't think it is a good idea to be modifying this, and most of what is being added is either already there and being overwritten. isActive is a duplicate of what is found in utils.py (which I think should be moved... see 3), and hasReloadFinished is unecessary due to the startup delay script.

Things I think should be removed: 1) core.utils.postUpdate and sendCommand should be removed, since they are converting to string, but that is not always correct and will cause issues. All these do is remove the need to type 'events.' before the commands, so I also do not think they are necessary. 2) [DONE] Nothing in core should be using customItemNames. Anything using it can be moved to implementation specific modules in community. 3) core.utils.getItemValue does not cover all possible cases for Item type. If we keep it, this should be cleaned up, but I really do not see much value in it. 4) [pe was cleaned up] core.utils.getLastUpdate should be removed. I'm not clear on the useage, but it appears to require PersistenceExtension to be supplied as a parameter? 5) TimeOfDay example rule should be removed or corrected (use String instead of Number). I've added another example that could replace it. 6) [DONE] isActive, isBright, isShady, isDark, isBlack shouild all be moved out of utils and into an implementation specific script or module.

Other considerations: 1) StartupTrigger will not work until the fix is merged (after this one). 2) PUSHOVER_DEF_DEV was imported in ideAlarm, but it wasn't used and there was no definition for it. 3) Any concern with moving community.clickatell.sendsms to clickatell.init.py? 4) I've migrated weatherStationUploader, but it will not work since it uses wunderground, who have changed their API. 5) What to do with versioning? There has been some discussion here about approaches for this. I've bumped some versions, but definitely something to verify in the review. 6) I tried something in IdeAlarm.getTriggers(), which wasn't really unecessary but will allow use of the decorators for rule definitions. Definitely needs testing. 7) postUpdateCheckFirst should use TypeParser.parseCommand and TypeParser.parseState.

Fixes #51, #52, #53.

[besynnerlig]

Thanks Scott.

I will dive into this as soon as you have finished and I will answer to each comment that you've made of course.

Encountered new problems using my Windows git client on a SSH mounted shared directory on my linux server. I think I'll have to give that up. Maybe it was doomed to malfunction from start. I will have to figure out a better setup and a better workflow and do all the git stuff from the command line in linux instead.

Please let me know when you think it's a good time for me to start testing.

[besynnerlig]

@openhab-5iver

A few questions...

Should I test this in OH2.4 stable?

I was thinking that the best test would be to use all my current scripts including ideAlarm. Do we really need a separate branch for ideAlarm or can we try to incorporate it into the Lucid branch? What would you prefer?

[besynnerlig]

Things I think should be removed:

1. I don't think configuration.postUpdate and sendCommand should be included, since they are converting to string, but that is not always needed and may cause issues

Unless I convert it to a string I'll get the following error:

TypeError: sendCommand(): 1st arg can't be coerced to String, org.eclipse.smarthome.core.items.Item

Using code

       from lucid.jsr223.scope import events
       events.sendCommand("Test_Number", 1)

Even though Test_Number is defined as a Number type Item.

[5iver]

Should I test this in OH2.4 stable?

Yes. Anything <= S1524. Builds between S1524 and S1566 had a bug that broke scripted automation and was resolved in S1566. But that build also included the ESH reintegration, where some packages were renamed and requires changes to the libraries. Once lucid is migrated, I will get some releases built so that people can easily pick a version of the libraries to suit their OH version.

Do we really need a separate branch for ideAlarm or can we try to incorporate it into the Lucid branch?

These can be merged. I'll do that today.

Unless I convert it to a string I'll get the following error:

To use sendCommand where the command is not a String, you need to use an Item. Similar for postUpdate.

events.sendCommand(ir.getItem("Test_Number"), 5)

https://www.openhab.org/docs/configuration/jsr223.html#events-operations

[5iver]

Encountered new problems using my Windows git client on a SSH mounted shared directory

Try a samba mount and VS Code. This works well for me for quick stuff (but I use Fedora for my OS... client and server), but I've also started using Eclipse with the Jython stuff. I has it's own quirks, but EGit has some benefits, particularly the interactive rebase.

[besynnerlig]

Try a samba mount and VS Code

Thanks I will try to set up a Samba mount tomorrow on my Ubuntu server.

[5iver]

But for backward compatibility and to prevent creating a breaking change, what if we just modify them like this:

I think that will resolve the issue, but I'm not clear on why you couldn't just send what is received.

My issue though, is that I question the need for this in the helper libraries at all, especially in core. The only convenience provided is to not have to type events., and I don't see that as such a terrible burden (:slightly_smiling_face:)! In the future, it may be possible to add the ScriptBusEvent methods as functions into the default scope, as is done for the DSL.

It's really not a big concern for me though, so I left them in!

[besynnerlig]

Have we now reached the point where it's time for me to start testing?

[5iver]

Yes... ready for initial smoke tests, but expect errors! I've tested some of the scripts for proper formatting, but nothing functional. It will be a lot easier for people who were already using the libraries to do the testing. It may be easier to correct errors after it is merged, but it would be nice to get as much as we can fixed now.

[besynnerlig]

Yes... ready for initial smoke tests, but expect errors! I've tested some of the scripts for proper formatting, but nothing functional. It will be a lot easier for people who were already using the libraries to do the testing. It may be easier to correct errors after it is merged, but it would be nice to get as much as we can fixed now.

Thanks a bunch!

The github pull requests extension in VSC is new to me and there is a learning threshold. I'll try to get used working with it and I'll see if I can start using code commenting as well.

One thing that happens if I checkout the pr/openhab-5iver/91* PR locally is that some recent changes in master are not there. (Missing .gitignore file and more...) I'll have to study how to handle that I guess or maybe the pull request can be rebased with master?

[5iver]

I'll try to get it synced up, but you could just download https://github.com/openhab-5iver/openhab2-jython/tree/lucid-migration and copy the files you need.

[besynnerlig]

I'm still a NOOB when it comes to github. I managed to get it synced locally but struggeled with merging conflicts due to differences regarding the "But-How-Do-I.md" whether it was renamed or deleted and git complained that I had to reolve that and commit before merge is possible. Anyway I got passed it but I probably messed up my local repo a bit. I might want to re-clone the original repo when/if you make the PR up to date with master.

I added a note in the getting started document about the need of creating a config file.

- You'll need a main configuration file for openhab2-jython. In directory `Core\automation\lib\python\`, rename the file `configuration.py.example` to `configuration.py` Then edit the configuration file to suit your needs.

I'm still trying to figure out a good workflow so that we don't have to make every single change 3 times. (I change, I write a comment here, you edit the original files). Maybe I need a web repo fork as an intermediate so that I can push my change proposals. Yeah, I think I will try to set that up tomorrow.

EDIT: I've now made forked openhab-5iver/openhab2-jython to a personal web repo and I sucessfylly merged the master branch into the lucid-migration branch. That worked well. After that I cloned my repo to my local server, checked out the lucid-migration branch (which is now up to date) and I start to push my changes to it. I guess that when I'm finished I can make a PR to your repo openhab-5iver/openhab2-jython Do you think that will be a good way forward?

I have one question though.

Previously I could change the logging level at rule level which I found very convenient.

@rule
class doorBell(object):
    """Someone rings the door bell"""
    def getEventTriggers(self):
        return [
            ItemStateUpdateTrigger('Door_Signal', str(ON)),
        ]
    def execute(self, modules, inputs):
        self.log.setLevel(DEBUG)

DEBUG is no longer in the global scope and even if I try to set the logging level for a rule it doesn't seem to make any difference.

I tried this but it didn't work out:

@rule("Hello World cron rule (decorator)", description="This is an example rule that demonstrates using a cron rule with decorators", tags=["Test tag", "Hello World"])# [description and tags are optional]
@when("Time cron 0/10 * * * * ?")
def hellowWorldDecoratorCron(event):
    from logging import DEBUG, INFO, WARNING, ERROR
    hellowWorldDecoratorCron.log.setLevel(DEBUG)
    hellowWorldDecoratorCron.log.info("This is a 'hello world!' from a Jython rule (decorator): Cron")

OK, today no progress for me really, just learning ...

EDIT:

In lucid (rules.py) I had the following code:

from org.eclipse.smarthome.core.library.types import IncreaseDecreaseType, NextPreviousType, OnOffType, OpenClosedType, PlayPauseType, RewindFastforwardType, StopMoveType, UpDownType, DecimalType

from lucid.log import logging, log_traceback, LOG_PREFIX
from logging import DEBUG, INFO, WARNING, ERROR

import lucid.config as config

from lucid.jsr223 import scope, get_automation_manager
from lucid.jsr223.scope import events, itemRegistry
import random
import time

NULL = UnDefType.NULL
UNDEF = UnDefType.UNDEF
ON = OnOffType.ON
OFF = OnOffType.OFF
OPEN = OpenClosedType.OPEN
CLOSED = OpenClosedType.CLOSED

and

def wrap_execute(fn):
    """Wrapper to extend the execute function"""
    functools.wraps(fn)
    def wrapper(self, modules, inputs):
        hasReloadFinished()
        self.event = Event(inputs)

        # Add global names, using the function's own global namespace:
        g = fn.__globals__
        g['DEBUG'] = DEBUG
        g['INFO'] = INFO
        g['WARNING'] = WARNING
        g['ERROR'] = ERROR
        g['NULL'] = NULL
        g['UNDEF'] = UNDEF
        g['ON'] = ON
        g['OFF'] = OFF
        g['OPEN'] = OPEN
        g['CLOSED'] = CLOSED

        fn(self, modules, inputs)
        # Place to add stuff after wrapped function
    return wrapper

I added this wrapper function into lucid trying to make just small things easier for people who like to get started scripting without prior experience. Is it something we'd like to retain or what do you think?

[5iver]

I guess that when I'm finished I can make a PR to your repo openhab-5iver/openhab2-jython Do you think that will be a good way forward?

Yes... that was one of the options. :smile: I'll also be getting the gitignore change synced.

DEBUG is no longer in the global scope and even if I try to set the logging level for a rule it doesn't seem to make any difference.

Can you just change this...

hellowWorldDecoratorCron.log.info("This is a 'hello world!' from a Jython rule (decorator): Cron")

to

hellowWorldDecoratorCron.log.debug("This is a 'hello world!' from a Jython rule (decorator): Cron")

Or am I missing something? I have set the LOG_PREFIX to "jsr223.jython", so be sure to set the logging level for this to at least DEBUG. You could log:set DEBUG jsr223.jython, or I manually update the org.ops4j.pax.loging.cfg file with...

log4j2.logger.jython.name = jsr223.jython
log4j2.logger.jython.level = DEBUG

... which is what the Karaf console command is adding. I also setup two appenders... one for rules and one for core. I'll be sure to add some explanation for logging in the doc.

I added this wrapper function into lucid trying to make just small things easier for people who like to get started scripting without prior experience. Is it something we'd like to retain or what do you think?

All of this (and more) is added to the context through the "default" scriptExtension, which is added to all scripts...

from org.eclipse.smarthome.core.library.types import IncreaseDecreaseType, NextPreviousType, OnOffType, OpenClosedType, PlayPauseType, RewindFastforwardType, StopMoveType, UpDownType, DecimalType

NULL = UnDefType.NULL
UNDEF = UnDefType.UNDEF
ON = OnOffType.ON
OFF = OnOffType.OFF
OPEN = OpenClosedType.OPEN
CLOSED = OpenClosedType.CLOSED

... so I did not see the need for them (number 5 in the Things Removed section of OP). Are you not able to access them in your scripts?!

[5iver]

OK... git fought hard, but I finally got things synced. The commits look weird, but I'll squash before the merge.

[besynnerlig]

EDIT: Logging levels worked as before. I removed that part from this text. Checking my 2.3 installation lucid log level shows DEBUG! That was a surprise. I didn't know I had set it to that. It seems that I can only use setLevel in a rule to a higher value locally than what is specified globally. The other way around doesn't work and probably shouldn't. ;)

I haven't tested a lot but the hello_world.py script can't access the global name DEBUG
NameError: global name 'DEBUG' is not defined . (ON is accessible though)

Cheers!

EDIT: I'm setting up a copy of my home environment for testing purposes. I'll have access to all my familiar OH Items etc that I'm used to and I'll be able to test the migration better. It takes some time though but I feel there is some progress. ;)

[5iver]

I haven't tested a lot but the hello_world.py script can't access the global name DEBUG NameError: global name 'DEBUG' is not defined . (ON is accessible though)

I'm a little confused... is this not an issue?

[besynnerlig]

I haven't tested a lot but the hello_world.py script can't access the global name DEBUG NameError: global name 'DEBUG' is not defined . (ON is accessible though)

I'm a little confused... is this not an issue?

It would be convenient to have it as a global name if it's frequently used. However it can easily be imported though. I use it in almost every script in my installation. ;)

[besynnerlig]

I'm now starting the testing with the community script weatherStationUploader.py

Maybe we should add a section in the documentation for the steps involved to get started using a community script?

I assume that for using weatherStationUploader I shall copy (or even make a soft link) Community\WeatherStationUploader\automation\jsr223\community\weatherStationUploader.py to automation\jsr223\community\weatherStationUploader.py and I shall copy and paste everything from Community\WeatherStationUploader\automation\lib\python\configuration.py.example into my configuration file automation\lib\python\configuration.py.

I'll start with that and testing if linking it works.

EDIT: If users shall copy custom scripts into the Core\automation\jsr223\community we need to add that folder to .gitignore. I'll do that.

[besynnerlig]

OK, enough testing for today.

I need to access NULL and UNDEF in utils.py

They are currently not in the global name space. I solved it by adding

# Some useful constants
from org.eclipse.smarthome.core.types import UnDefType
NULL = UnDefType.NULL
UNDEF = UnDefType.UNDEF

I can think of many other scripts that NULL and UNDEF are useful. Can we add them in some way to the global name space (maybe together with DEGUG, INFO, WARN) or what do you think?

[mjcumming]

When using Jyhton when should we be using a native Python type, ie None, vs a Java type, ie Null?

[5iver]

when should we be using a native Python type, ie None, vs a Java type, ie Null?

Always use None. Jython converts this to null for Java.

[5iver]

I need to access NULL and UNDEF in utils.py

Strange. This should be providing these, since they are in the default scope...

scope.scriptExtension.importPreset("default")

But scope.UnDefType.NULL should work too (no need for additional imports). I'll look into this.

Maybe we should add a section in the documentation for the steps involved to get started using a community script?

Yes. I think I wrote one with the next round of updates that was waiting for the ESH reintegration to wrap up. I'll add it into lucid-migration.

maybe together with DEGUG, INFO, WARN

I still don't fully understand what you are using this for and how you are implementing it (I'm not using setLevel). Could you please provide an example and use case? It seems to me that it is easier/less confusing to just change the log level of specific entries or the global logger.

I assume that for using weatherStationUploader I shall copy (or even make a soft link) Community\WeatherStationUploader\automation\jsr223\community\weatherStationUploader.py to automation\jsr223\community\weatherStationUploader.py and I shall copy and paste everything from Community\WeatherStationUploader\automation\lib\python\configuration.py.example into my configuration file automation\lib\python\configuration.py.

Correct

If users shall copy custom scripts into the Core\automation\jsr223\community we need to add that folder to .gitignore. I'll do that.

That's a tricky one. Nobody should be putting anything into /community/ unless it came from the repo, and if they are changing it, then they should submit a PR (or create and issue). Anything personal, or modifications or personalizations to /community/ should go into /personal/. If we add /community/ to .gitignore, then changes from developers won't be picked up when we add/modify anything in /community/. Users should only copy over what they want from /Community/. This may only affect a setup where symlinks are being used to a local repo. Maybe not put it into .gitignore and suggest to not link /community/, but manually copy them over? Hmmm...

Please make use of the review comments, as it will make it easier to track them. This isolates the the specific topic and replies, and they will be hidden once resolved. Much easier to see what still needs attention. https://help.github.com/en/articles/commenting-on-a-pull-request

[besynnerlig]

I still don't fully understand what you are using this for and how you are implementing it (I'm not using setLevel). Could you please provide an example and use case? It seems to me that it is easier/less confusing to just change the log level of specific entries or the global logger.

Thanks. Setting the logging level in a rule (just for that specific rule) a very fast way to view debugging output from a script withouit having to change the log level of specific entries or the global logger.

I utilize it all the time. Below is an example rule:

@rule
class OnExportChange(object):
    """Do stuff by detecting power export status changes"""
    def getEventTriggers(self):
        return [
            ItemStateChangeTrigger('Exporting_Power', str(OPEN)),
            #StartupTrigger()
        ]

    def execute(self, modules, inputs):
        if not hasReloadFinished(True): return
        self.log.setLevel(INFO)
        self.log.debug(u'Now we are exporting power, we might want to announce that if it\s the first time today.')
        if (getItemValue('Exporting_Power', OPEN) == OPEN) and (getItemValue('First_Power_Export_Today', OFF) == OFF):
            postUpdate('First_Power_Export_Today', ON)
            playsound('katching.mp3')
        else:
            self.log.debug(u'Make no fuss, we\'ve been here earlier today')

addRule(OnExportChange())

Now, that's just a small rule. It doesn't currently log anything. Let's say I've implementet it some time ago and I go back to it to find out why its not working as expected. Then I just change INFO to DEBUG and all debug messages starts to show (just for that rule) without me having to open karaf or edit any other files. When I'm done, I just change it back to INFO again. It's a very convenient way for me to work.

But scope.UnDefType.NULL should work too (no need for additional imports). I'll look into this.

Thanks. I've started a review comment for that.

That's a tricky one. Nobody should be putting anything into /community/ unless it came from the repo, and if they are changing it, then they should submit a PR (or create and issue). Anything personal, or modifications or personalizations to /community/ should go into /personal/. If we add /community/ to .gitignore, then changes from developers won't be picked up when we add/modify anything in /community/. Users should only copy over what they want from /Community/. This may only affect a setup where symlinks are being used to a local repo. Maybe not put it into .gitignore and suggest to not link /community/, but manually copy them over? Hmmm...

I'll start a review comment for that too. EDIT: I cant do that because .gitignore is not among changed files.

Please make use of the review comments, as it will make it easier to track them. This isolates the the specific topic and replies, and they will be hidden once resolved. Much easier to see what still needs attention. https://help.github.com/en/articles/commenting-on-a-pull-request

Thanks for the hint. I'm still learning. I'll try to do that.

;)

[5iver]

I pushed some more cleanup!

But it will happen sooner or later and we don't want those files to be sent upstream.

I think we're missing each other :smile:. We want the ability to change the file in community in order to submit PRs for them. If that's not something you think you will be doing, then you might want your own gitignore, or exclude them in your .git/info/exclude file. Can we deal with this one separate from this PR, if we need to change something?

Maybe we should add a section in the documentation for the steps involved to get started using a community script?

Yes. I think I wrote one with the next round of updates that was waiting for the ESH reintegration to wrap up. I'll add it into lucid-migration.

I couldn't find this. Lets leave it for another PR.

[5iver]

Setting the logging level in a rule (just for that specific rule) a very fast way to view debugging output from a script withouit having to change the log level of specific entries or the global logger.

You'll like this one... setLevel can take a string for the logging level. :smile:

So, this should work for you...

self.log.setLevel('INFO')

I tested with Jython 2.7.1.patch2618, so it may not work with 2.7.0.

[besynnerlig]

Setting the logging level in a rule (just for that specific rule) a very fast way to view debugging output from a script withouit having to change the log level of specific entries or the global logger.

You'll like this one... setLevel can take a string for the logging level. 😄

So, this should work for you...

self.log.setLevel('INFO')

I tested with Jython 2.7.1.patch2618, so it may not work with 2.7.0.

Thanks.

[besynnerlig]

Tests are going well. Basic functionality is now achieved and I'll start working with ideAlarm tomorrow.

[besynnerlig]

I have a problem with that scripts in the jsr223/community folder are running before scripts in jsr223/core. Would you like to consider renaming core to 00core to prevent that? If so, please do not make any changes now until your PR is merged and synced.

[5iver]

I have a problem with that scripts in the jsr223/community folder are running before scripts in jsr223/core. Would you like to consider renaming core to 00core to prevent that?

This has been an embarrassment for me since the repo directory restructuring. I should have caught it before pushing those changes :roll_eyes:, but I have modified ScriptFileWatcher to remedy this. There is some background and detail in #76. OH 2.5 M2 will correct this, but what to do until then... and for people using previous versions of OH... :thinking:.

It's not pretty, but we have to do something about it. Since it only effects scripts, let's rename the /jsr223/core/ directory to 000_core, and then change it back in #94. My plan is to have a list of archived releases, so that people can select a version of the libraries that works with older versions of OH.

OH compatibility	Release	Notes
2.3 (up to 2.4 S1317)	0.1.0	Original libraries from Steve
2.4 (up to 2.5 M1/S1521)	1.0.0	Decorators, directory restructure, lucid reintegration
Current	2.0.0	Repo rename, ESH reintegration

If so, please do not make any changes now until your PR is merged and synced.

I'm not clear on what you mean by this. Changes to master or the lucid-migration branch in my fork? I'm thinking I should make this change in master right now and rebase my fork before the merge.

[besynnerlig]

I've been thinking (yeah...) and I'd like to finish the testing of ideAlarm and the other community stuff. When those seem to work I'd like to send a PR to your personal repo for review. After merging I will sync my repo again with yours and my testing phase 2 starts. Then I will upgrade my ordinary OH installation from 2.3 to OH 2.4, install openhab2-jython and convert all my personal lucid scripts to openhab2-jython. That will be the ultimate testing environment and I'll test 24/7. I'm prepared some things won't be working initially but it's a home, it's not a nuclear plant I'm running here (but I do export energy to the grid actually). What do you think about that? I will need one or two more days for phase 1.

[5iver]

I recommend not using 2.4. There was not much time between it and 2.5 M1, but there were a number of fixes. Especially MQTT and hueemulation. IIRC, there were also some fixes in openhab-core. As far as testing in your production environment, I say go for it. Keep a backup in case you need to rollback, but it needs to be done at some point! Are there others that we could get in on the testing?

I'd really like to get this wrapped up NLT the end of the weekend, to get moving on #94 :tada:.

[5iver]

@besynnerlig, I've made some more changes that will need your review and synced up with master to pick up the core.date fixes. I did not include the rename of the /automation/jsr223/core/. I will do that right after this PR merged, so just rename your core directory to 000_core for testing.

[besynnerlig]

@besynnerlig, I've made some more changes that will need your review and synced up with master to pick up the core.date fixes. I did not include the rename of the /automation/jsr223/core/. I will do that right after this PR merged, so just rename your core directory to 000_core for testing.

Thanks. I'll have a look at it tomorrow (monday morning).

[besynnerlig]

@besynnerlig, I've made some more changes that will need your review and synced up with master to pick up the core.date fixes. I did not include the rename of the /automation/jsr223/core/. I will do that right after this PR merged, so just rename your core directory to 000_core for testing.

My web repo branch is 23 commits ahead, 9 commits behind openhab-5iver:lucid-migration. I have no idea how to re-sync. I guess the easiest would be to discard my repo on my local machine and clone yours again. I think I'll do just that.

[5iver]

I guess the easiest would be to discard my repo on my local machine and clone yours again. I think I'll do just that.

That would do it, but this should work too (do this from inside the git directory of your clone)...

git checkout lucid-migration
git reset --hard origin/lucid-migration

[5iver]

When you feel that the time is right, please let me know and I'll make a more extensive test.

After those last two fixes, I think we are ready to merge this. If we find bugs after the merge, it will be a little more effort to update both branches, but not too much.

After the merge, I'll setup a branch to store the <=2.5M1 code, update master to >2.5M1, and get on with the repo renaming!

[5iver]

@besynnerlig, the last two fixes are merged. Are you ready to approve the PR?

[DONE] O wait... need to remove the gitignore entries for community.

[5iver]

The way the repo is currently setup, the person that created the PR can't approve their own PR, and an approval is required before it can be merged. Being an admin, I can do anything, but to be official about it, you should start another review and select 'approve changes'. :smile:

[besynnerlig]

The way the repo is currently setup, the person that created the PR can't approve their own PR, and an approval is required before it can be merged. Being an admin, I can do anything, but to be official about it, you should start another review and select 'approve changes'. 😄

I'm not sure. Am I approving changes in the wrong place?

[5iver]

No... you don't have write privileges... I'll need to rethink that. I'll waive the magic wand...