-
Notifications
You must be signed in to change notification settings - Fork 8
Added option to pull data from the Octopus Agile Outgoing API #10
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
base: dev
Are you sure you want to change the base?
Conversation
Fix number of outputs and rename node for identifiability
semicolons and closed brackets
Ctrl+C the brackets, don't control X...
Renamed because no longer need identifiability
I fat finger removed the m from msg3
different Outgoing check because everything is coming back as agile
octopus.js
Outdated
|
||
msg.payload.results.forEach(function(item, index) { | ||
msg3.payload.push([{ value_inc_vat : item.value_inc_vat, | ||
if (APIurl.includes("OUTGOING")) { |
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.
Why not just use the this.tariff == "OUTGOING"
comparison?
Even better is remove the if, and just set the value of source
dependent on the Tariff selected (removes repeated code).
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.
I tried this, but I think "this" doesn't refer to the node by this point in the function (and therefore this.tariff doesn't evaluate to anything)? The condition always evaluated to false. I couldn't think of a way to get the tariff value at that point in the code without just using the APIurl which I knew was there. I'm not massively familiar with js, so it may just be me not understanding.
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.
This was the final 2 commits, the penultimate one was verifying what I thought was happening (both outputs swapped to Agile), and the final commit let them evaluate to different things.
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.
Looking at this (and to be honest my JS is a little minimal) I wonder why I ever moved it from n.tariff
to this.tariff
. n.tariff
should be fine to use throughout. Try that.
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.
Looking at this (and to be honest my JS is a little minimal) I wonder why I ever moved it from
n.tariff
tothis.tariff
.n.tariff
should be fine to use throughout. Try that.
Would you prefer I change this.region as well, or just this.tariff?
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.
That was my point. If you set a variable for source
dependent on the tariff in use, you can use it without resorting to multiple 'if' statements and repeating code blocks.
On looking for maximum, at line 103, there is an array of blocks, of length specified, that I then look for the Will need some rewording of the variables to make sense. |
I have also realised the |
Documentation and code changes to add selection for min vs max block, and associated calcuation changes.
Not required
Fix misplaced identifier parameter
Fixed syntax
Clearly open to suggestions, I would like to be able to change the block calculation to find the max rather than min for the Outgoing tariff block, but am interested in your input.
Thanks