emoncms / app

Emoncms App module: application specific dashboards: includes myelectric, mysolarpv, myheatpump and solar + wind app.
GNU Affero General Public License v3.0
27 stars 71 forks source link

Make the apps module more modular #48

Closed pb66 closed 6 years ago

pb66 commented 6 years ago

See the https://community.openenergymonitor.org/t/copy-users-on-private-server-including-dashboard-inputs-feeds-graphs-etc/6673?u=pb66 forum thread.

It would be useful if the apps module allowed "drop-in" app template files (without them needing to be explicitly listed elsewhere in the code) and that the templates could be sub-foldered so that users can maintain (and share?) their own folder or repo of templates without this app module becoming bloated with many different personal apps..

greeebs commented 6 years ago

@pb66 Do you have any thoughts on where the "personal" apps go? Something like /var/www/emoncms/Modules/app/personal? Or /home/pi/emoncms_apps? Something else?

I was thinking about this tonight and it doesn't seem like it would be a lot of work to rewrite "available_apps.php" to do a folder scan of "apps" plus one other location and generate the existing static "available_apps" array dynamically instead.

Then there's only a few lines to change in app_controller.php so it can find the files in more than one folder.

I might actually look at starting this even without a second location...

pb66 commented 6 years ago

My thoughts were that simply scanning the apps folder to create a list of apps in the format of subfolder:name would allow a user to just drop in thier own folder of any name and we can also swap folders or maintain specialist app collection repos, a app collection repo can just be cloned into the apps folder.

This is already in service in the device module and could also be used in the post-process or for dash visualisations etc. I only mention this as I think early concideration of a scalable solution could be benificial, for example if all modules had a "sub-modules" folder, a emoncms updater script could also visit each modules "sub-module" folder and update each folder that has a .git file. This is only possible if all modules use the same sub-folder name and path (static eg "Modules/app/sub-module" or dynamic eg "Modules/app/apps" and "Modules/device/devices"). Using a consistent path/name like that will also ensure that the content ends up where it should be eg the postprocess sub-modules outside the www path.

We can also add a .gitignore regex so that only the OEM/emon folder(s) get uploaded when doing PR's, users can simply move thier app from thier personal folder to the appropriate (oem?) folder when they are ready to share.

greeebs commented 6 years ago

I've half re-written available_apps.php to scan the "apps" subfolder for .xml files. The xml files I've created each basically have a single "app" object with a title and description property (and an optional status property which defaults to "Development"). These match one for one with the .php files and correspond to the array properties from the current available_apps array. The idea being if you want to add a new app, you drop your .php and a corresponding .xml file into the existing "Module/app/apps" folder and it'll get loaded (assuming you didn't stuff up the file format etc.)

greeebs commented 6 years ago

@pb66 I've just pushed a "modular" branch to my fork - is that the sort of thing you had in mind?

adminde commented 6 years ago

I totally agree, that would be a great enhancement of the module and I like your first suggestion a lot @greeebs, I'd like to point out though, that the app module already is very inconsistent with all the emoncms code in terms of naming-schemes and module structure, hence I'd strongly discourage the use of xml files for app meta data...

Further, maybe a modularization through directories could be nice?

--app
  |--Apps
  |  |--myelectric
  |    |--view.php
  |    |--app.json

With app.json containing the metadata similar to the already existing module.json files in each module:

{
    "title"          : "My Electric",
    "description"    : "A simple electricity consumption app showing real-time power in Watts and daily consumption in kWh. Switch between energy and cost modes.",
    "status"         : ""
}

This way, developers could structure their own emoncms extensions like

--web
  |--Apps
  |  |--foo
  |--Modules
  |  |--bar
  |--Theme
  |  |--test

and simply softlink them to their supposed place, to improve e.g. maintainability for separate git projects

greeebs commented 6 years ago

As it happens, I've already started converting those xml files into .json files in my working directory :) The folders seemed a little overkill for two files, but I see your thought about using symlinks.

I'll reorganise things a bit and then push another commit for comment... Changing to a folder structure will require modifications to app_controller.php as well.

greeebs commented 6 years ago

So that didn't turn out to be as difficult as I thought it might be... The folder structure looks like this:

Modules/app
   |--apps
   |   |--myelectric
   |   |   |--myelectric.php
   |   |   |--app.json
   |   |--timeofuse
   |   |   |--timeofuse.php
   |   |   |--app.json

Changes can be seen here Turns out I only needed to add a single line to app_controller.php :)

pb66 commented 6 years ago

Sorry, I've been short on time, but I do have a couple of comments.

JSON would indeed be better and more consistent than XML.

