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.

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

Add v8 mockdata #6261

Merged
merged 3 commits into from
Dec 18, 2023
Merged

Add v8 mockdata #6261

merged 3 commits into from
Dec 18, 2023

Conversation

VIKTORVAV99
Copy link
Member

@VIKTORVAV99 VIKTORVAV99 commented Dec 18, 2023

Description

This adds v8 mock data, updates the v7 mock data and removes the v6 mock data that are no longer in use.

@github-actions github-actions bot added javascript Pull requests that update Javascript code python Pull requests that update Python code labels Dec 18, 2023
Copy link
Contributor

PR Analysis

  • 🎯 Main theme: Updating mock data to v8 and removing v6 mock data
  • 📝 PR summary: This PR updates the mock data from v6 to v8 in the project. It modifies the scripts to fetch and store data from v8 endpoints and removes the v6 data that is no longer in use. It also adds new v8 mock data files and updates existing ones.
  • 📌 Type of PR: Enhancement
  • 🧪 Relevant tests added: No
  • ⏱️ Estimated effort to review [1-5]: 2, because the changes are straightforward and mainly involve updating version numbers and file paths. However, the reviewer needs to understand the data structure and the impact of these changes on the overall system.
  • 🔒 Security concerns: No security concerns found

PR Feedback

  • 💡 General suggestions: The PR is well-structured and the changes are clear. It would be beneficial to add a brief explanation in the PR description about why this update is necessary and what impact it will have on the system. Also, it would be good to ensure that all parts of the system that rely on this mock data are updated to use the new v8 data.

  • 🤖 Code feedback:
    relevant filescripts/remove_zone.py
    suggestion      Consider using a constant or a configuration value for the API versions instead of hardcoding them. This will make future updates easier and less error-prone. [important]
    relevant linefor API_version in ["v7", "v8"]:

    relevant filemockserver/generate_mock_data.js
    suggestion      Similar to the previous suggestion, consider using a constant or a configuration value for the API version in the URL. [important]
    relevant lineconst CORE_URL = 'http://localhost:8001/v8';

How to use

Instructions

To invoke the PR-Agent, add a comment using one of the following commands:
/review: Request a review of your Pull Request.
/describe: Update the PR title and description based on the contents of the PR.
/improve [--extended]: Suggest code improvements. Extended mode provides a higher quality feedback.
/ask <QUESTION>: Ask a question about the PR.
/update_changelog: Update the changelog based on the PR's contents.
/add_docs: Generate docstring for new components introduced in the PR.
/generate_labels: Generate labels for the PR based on the PR's contents.
see the tools guide for more details.

To edit any configuration parameter from the configuration.toml, add --config_path=new_value.
For example: /review --pr_reviewer.extra_instructions="focus on the file: ..."
To list the possible configuration parameters, add a /config comment.

@@ -75,7 +75,7 @@ def remove_translations(zone_key: ZoneKey):


def remove_mockserver_data(zone_key: ZoneKey):
for API_version in ["v6", "v7"]:
for API_version in ["v7", "v8"]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Instead of hardcoding the API versions, consider using a variable that contains all the versions. This will make the code more maintainable.

Suggested change
for API_version in ["v7", "v8"]:
API_versions = ["v7", "v8"]
def remove_mockserver_data(zone_key: ZoneKey):
for API_version in API_versions:
for state_level in ["daily", "hourly", "monthly", "yearly"]:
try:
with JsonFilePatcher(

@@ -12,7 +12,7 @@ const fetchAndStoreData = async (url, savePath) => {
return axios.get(url).then((res) => writeJSON(savePath, res.data));
};

const CORE_URL = 'http://localhost:8001/v6';
const CORE_URL = 'http://localhost:8001/v8';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Similar to the previous suggestion, consider using a variable for the API version in the URL. This will make the code more maintainable and easier to update in the future.

Suggested change
const CORE_URL = 'http://localhost:8001/v8';
const API_VERSION = 'v8';
const CORE_URL = `http://localhost:8001/${API_VERSION}`;
const timeAggregates = ['hourly', 'daily', 'monthly', 'yearly'];
const detailsZones = ['DE', 'DK-DK2'];

Comment on lines 26 to 30
detailsZones.forEach(async (zoneId) => {
await fetchAndStoreData(
`${CORE_URL}/details/${agg}/${zoneId}`,
`./public/v7/details/${agg}/${zoneId}.json`
`./public/v8/details/${agg}/${zoneId}.json`
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: The 'fetchAndStoreData' function is called inside a forEach loop. This could lead to unexpected behavior due to the asynchronous nature of JavaScript. Consider using 'Promise.all' to handle these asynchronous operations.

Suggested change
detailsZones.forEach(async (zoneId) => {
await fetchAndStoreData(
`${CORE_URL}/details/${agg}/${zoneId}`,
`./public/v7/details/${agg}/${zoneId}.json`
`./public/v8/details/${agg}/${zoneId}.json`
);
await Promise.all(detailsZones.map(async (zoneId) => {
await fetchAndStoreData(
`${CORE_URL}/details/${agg}/${zoneId}`,
`./public/v8/details/${agg}/${zoneId}.json`
);
}));

@VIKTORVAV99
Copy link
Member Author

This is in case I get bored in the middle of the woods and want to work with no internet connection 😅

Copy link
Member

@madsnedergaard madsnedergaard left a comment

Choose a reason for hiding this comment

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

LGTM :)

@VIKTORVAV99 VIKTORVAV99 enabled auto-merge (squash) December 18, 2023 19:41
@VIKTORVAV99 VIKTORVAV99 merged commit 0ba224d into master Dec 18, 2023
12 checks passed
@VIKTORVAV99 VIKTORVAV99 deleted the vik/v8_mockdata branch December 18, 2023 19:41
@tonypls
Copy link
Collaborator

tonypls commented Dec 20, 2023

It seems this PR may have impacted the Cypress tests on master, as there's an issue with the zone details API not loading the expected data.

@VIKTORVAV99
Copy link
Member Author

It seems this PR may have impacted the Cypress tests on master, as there's an issue with the zone details API not loading the expected data.

I'll take care of it!

@madsnedergaard
Copy link
Member

Any news on this, @VIKTORVAV99? This PR introduced different dates for the state and the details files (2022 vs 2023) :)
Also happy to help out if you're busy

@VIKTORVAV99
Copy link
Member Author

VIKTORVAV99 commented Jan 5, 2024

Any news on this, @VIKTORVAV99? This PR introduced different dates for the state and the details files (2022 vs 2023) :) Also happy to help out if you're busy

I thought it would be a quick fix, but for some unknown reason the details and state files are out of sync by days by the look of it. And when I tired to update them again I got the same issue.

I'll see if I can download some new data later today and hopefully they will be in sync...

This works in the v8 pr though 😅

@madsnedergaard
Copy link
Member

I just realised you also fixed this in the v8 branch, so I'm wondering if we should split out the mockserver/cy-testing part from that PR or try to get the whole thing reviewed and merged soon? :)

@VIKTORVAV99
Copy link
Member Author

I just realised you also fixed this in the v8 branch, so I'm wondering if we should split out the mockserver/cy-testing part from that PR or try to get the whole thing reviewed and merged soon? :)

They are kind of tied hand in hand as the tests should reflect that the app is calling a new endpoint. But I have been testing the preview branch a bunch and so far I have not found any issues besides those I solved yesterday. So we could just go forward with that.

But I had hoped we could sneak in a fix for the GFS data structure being different between the endpoints but I suppose we could do that later on as it calls another endpoint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javascript Pull requests that update Javascript code python Pull requests that update Python code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants