OpenEnergyPlatform / oeplatform

Repository for the code of the Open Energy Platform (OEP) website. The OEP provides an interface to the Open Energy Family
http://openenergyplatform.org/
GNU Affero General Public License v3.0
61 stars 19 forks source link

Oat #1091

Closed adelmemariani closed 1 year ago

jh-RLI commented 1 year ago

Hi @adelmemariani Thanks for your work - I don't know why there are two new migrations in dataedit, you remove fields and then add them back. image

Also, this PR again includes all the work for the OAT and your work on the factsheet. But if you think you can't implement the changes independently, then just go ahead with this PR. I think at this point we could have implemented the code from scratch and it would have been easier for you :D

I still think you've added too many changes (stuff that is already in dev and will be overwritten by this PR) and I'm still not sure if you'll overwrite existing code, but we've discussed this several times - I'll try to help you with the CI error, but I don't have the time to make sure this PR doesn't break any of the existing code.

jh-RLI commented 1 year ago

One more thing - I know this feels like a pain in the ***, but please use our contribution guidelines - branches should be called feature/branch-name#RelatedIssueNumber. we work as a team and you are part of the team :) so it's better to keep some structure to make our workflow more reliable and transparent for others - branch name is a small part but quite important

jh-RLI commented 1 year ago

I found the reason for the CI error, I updated the omi compiler because it inserted unexpected data in the metadata string. Since this was just released this error was not noticed yet :) . I will try to fix it - It is triggered because the test that fails is trying to compile the metadata json object with a minimum of keys (only the "id" key is present) - you can ignore this for now, I will fix it in OMI. Please have a look at my comments above.

adelmemariani commented 1 year ago

Also, this PR again includes all the work for the OAT and your work on the factsheet.

As it is shown and communicated via emails, I removed the factsheet from the code and the PR does not include the factsheet:

remove factsheet code 8fb36dd

but the previous commit messages are still shown. The commit to removing them is also there.

adelmemariani commented 1 year ago

I still think you've added too many changes (stuff that is already in dev and will be overwritten by this PR)

The PR only contains the changes for OAT. I just pulled from DEV and checkout a new branch called OAT and added the codes for OAT.

adelmemariani commented 1 year ago

It is triggered because the test that fails is trying to compile the metadata json object with a minimum of keys (only the "id" key is present)

This prevented me from merging my branch and it is not related to the new code and other highlighted issues, I explained it via email.

jh-RLI commented 1 year ago

I never blamed you for the CI error, but that was not the main reason blocking the merge. I spent some time reviewing this and it contains several things that we should not include in the codebase.

jh-RLI commented 1 year ago

The PR only contains the changes for OAT. I just pulled from DEV and checkout a new branch called OAT and added the codes for OAT.

If thats the case why are these changes included in this PR: image

adelmemariani commented 1 year ago

If thats the case why are these changes included in this PR:

Those are compiled js files for the oeo_viewer. Each release has newly compiled files. These files are the result of running the npm build command of the oeo-viewer. The .gitignore contains 'static', and I have no idea why the 'static' is not ignored. I am currently a bit occupied by other tasks and I can not investigate more. I think we are good to go as all tests are now successful. I would suggest merging the PR.

jh-RLI commented 1 year ago

Oaky I will merge the PR soon - just removing your prints and some other things. I'm just saying that the next PR will have to fix the mistakes here - please work on this as soon as your workload is not as it is at the moment :) . And please no more big PR in the future, small prs with eraly PR so others can have a look - qick and dirty is not a good practice to maintain a project in the long term.

jh-RLI commented 1 year ago

@adelmemariani can you tell me if this "CORS_ORIGIN_WHITELIST" is needed? you put it in the settings.py

jh-RLI commented 1 year ago

@adelmemariani i cant test this on my local steup :/ or is it not working ATM?

image

adelmemariani commented 1 year ago

As you are running it locally, is the leop server running on your local machine too? the image shows a request from your local host. look at the code here:

https://github.com/OpenEnergyPlatform/oeplatform/blob/oat/api/views.py#L1183 the API requests from the loep server. The production server will communicate with loep server. In my local machine which is in the same network as the loep server, things are working.

jh-RLI commented 1 year ago

Yes that's what I thought :) as long as all runs with you it's good. I don't have the loep running locally - thought maybe my local oep communicates with the loep in magdeburg.