-
Notifications
You must be signed in to change notification settings - Fork 1
Enhancement/projects file upload #62
Changes from 34 commits
12830d5
81e3d3c
658dfd3
7dd5682
e42c57d
3467bb2
69df26e
e836f50
e26c015
fd941b2
073d6c5
fd2ae83
d675fe6
2298d8c
d0e9a10
6b3e733
a00cb99
f4aff02
4e0945f
05038c9
a142a88
1f11956
7fad876
ca42936
ed94408
8d75ae6
11377d1
bc6e4a9
62632be
6f323ec
b5f8ef9
72a9b7d
e1f79c7
12d0aa5
e1624b1
123992c
2c3a719
33880d6
10b0c1a
0edbd46
3438972
8892d98
c5e3bc4
5c6a28d
0be828f
adbb14b
f0be949
220856f
8c848d6
cb7eac1
6db6fad
058f53e
2314ff6
251ff29
f01fed9
d53eef1
dad7015
f4fe9fa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,9 +9,10 @@ | |
from flask import request | ||
from flask_restful import Resource | ||
|
||
from project.models.project import Project | ||
from project.utils.query_agent import query_by_id_from_model, delete_by_id_from_model, \ | ||
patch_by_id_from_model | ||
from project.models.project import Project | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the point of this change? You moved this down for no obvious reason. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
|
||
|
||
API_URL = getenv('API_HOST') | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,16 +1,34 @@ | ||
""" | ||
Module that implements the /projects endpoint of the API | ||
""" | ||
import os | ||
from os import getenv | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. either import os, or everything you need from os, not both. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
from urllib.parse import urljoin | ||
import zipfile | ||
|
||
from flask import request | ||
from flask_restful import Resource | ||
|
||
from project.models.project import Project | ||
from project.utils.query_agent import query_selected_from_model, insert_into_model | ||
from project.utils.query_agent import query_selected_from_model, create_model_instance | ||
|
||
from project.endpoints.projects.endpoint_parser import parse_project_params | ||
|
||
API_URL = getenv('API_HOST') | ||
UPLOAD_FOLDER = getenv('UPLOAD_URL') | ||
ALLOWED_EXTENSIONS = {'zip'} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aren't there multiple There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should indeed be possible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
|
||
|
||
|
||
|
||
def allowed_file(filename: str): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rename to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function is actually removable since it isn't used anywhere. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Than remove it 💀 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then* :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
""" | ||
check if file extension is allowed for upload | ||
""" | ||
return '.' in filename and \ | ||
filename.rsplit('.', 1)[1].lower() in ALLOWED_EXTENSIONS | ||
|
||
|
||
class ProjectsEndpoint(Resource): | ||
""" | ||
|
@@ -40,7 +58,40 @@ def post(self): | |
using flask_restfull parse lib | ||
""" | ||
|
||
return insert_into_model( | ||
Project,request.json, | ||
file = request.files["assignment_file"] | ||
project_json = parse_project_params() | ||
filename = file.filename.split("/")[-1] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you mean by this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't split by "/" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
# save the file that is given with the request | ||
|
||
new_project = create_model_instance( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't handle the case where this fails There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The only thing that fails is me 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
Project, | ||
project_json, | ||
urljoin(API_URL, "/projects"), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Change to f"{API_URL}/" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
"project_id") | ||
required_fields=[ | ||
"title", | ||
"descriptions", | ||
"course_id", | ||
"visible_for_students", | ||
"archieved"] | ||
) | ||
|
||
project_upload_directory = f"{UPLOAD_FOLDER}{new_project.project_id}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
file_location = "." + os.path.join(project_upload_directory) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove the "." it shouldn't be relative, we need to allow storage outside codebase. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
if not os.path.exists(project_upload_directory): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just use, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
os.makedirs(file_location, exist_ok=True) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you check if it exists if you set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
file.save(file_location + "/" + filename) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use path.join There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
try: | ||
with zipfile.ZipFile(file_location + "/" + filename) as upload_zip: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not solved eh There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
upload_zip.extractall(file_location) | ||
except zipfile.BadZipfile: | ||
return {"message": "Please provide a .zip file for uploading the instructions"}, 400 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. missing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
return { | ||
"message": "Project created succesfully", | ||
"data": new_project, | ||
"url": f"{API_URL}/projects/{new_project.project_id}" | ||
}, 201 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,6 +50,31 @@ def delete_by_id_from_model( | |
return {"error": "Something went wrong while deleting from the database.", | ||
"url": base_url}, 500 | ||
|
||
|
||
def create_model_instance(model: DeclarativeMeta, | ||
data: Dict[str, Union[str, int]], | ||
response_url_base: str, | ||
required_fields: List[str] = None): | ||
""" | ||
Create an instance of a model | ||
""" | ||
if required_fields is None: | ||
required_fields = [] | ||
# Check if all non-nullable fields are present in the data | ||
missing_fields = [field for field in required_fields if field not in data] | ||
|
||
if missing_fields: | ||
return {"error": f"Missing required fields: {', '.join(missing_fields)}", | ||
"url": response_url_base}, 400 | ||
|
||
filtered_data = filter_model_fields(model, data) | ||
new_instance: DeclarativeMeta = model(**filtered_data) | ||
db.session.add(new_instance) | ||
db.session.commit() | ||
|
||
return new_instance | ||
|
||
|
||
def insert_into_model(model: DeclarativeMeta, | ||
data: Dict[str, Union[str, int]], | ||
response_url_base: str, | ||
|
@@ -69,26 +94,21 @@ def insert_into_model(model: DeclarativeMeta, | |
a message indicating that something went wrong while inserting into the database. | ||
""" | ||
try: | ||
if required_fields is None: | ||
required_fields = [] | ||
# Check if all non-nullable fields are present in the data | ||
missing_fields = [field for field in required_fields if field not in data] | ||
|
||
if missing_fields: | ||
return {"error": f"Missing required fields: {', '.join(missing_fields)}", | ||
"url": response_url_base}, 400 | ||
|
||
filtered_data = filter_model_fields(model, data) | ||
new_instance: DeclarativeMeta = model(**filtered_data) | ||
db.session.add(new_instance) | ||
db.session.commit() | ||
return jsonify({ | ||
new_instance = create_model_instance(model, data, response_url_base, required_fields) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
# if its a tuple the model instance couldn't be created so it already | ||
# is the right format of error message and we just need to return | ||
if isinstance(new_instance, tuple): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make both cases return status code and check for the status code instead of instance, this isn't scalable There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
return new_instance | ||
|
||
return (jsonify({ | ||
"data": new_instance, | ||
"message": "Object created succesfully.", | ||
"url": urljoin( | ||
f"{response_url_base}/", | ||
str(getattr(new_instance, url_id_field)))}), 201 | ||
"url": | ||
urljoin(response_url_base + "/", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use f-string There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
str(getattr(new_instance, url_id_field)))}), | ||
201) | ||
except SQLAlchemyError: | ||
db.session.rollback() | ||
return jsonify({"error": "Something went wrong while inserting into the database.", | ||
"url": response_url_base}), 500 | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,6 +29,7 @@ services: | |
POSTGRES_PASSWORD: test_password | ||
POSTGRES_DB: test_database | ||
API_HOST: http://api_is_here | ||
UPLOAD_URL: /project/endpoints/uploads/ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For tests we try to come as close as possible to production. Putting the data storage in our codebase isn't something done in production. Change this to e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
volumes: | ||
- .:/app | ||
command: ["pytest"] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,9 +23,16 @@ def test_post_project(db_session, client, course_ad, course_teacher_ad, project_ | |
db_session.commit() | ||
|
||
project_json["course_id"] = course_ad.course_id | ||
# cant be done with 'with' because it autocloses then | ||
# pylint: disable=R1732 | ||
project_json["assignment_file"] = open("testzip.zip", "rb") | ||
|
||
# post the project | ||
response = client.post("/projects", json=project_json) | ||
response = client.post( | ||
"/projects", | ||
data=project_json, | ||
content_type='multipart/form-data' | ||
) | ||
assert response.status_code == 201 | ||
|
||
# check if the project with the id is present | ||
|
@@ -34,7 +41,6 @@ def test_post_project(db_session, client, course_ad, course_teacher_ad, project_ | |
|
||
assert response.status_code == 200 | ||
|
||
|
||
def test_remove_project(db_session, client, course_ad, course_teacher_ad, project_json): | ||
"""Test removing a project to the datab and fetching it, testing if it's not present anymore""" | ||
|
||
|
@@ -46,8 +52,11 @@ def test_remove_project(db_session, client, course_ad, course_teacher_ad, projec | |
|
||
project_json["course_id"] = course_ad.course_id | ||
|
||
# cant be done with 'with' because it autocloses then | ||
# pylint: disable=R1732 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this be done by putting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
project_json["assignment_file"] = open("testzip.zip", "rb") | ||
# post the project | ||
response = client.post("/projects", json=project_json) | ||
response = client.post("/projects", data=project_json) | ||
|
||
# check if the project with the id is present | ||
project_id = response.json["data"]["project_id"] | ||
|
@@ -59,7 +68,6 @@ def test_remove_project(db_session, client, course_ad, course_teacher_ad, projec | |
response = client.delete(f"/projects/{project_id}") | ||
assert response.status_code == 404 | ||
|
||
|
||
def test_patch_project(db_session, client, course_ad, course_teacher_ad, project): | ||
""" | ||
Test functionality of the PUT method for projects | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only import from the dependency what you use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e1624b1