sunportal icon indicating copy to clipboard operation
sunportal copied to clipboard

Several changes and new features

Open nfsprodriver opened this issue 5 years ago • 9 comments

This pull request may not be finished yet, so @philipptrenz please add your comments or contribute (in a new branch would be best).

Overview of changes in this pull request:

  • added a total chart (based on year chart calculations)
  • fixed capitalization for displayed strings
  • improved labels of data
  • walk through days (with existent data!) by clicking on bars
  • unified calculations of sums (SQL sums and python sums are unequal!)
  • optimized loading times by only refreshing month and day data (we may think about that?)
  • updated README.md according to these changes (we may mention more of these)

When some changes are missing, I'll add these to the changelog.

nfsprodriver avatar Aug 13 '19 11:08 nfsprodriver

Hey @nfsprodriver, many thanks for your contribution! Just tested it, it adds a lot of features I also wanted and it works great. Which additional features do you plan to implement?

philipptrenz avatar Aug 13 '19 14:08 philipptrenz

Moved your changes into nfsprodriver-dev branch

philipptrenz avatar Aug 13 '19 14:08 philipptrenz

As pylint failed, I took a look into util/database.py and I understand what you tried to achieve. Nevertheless, from my point of view the backend processing should be stateless, as there can be more than one client connected and the API should be REST-like.

So the frontend should specify the data it wants to update. This could be done within the JSON like so:

static/js/main.js, line 449:

var request_data = {
    date: currentDay
    requested_data: {
        day = true,
        month = true,
        year = false,
        years = false,
        total = false 
    }
};

Then the backend builds the response JSON only with the requested data.

OR

My preferred solution would be to have several API endpoints for the different data entities. So that the frontend no longer requests to /update, but instead to

GET /day
GET /month
GET /year
GET /total

For initial loading there could be a GET /all endpoint and afterwards only the necessary updates get requested (if needed with multiple queries).

philipptrenz avatar Aug 13 '19 14:08 philipptrenz

Thanks @philipptrenz for the fast answer. For now, I don't have any idea for more features, which are necessary in the PR, so I'd like to feature freeze it. Talking about the update thing your point is very good to move it out of the backend. I don't know much about REST-API than it may need some work. So for this PR I'll try to implement your JSON proposal. If there's anything else, which needs to be changed before merging, please tell me ;)

nfsprodriver avatar Aug 13 '19 16:08 nfsprodriver

Did you mean like this 8dca723c5a9282391fa80075ca79e6321aa3ce57 ?

nfsprodriver avatar Aug 13 '19 17:08 nfsprodriver

One little thing: In the total chart the first year is cut off a little bit. May we add the year before first data on X-axis or do you have a better idea?

nfsprodriver avatar Aug 13 '19 17:08 nfsprodriver

Found the following issues:

  • Pulled your latest changes and found, that your changes increases the loading time by a factor of 30. The current master branch loads within 130ms on my machine, with your changes it's around 4 seconds.
  • Your Python code is not PEP8 conform, you can check that by using a linting tool like pylint
  • Your global variables in util/database.py do not fit the object-oriented style of this class

One little thing: In the total chart the first year is cut off a little bit. May we add the year before first data on X-axis or do you have a better idea?

Yeah, this can be changed by manipulating the from and to values of the chart, like within the following lines:

// update scale
all.tot.interval.from = moment.unix(all.tot.interval.from).subtract('months', ???);
all.tot.interval.to = moment.unix(all.tot.interval.to).add('months', ???);
totChart.data.labels = [
    all.tot.interval.from,
    all.tot.interval.to
];

philipptrenz avatar Aug 14 '19 12:08 philipptrenz

About the conformidity, pylint does not only detects my lines as unconfirm. Can you explain, what you mean? I'm very new to this topic.

nfsprodriver avatar Aug 14 '19 18:08 nfsprodriver

This is a proposal, which you could have meant https://github.com/philipptrenz/sunportal/pull/2/commits/a6b54ddfc47b1196f00f886fbabed4dd8ae6fed6

nfsprodriver avatar Aug 14 '19 19:08 nfsprodriver