[refactor] Make functions in utils2.storages us the Project instance, not only the project_id
#1078
Conversation
|
Task linked: QF-2760 Keep the file metadata in the database |
suricactus
changed the title
utils2.storages us the Project instance, not only the project_idutils2.storages us the Project instance, not only the project_id
suricactus
force-pushed
the
pass_project_not_only_id
branch
from
bf28708 to
de647dd
Compare
| @@ -591,7 +602,8 @@ def delete_project_file_version_permanently( | |||
| return versions_to_delete | |||
|
|
|||
|
|
|||
| def get_stored_package_ids(project_id: str) -> set[str]: | |||
| def get_stored_package_ids(project: qfieldcloud.core.models.Project) -> set[str]: | |||
| project_id = project.id | |||
There was a problem hiding this comment.
I use the project_id = str(project.id) to minimize the diff.
These funcions will be gone in 3 months or so anyways.
There was a problem hiding this comment.
Is there any added value to add a warning about this deprecation ?
There was a problem hiding this comment.
There is, check the #1065 PR.
There was a problem hiding this comment.
The current PR is not proposing the deprecation yet. But in the PR that actually introduces the deprecation I have added a deprecation warning.
…ly the `project_id` This simplifies some of the future code that is coming in QF-2760.
suricactus
force-pushed
the
pass_project_not_only_id
branch
from
de647dd to
d76ec69
Compare
| @@ -526,7 +536,8 @@ def delete_project_file_version_permanently( | |||
| Returns: | |||
| int: the number of versions deleted | |||
| """ | |||
| file = qfieldcloud.core.utils.get_project_file_with_versions(project.id, filename) | |||
| project_id = str(project.id) | |||
| file = qfieldcloud.core.utils.get_project_file_with_versions(project_id, filename) | |||
|
|
|||
| if not file: | |||
| raise Exception( | |||
There was a problem hiding this comment.
Why not adding a specific exception such as
raise FileNotFoundError(f"File '{filename}' not found for project '{project_id}'
There was a problem hiding this comment.
We did not change this line in this PR, is it a must to change it here?
| @@ -431,7 +430,7 @@ def after_docker_run(self) -> None: | |||
| if package_id in job_ids: | |||
There was a problem hiding this comment.
this can be moved to the if above:
if package_id ==str(self.job.project.last_package_job_id) or package_id in job_ids:
There was a problem hiding this comment.
This is not a change of this specific PR, is it a must to apply this change?
Also, I would argue that small individual ifs are much more readable than a bunch of conditions in one place.
| @@ -408,8 +408,7 @@ def after_docker_run(self) -> None: | |||
| ) | |||
|
|
|||
| try: | |||
| project_id = str(self.job.project.id) | |||
| package_ids = storage.get_stored_package_ids(project_id) | |||
| package_ids = storage.get_stored_package_ids(self.job.project) | |||
| job_ids = [ | |||
There was a problem hiding this comment.
we can use sets and and values_lists with flat to make it more efficient
There was a problem hiding this comment.
It is not clear to me what you mean by "make it more efficient" by using sets?
I would suggest that we don't change too much on these functions as they were serving the codebase well so far.
What I see as potential improvement here is indeed using value_list("id", flat=True), but this is out of scope of this PR. Personally I would prefer to keep it as it is. But I will also accept a follow-up PR that changes things in the proposed way.
There was a problem hiding this comment.
Hi, @tdrivas, thanks for the review!
Do you see some very scary things in the surrounding code that is not part of this patch? Most of your comments here are about code that is not part of this PR, so I would propose you add the respective tasks in the backlog and we just move on with the changes that are actually introduced in this PR.
This simplifies some of the future code that is coming in QF-2760.