Using a folder to wrap each app is good, I thought the single file concept was a bit limiting, this way we could include single app specific css and javascript files without bloating the app module.

I still think there needs to be a "user/organisation" level of sub-foldering, all the existing apps should be in "apps/oem" or something similar, likewise my personal apps would reside in apps/pb66 etc, that way (for example) if any 3rd parties want to create drop-in modules eg a heatpump manufacturer might make a collection apps to report different types of heatpump eg showing a animated air-source verses ground source heatpump diagrams etc.

This method "sub-foldering" would pave the way for automated updates and simplify PR submission by .gitignore rules.

I have pondered on my previous point about the main sub-folder naming and it cannot be dynamic as I suggested

This is only possible if all modules use the same sub-folder name and path (static eg "Modules/app/sub-module" or dynamic eg "Modules/app/apps" and "Modules/device/devices").

this is because sometimes it is easier to run a second "dev" copy of a module eg cloning the app repo a second time in the emoncms/Modules folder and calling it "apps-dev" will allow you to run a stock version at http://server.com/emoncms/app and a dev version at http://server.com/emoncms/apps-dev, BUT an auto-updater script would not find the apps folders in Modules/apps-dev/apps-devs as the folder is named "apps". So a consistent folder name that can be used in all modules is prefered eg "emoncms/Modules/app/submodules", "emoncms/Modules/apps-dev/submodules" or even "emoncms/Modules/device/submodules" (not device/data) etc.

This way, developers could structure their own emoncms extensions like

True, and this would still work with the suggestion above, if users/devs prefered that method, although I think my point about another "user/organisation" level of folders is essential for even this suggestion because you can either symlink the whole "apps" folder into "app/subfolder" as "app/subfolder/myapps/foo" or you would need to symlink each and every app folder contained in the users "app" folder individually to get "Module/app/subfolder/foo" and the .gitignore regex is going to be complex and unlikely to work, whereas it is very easy to exclude all except "subfolder/oem/*".

adminde commented 6 years ago

I actually started yesterday evening to clean the app module up a little based on your fork 😄

Now I had some time on my hands to address @pb66 ideas... of which I just implemented most of and started a PR here

I'm just a little confused regarding

it is easier to run a second "dev" copy of a module eg cloning the app repo a second time in the emoncms/Modules folder and calling it "apps-dev" will allow you to run a stock version at http://server.com/emoncms/app and a dev version at http://server.com/emoncms/apps-dev, BUT an auto-updater script would not find the apps folders in Modules/apps-dev/apps-devs as the folder is named "apps".

As to my understanding, emoncms is looking for <module_name>_controller.php e.g. to handle request routes. Hence a renamed app_dev/app_controller.php would never be found and would not work?
I added the optional setting to change the whole App dir in the PR e.g. to *emoncms/Apps/ or whatever you like though^^

pb66 commented 6 years ago

As to my understanding, emoncms is looking for _controller.php e.g. to handle request routes. Hence a renamed app_dev/app_controller.php would never be found and would not work?

That makes total sense, maybe I'm mistaken, I have on many occasions duplicated modules in the Modules folder (with a similar name eg app2 etc) for the purpose of dev'ing, perhaps i have never tried running the second instance, I do know this approach works with duplicating emoncms installs, but your right, it shouldn't work with Modules (although I'm not able to test right now) so scrap that, But none the less it maybe simpler to update/upgrade and install (even if just using symlinks) if the folder of the same name can be found in all modules (that have submodules) but either way it needs to be consistent. So if it's "app/apps" it also needs to be "device/devices" not "device/data" likewise for postprocess/postprocesss, which isn't so good as just adding an "s" to the root folder (Module) name results in "postprocesss", Therefore we really need to perhaps just reuse the module name "app/app" or use a generic static name like "submodules", "templates" or something else, I prefer "submodules" because it works for all modules and it tells us what exactly is in that folder.

As a sidenote on this topic of module structure and naming, I also think that where a module is not installed in the www path eg postprocess, sync and emailreports, there should be a "www" folder rather than randomly named folders as we currently have.

When it comes to support it makes things so much easier, eg

"if a module has a www folder it must be installed outside the www path and that www folder then symlinked into emoncms/Modules"

and

"if a module has a submodules folder, it means you can add your own or share other users files by just dropping or cloning a shared submodule folder/repo into the submodules folder (or install elsewhere and symlink it there)."

With regards to

I added the optional setting to change the whole App dir in the PR e.g. to *emoncms/Apps/ or whatever you like though^^

I couldn't pick out that code to confirm what you mean there, but on reflection after the discussion above, we already have a mechanism to fudge the declared Modules name in modules.json, I think having a potential 3 names the the exact same module is asking for trouble.

Also, I'm really not keen on the idea of having a app_settings.default.php file, the existing setting.php is tricky enough to manage/upgrade, we do not want to expand that awkwardness to each module, the blocking of git pulls and/or having to redo the local file after each update of the defaults is a really bad idea IMO.

Another small point, "OpenEnergyMonitor" is a lot of typing, especially if you forget to CamelCase it and have to type it twice, lets please stick to "oem" all lower case, we all know that's OpenEnergyMonitor in this context, it just seems to make the path longer than is needed.

adminde commented 6 years ago

I couldn't pick out that code to confirm what you mean there, but on reflection after the discussion above, we already have a mechanism to fudge the declared Modules name in modules.json, I think having a potential 3 names the the exact same module is asking for trouble.

its in the first lines of the new function AppConfig->get_available(), where the root dir to resursively look for app.json files is defined. The option is commented by default and the hardcoded Modules/app/Apps dir is used. I just thought it could be nice to be able to optionally mirror e.g. the Apps, Modules, Theme approach for custom emoncms extension packs to reduce the depth at the moment to link to Modules/app/Apps. Which would only be practical if you plan to not use any of the core apps but who knows^^

the existing setting.php is tricky enough to manage/upgrade, we do not want to expand that awkwardness to each module, the blocking of git pulls and/or having to redo the local file after each update of the defaults is a really bad idea IMO.

I actually never had any problems with that, as the settings.php file is ignored by git but having an optional $app_settings in the main settings.php similar to the global $feed_settings setting would be just as good for me and I could change this in 1 minute if preferred.

At last I actually do not understand why you would ever type OpenEnergyMonitor 😄 the app ids are just the basename e.g. myelectric. I just tried to mirror the device/data/OpenEnergyMonitor in the device module as originally added I think by @TrystanLea. I really dont care to be honest, I just tried to be consistend.

greeebs commented 6 years ago

Another small point, "OpenEnergyMonitor" is a lot of typing, especially if you forget to CamelCase it and have to type it twice, lets please stick to "oem" all lower case, we all know that's OpenEnergyMonitor in this context, it just seems to make the path longer than is needed.

I'm with @adminde on this one "why you would ever type OpenEnergyMonitor"... surely you're just typing "O[tab]" if you ever need to go in there in the shell? Mind you, oem (the "original equipment manufacturer") is actually OpenEnergyMonitor in this case, so both names work equally well even if you don't twig to the abbreviation being for open energy monitor! Consistency is good though if it has already been used elsewhere.

I added one comment to the PR about checking for poorly formed json files too...

pb66 commented 6 years ago

I just tried to mirror the device/data/OpenEnergyMonitor in the device module as originally added I think by @TrystanLea. I really dont care to be honest, I just tried to be consistent.

Perhaps you're right? I was just thinking from a slow typer's perspective, but if the submodules are to be listed in groups eg like the device module then perhaps it is better longhand? I'm not overly fussed either, but yes I to would like it to be consistent.

I actually never had any problems with that, as the settings.php file is ignored by git

Nor do I personally, but I do end up helping many users on the forum that do get it wrong. Since I understand why and how, it's not a problem for me, but it is the one and only software I know of that makes is a complex as it is.

Which would only be practical if you plan to not use any of the core apps but who knows^^

Exactly, it only makes a minor improvement for a very small group of users, the downside is it makes it twice as hard to re-introduce the additional level to add other groups of apps and it adds confusion if some users have a different number of levels to other users, it should be the same for everyone and if a super-user wants to customise their setup to use only their own apps, I'm sure they would have the skills to do that, it doesn't need to be a standard option/variation.

greeebs commented 6 years ago

Exactly, it only makes a minor improvement for a very small group of users, the downside is it makes it twice as hard to re-introduce the additional level to add other groups of apps and it adds confusion if some users have a different number of levels to other users, it should be the same for everyone and if a super-user wants to customise their setup to use only their own apps, I'm sure they would have the skills to do that, it doesn't need to be a standard option/variation

👍

adminde commented 6 years ago

I'm already completely convinced^^

I'll remove the app_settings.php file then and introduce the optional global setting in the root settings.php:

    $app_settings = array(
        // Whitelisted Apps. Only uncommented app ids will be shown as available.
        // Available Apps will be shown in order as defined here.
        $applist = array(
            'myelectric',
            'mysolarpv',
            'mysolarpvdivert',
            'myenergy',
            //'myelectric2',
            //'timeofuse',
            //'timeofuse2',
            //'timeofusecl',
            //'costcomparison',
            //'myheatpump',
            //'openevse'
        );
    );

