Import JSON #51
Import JSON #51
Conversation
… expenses template duplicate messages removed.
app/recurring_expenses/views.py
Outdated
| raise ValueError("Invalid CSV data in the file.") | ||
|
|
||
|
|
||
| def upload_json_date(data: dict): |
There was a problem hiding this comment.
Is _date a typo? Should it be data instead?
There was a problem hiding this comment.
don't forget to update everywhere this method is called as well :)
There was a problem hiding this comment.
That is a typo, I'll fix it in the next commit.
app/recurring_expenses/views.py
Outdated
| for item in data: | ||
| try: | ||
| recurring_expense = RecurringExpense( | ||
| id=item["id"] if "id" in item else None, |
There was a problem hiding this comment.
The idiomatic python way of doing this is
id=item.get("id")
# NoneIf item does not have an id, item.get("id") will return None
There was a problem hiding this comment.
Note, you can also supply a default value to get. So you can also do item.get("foo", "bar")
If foo is not a key, the default value bar will be returned instead
app/recurring_expenses/views.py
Outdated
|
|
||
| try: | ||
| recurring_expense = RecurringExpense( | ||
| id=UUID(row[0]) if row[0] else None, |
There was a problem hiding this comment.
Not sure what the intended behavior here is? Unless the row is empty, there will always be a row[0] right?
If the row is completely empty, then we probably wont make it to this line
What are we trying to check here? Maybe it handles the following scenario
id,active,particulars,amount,currency,amount_nzd,period
00000000-0000-0000-0000-000000000000,True,Foo,1.00,NZD,1.00,1
,True,Bar,1.00,NZD,1.00,1
There was a problem hiding this comment.
I actually put that so that the ID in case the ID is not provided by the user in the import file, it's generated automatically, but we can have it mandatory as well.
There was a problem hiding this comment.
nah thats fine. I sort of realised the answer while I was typing the question I think its fine as it is?
app/recurring_expenses/views.py
Outdated
| id=UUID(row[0]) if row[0] else None, | ||
| active=parse_bool(row[1]), | ||
| particulars=row[2].strip(" "), | ||
| amount=row[3].strip("$ "), |
There was a problem hiding this comment.
I'm suprised this doesn't need to be converted to Decimal? It seems to work none the less so 🤷
There was a problem hiding this comment.
I guess so, the csv upload logic was per-written I just refactored it into a different function. Guess it's fine?
app/recurring_expenses/views.py
Outdated
| if uploaded_file.content_type not in _IMPORT_CONTENT_TYPES: | ||
| messages.error(request, "Invalid file type") | ||
| return HttpResponseRedirect(reverse("recurring_expenses:import")) |
There was a problem hiding this comment.
I like that we are checking to make sure the file's content type is one of the expected content_types
app/recurring_expenses/views.py
Outdated
| _IMPORT_CONTENT_KEYS = { | ||
| "json": "application/json", | ||
| "csv": "text/csv", | ||
| } | ||
|
|
||
| _IMPORT_CONTENT_TYPES = {value: key for key, value in _IMPORT_CONTENT_KEYS.items()} |
There was a problem hiding this comment.
Constants are fine but in python we don't really prefix them with an _
I would prefer _IMPORT_CONTENT_KEYS becomes IMPORT_CONTENT_KEYS
Same for any other constants
There was a problem hiding this comment.
Sure, will rename those.
|
@iokiwi I've made the appropriate changes 😄 |
import from JSON implemented | import from CSV refactored | recurring expenses template duplicate messages removed.