Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Obvius2020 #520

Merged
merged 23 commits into from
Feb 23, 2021
Merged

Obvius2020 #520

merged 23 commits into from
Feb 23, 2021

Conversation

lindavin
Copy link
Contributor

@lindavin lindavin commented Dec 2, 2020 •
edited by huss
Loading

This PR brings in the previously done Obvius2020 pipeline, with the added work of properly creating meters from a .ini configuration file.

Fixes #163.

Note merging this changes the database configuration. Note merging this changes the node modules.

lindavin and others added 15 commits November 7, 2020 10:18
@lindavin
Add test for obvius config file processing
f73b486
@lindavin
Wrap config file processing into a function
080e74f
@lindavin
Add returns param
df95dfb
@huss
fix obvius bug & upgrade pg-promise
4781aef 
The upgrade of pg-promise is needed per discussion in a recent PR.
This does not completely fix the issues but allows current developers to
work with this. This will be completely resolved once PR is merged into
this branch (after approved).

The upgrade found a mistake I made in DB usage. You should not be doing
a dropConnection() as this does a stopDB() that invalidates DB
connection. pg-promise now detects and errors in this case.
Thus, they were removed.
@huss
add better obvius test data
d2e4d85 
The new test files have several days of data that are hourly.
This means OED can graph the data after refreshing views because
it has multiple days. The switch to hourly data fixes a temporary issue
that the the code assumes each point spans one hour (this needs to be
changed as we merge in the new pipeline code).

It is recommended to look at meter  number 4 as it has the best data.
To see the data reasonably, you need to move the right endpoint and
redraw (we need to look into this issue elsewhere as not related to
obvius). You will lose one day of the three days due to how OED
currently graphs data. Finally, you need to be logged in as admin to see
the meters since display is disabled when obvius meters are added.
You can change this for desired meter(s) on the meter page as admin.
@lindavin
Merge remote-tracking branch 'upstream/obvius2020' into obvius2020
a5abd60
@lindavin
Replace .param with .body
e5eeea9
@lindavin
Log warning when an obvius meter is being created from the data file
174e9ed
@lindavin
Allow way to create obvius meters from uploaded config file
7d17122
@lindavin
Revert to using the generic parameter pipeline
2a3d6b8
@lindavin
Remove the first three items
430b56d 
We expect that the first three items will just be arrays whose values
are 0.
@lindavin
Fix linting errors
44e4809
@lindavin
Replace info with proper warn method
4068487
@lindavin
Add license to the top of the file
afb53b5
@lindavin
Escape html to prevent xss attacks
4644d91
@huss huss mentioned this pull request Dec 10, 2020
Closed
@huss
Copy link
Member

huss commented Jan 23, 2021

This PR works fine. I want to thank @NoraCodes for important earlier work on the code in this pull request (and @lindavin who has done the recent work). I also note that @mm413 worked on this a little before.

I have merged in the current development. I made a couple of other edits while I did that but they should be minor. There are some issue(s) left that I hope will be resolved when earlier submitted but pending PRs are in development and merged here (waiting to do that). One is that Meter.js mapRow is missing identifier. I will do another round of full testing at that time.

Here are comments on code not changed in the PR that I cannot enter in that file/line:

  • src/server/models/obvius/Configfile.js on line 85 has a comment marking the return value uncertain. Is there actually a return value?
  • If you upload a config file twice it causes an error of rejected promise. I think an OED error would be best if the meter already exists, etc.
  • There are 7 failing tests on my local machine. The first one ("Read Mamac log from a file:
    rolls back correctly when it rejects:") is likely only on my machine due to a local postgres issue but wanted to know if others see it. The others seem related to obvius work. Can @lindavin look at these? I can help as needed.

huss
huss requested changes Jan 23, 2021
View reviewed changes
Copy link
Member

@huss huss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few files need updating and the general comment I left should be addressed.

src/server/models/obvius/processConfigFile.js Outdated Show resolved Hide resolved
src/server/models/obvius/processConfigFile.js Show resolved Hide resolved
src/server/routes/obvius.js Outdated Show resolved Hide resolved
src/server/test/web/obviusConfig.js Outdated Show resolved Hide resolved
@huss
Copy link
Member

huss commented Jan 23, 2021

I want to note that a few items will need to be addressed but are outside this PR:

  • The code assumes every reading is 1 hour. We have had discussions on how to get the correct time for a reading when the start & end times are both not provided. Note this is more complex for the last reading and esp. if there is only one reading provided (as may happen with obvius).
  • This code needs to be tried on a live server getting real Obvius data to confirm it works correctly.
  • We are working on enhancing the types of users allowed. Once this is done then the password assumed will be removed.

huss added 5 commits February 22, 2021 12:10
@huss
Merge branch 'development' of https://github.com/OpenEnergyDashboard/OED
610f210 
 into pr/lindavin/520
@huss
fix commenting mentioned in PR
8e1d3b5
@huss
fix tests
fb74e08 
Since the original code was written, we figured out that the first
three columns following the date/time in the CSV file are ignored.
The tests failed because they did not assume this and the current code
does. Test data modified so now correct.
@huss
fix meter creation
897d674 
The new timezone for meters needed to be added as a null argument
to the meter creation.
Note that obviusConfig.js in testing has the same issue but it is not
working in other ways so it should be fixed later (if kept).
@huss
remove test that not automated
49fa259
@huss huss merged commit 0634112 into OpenEnergyDashboard:development Feb 23, 2021
@lindavin lindavin deleted the obvius2020 branch February 5, 2022 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@huss huss huss approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Read non-Mamac meter data via Obvius
2 participants
@lindavin @huss