I wont add it to the settings, but only mention this possibility in the Readme.
The Modules/app/Apps app directory will be hardcoded in app_module.php

I suggest you add some error checking for poorly formed .json files (with a missing title or description primarily) - its a straightforward check that I added to my fork last night... it should go around line 46 of the new app_model.php...

I was not able to find your commit in your fork @greeebs sadly, but I'll take some time this afternoon to implement all of this :) shouldnt be a lot of effort And thanks again for your input

pb66 commented 6 years ago

The Modules/app/Apps app directory will be hardcoded in app_module.php

??? So will the postprocesses be in a folder Modules/postprocess/Postprocesss with 3 s's at the end?

And why is it Capitalized? Surely that just paves the way to frustration due to typo's.

I really think it needs to either repeat the repo name app/app (no s) or preferably use a generic name eg "submodules".

I'll remove the app_settings.php file then and introduce the optional global setting in the root settings.php:

I do not see a need for this setting, it is an unnecessary complication that has little or no value. All apps should always be shown, are we going to start adding lists of visualisations and input processes to restrict what the user see's? How will this work if there are 2 MyElectric apps, one in the OpenEnergyMonitor folder and a modified one in my personal folder? I might want to use both.

Plus this blocks the "drop-in" feature which is the main point of all this, if a user has a list as you suggest, just "dropping in" an app will have no effect until they edit the list. If they add a whole new group of apps, they will have to add each one to the list to try them. I'm sorry but this is a no go for me.

I wont add it to the settings, but only mention this possibility in the Readme.

but as long as it's hidden and doesn't effect normal use I won't pursue this any further.

greeebs commented 6 years ago

I was not able to find your commit in your fork @greeebs sadly, but I'll take some time this afternoon to implement all of this :) shouldnt be a lot of effort

Here: https://github.com/greeebs/emoncms-app/blob/74b3e9f371220000e7cd9217c7406f0f078b2aff/available_apps.php#L16-L19

adminde commented 6 years ago

Thanks @greeebs

Oook @pb66 nooow I finally understand what you meant, sorry.
I was actually just talking about removing the app dir path from the settings file and hardcoding it. I totally missed the general discussion you tried to start here^^.

I agree, the uppercase Apps is not very nice. I bother myself quite a lot to be honest that Lib, Modules, Theme and Views is uppercase in all emoncms core modules, but nothing else... Hence I thought it being the "most consistently"-ish... But you're right, why not thinking of something that makes more sense and adjusting the device module as well, while we're at it 😉
I immediately agree for it to be preferrable to be lowercase... but isn't processes exactly what the postprocessing module does? Just plural. device/devices, postprocess/[post]processes and maybe app/apps ?

And regarding the whitelist: I will immediately use it and this was a feature I created for myself, as I manage quite the big battery of emoncms setups as smartgrid controllers in the field and would very much like to get rid of the timeofuse apps (as they simply do not apply for us in Germany) without deleting anything and blocking any further git updating.

How will this work if there are 2 MyElectric apps, one in the OpenEnergyMonitor folder and a modified one in my personal folder? I might want to use both.

This will not work eitherway, as currently the id of an app is defined by its dir and .php file. Any second myelectric app will immediately override the already existing. And I'm really not sure if there is any possibility for us to change this without completely breaking backwards compatibility

greeebs commented 6 years ago

I have one more comment on your latest commit... it applied to the previous one too. The folder scan is looking for *.json - what if someone develops an app which uses a .json file for its own data? That's a real possibility now they're in their own folders. We really don't want to be reading that file - I suggest locking down the name of the file you read so that it must be "app.json".

It also bothers me slightly that you're going to recurse infinitely deeply. I can see how it makes it flexible, but what if someone accidentally symlinks "/" into there? The "New App" page will never load...

TrystanLea commented 6 years ago

removed my comment, realised I wasnt looking at the right thing

adminde commented 6 years ago

The folder scan is looking for *.json - what if someone develops an app which uses a .json file for its own data?

you are absolutely right, I just changed that to

if(basename($file ,".json") == 'app') {
    ...
}

which should be less error prune.
I do not think the softlinking-/-to-the-apps-dir problem is a real issue here, as emoncms is not immune to stupidity eitherway. You could also link / to emoncms/Modules and so on...

Further, I realized thanks to @TrystanLea that it was idiotic to rename Modules/app/apps to an uppercase Apps, as git will not rename this when pulling... failing everything. So currently, we're back to the original 😉

greeebs commented 6 years ago

I do not think the softlinking-/-to-the-apps-dir problem is a real issue here, as emoncms is not immune to stupidity eitherway. You could also link / to emoncms/Modules and so on...

While that is absolutely true, why keep the problem going? 😉