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

get site group if it exists #153

Open
peterdudfield opened this issue Sep 4, 2024 · 14 comments · Fixed by #156
Open

get site group if it exists #153

peterdudfield opened this issue Sep 4, 2024 · 14 comments · Fixed by #156
Assignees
Labels
good first issue Good for newcomers

Comments

@peterdudfield
Copy link
Contributor

Detailed Description

Currrently when making a new user, if it doesnt exsits, a new user and site group is made.
Sometimes the site group already exists, so we should laod that, not cause an error by overwriting it

Context

  • good to reduce errors

Possible Implementation

in the code here, try to get a site group with this email first, and if it doesnt exsits, then make it.
it would also be nice to use create_site_group function to reduce code

@peterdudfield peterdudfield added the good first issue Good for newcomers label Sep 4, 2024
@ProfessionalCaddie
Copy link
Contributor

Hello. Would you please assign this to me? I am trying to build my open source contributions!

@peterdudfield
Copy link
Contributor Author

Of course, thanks @ProfessionalCaddie

@ProfessionalCaddie
Copy link
Contributor

Hello @peterdudfield ,
I have learned a lot so far and I think I have a good test and solid logic but I am having trouble running tests.

I am assuming the issue is with poetry as I have had trouble installing certain packages inside the virtual environment(greenlet,numpy). So I started from the forked version(no changes)and changed Poetry's dependencies to be all the latest versions.

Now I can run the pytest command but it fails the tests/test_alembic.py. Do you have any way to move forward?

@peterdudfield
Copy link
Contributor Author

whats the error you get?

@ProfessionalCaddie
Copy link
Contributor

FAILED tests/test_alembic.py::test_migrations - docker.errors.DockerException: Error while fetching server API version: (2, 'CreateFile', 'The system cannot find the file specified.')

Is this because I haven't started a local database in docker?

@peterdudfield
Copy link
Contributor Author

you have to start up the docker engine locally. I do this by starting the docker desktop app

@ProfessionalCaddie
Copy link
Contributor

Hi Peter, I believe that I have fixed this issue, however I couldn't use the function create_site_group from user_and_site.py because importing it into user.py implements circular logic.

In this case I was thinking it may be beneficial to move all database operations to a different file so the API functions can all import the file to reference the operation functions. Let me know what you think.

@peterdudfield
Copy link
Contributor Author

what structure do you propose?

I dont quite understand the circular import, but perhaps havent look at the code enough?

@ProfessionalCaddie
Copy link
Contributor

The circular import happens when I try to import create_site_group from user_and_site.py, which is what I took "it would also be nice to use [create_site_group] function to reduce code" to mean.

The code breaks when I try to import create_site_group because in the source for that(user_and_site.py) it imports pvsite_datamodel.read which is where I have been working(user.py)

@peterdudfield
Copy link
Contributor Author

Ah I see. Try to do what make sense and then we can review. I do like the separation of read and write at the moment.

And note, api is one component that uses this, but there are a few other components that use this too. So I'd probably be against a pure api file.

@ProfessionalCaddie
Copy link
Contributor

Hi Peter.

I added a pull request but I wasn't sure how to have it show in this issue. Here is the link: #154

@peterdudfield peterdudfield mentioned this issue Sep 19, 2024
Merged
6 tasks
@peterdudfield peterdudfield reopened this Sep 19, 2024
@peterdudfield
Copy link
Contributor Author

The next step would be to use this new version in https://github.com/openclimatefix/india-api and https://github.com/openclimatefix/pv-site-api

@ProfessionalCaddie
Copy link
Contributor

@peterdudfield, does this mean to follow steps in readme for one or both of those projects and run tests? Does this entail anything else?

@peterdudfield
Copy link
Contributor Author

peterdudfield commented Sep 19, 2024

For India-api, it should be just increase the version here

For pv-site-api, increase the version here and also update the poetry lock file

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants