Backend / Purchase functionality

[milensski]

Hi, This PR is implementation for the issue #61.

Most of the functionality was already implemented in PR #49 so the new part is :

PATCH Request for updating the status of the order will update the order if the user is the sender or receiver and also:

• this creates Order History record in a new table to track order statuses

  [!IMPORTANT] Make sure to rebuild the image in docker when testing locally

• IF the order is changed to rejected it restores the purchased quantity to the listing as well as the is_sold and is_listed flags and it will lock it for further updates.

• If the order is completed it will also lock the status update.

The custom command for auto-completing all orders on status sent or above more than 7 days has also been created, but unfortunately, I was not able to integrate it to work in the docker container with cronjob (the cronjob is created but for some reason it is not triggered) It is in the changes of this PR and does not interfere with the current flow of the server, it can be debugged later. Manually executing the command is working. python manage.py complete_old_orders will trigger it.

I have added 3 tests as well.

Please review and if something is missing, I will add it or change it. Thank you!

I want to apologize for my absence and for the caused delay in the project, I am now here and able to contribute again!

[julkascript]

Are the following migrations applied in production? I think not, but can you confirm, please?

0002_rename_given_to_feedbackandrating_receiver_user_and_more.py
0003_alter_feedbackandrating_receiver_user_and_more.py

• If they are not applied, you can totally delete them, no problem - even better, keep the number of migrations low. 1 migration per version is perfect.
• If they are applied, we will have to drop the entire db.

[milensski]

Are the following migrations applied in production? I think not, but can you confirm, please?

0002_rename_given_to_feedbackandrating_receiver_user_and_more.py
0003_alter_feedbackandrating_receiver_user_and_more.py

• If they are not applied, you can totally delete them, no problem - even better, keep the number of migrations low. 1 migration per version is perfect.
• If they are applied, we will have to drop the entire db.

As far as I know there are not applied, @ZhivkoTringov or @dinocom33 , can advise If I am right.

[dinocom33]

Are the following migrations applied in production? I think not, but can you confirm, please?

0002_rename_given_to_feedbackandrating_receiver_user_and_more.py
0003_alter_feedbackandrating_receiver_user_and_more.py

• If they are not applied, you can totally delete them, no problem - even better, keep the number of migrations low. 1 migration per version is perfect.
• If they are applied, we will have to drop the entire db.

As far as I know there are not applied, @ZhivkoTringov or @dinocom33 , can advise If I am right.

They not applied.

[RyotaMitaraiWeb]

Most of the status flow works (at least as far as I understand it), but there are a few behaviors that I don't think are intended:

• the purchaser can directly mark an order as rejected or completed, even if the order has not been sent. I would think that there would be checks to make sure that you can complete or reject an order only when it's marked as sent (as far as I understand the status flow, at least)
• this is moreso a question, as I am not sure what the intended behavior is, but are both the seller and purchaser supposed to be able to mark the order as complete or rejected? I would think only the purchaser should be able to do that, but the seller can also mark them as such
• theoritically, you can regress the status (except when it's rejected or completed, where it correctly locks the status); one can mark the order as sent and then mark it as ordered. If this is intended, ignore this.

Also, the order API does not provide the purchased cards at all, which would prevent me from displaying them in the modal (according to the Swagger schema, there's supposed to be an order_items property, but it's missing on my end). Not sure if this is in the scope of this PR or an issue on my end, but figured I should mention this.

[milensski]

@RyotaMitaraiWeb notes taken, please check now and let me know what you think, I will correct or add something if needed

Also, the order API does not provide the purchased cards at all, which would prevent me from displaying them in the modal (according to the Swagger schema, there's supposed to be an order_items property, but it's missing on my end).

• OrderItems are now being correctly populated

this is moreso a question, as I am not sure what the intended behavior is, but are both the seller and purchaser supposed to be able to mark the order as complete or rejected? I would think only the purchaser should be able to do that, but the seller can also mark them as such

• This is intended behavior as described in the issue, but I also think it is more convenient that way because for example if the card is sent to the buyer but he return it or don't take it the seller should be able to reject, so the listing is restored. Or simply seller don't want to send it he should be able to reject the order as well.

theoritically, you can regress the status (except when it's rejected or completed, where it correctly locks the status); one can mark the order as sent and then mark it as ordered. If this is intended, ignore this.

• I am not sure if this was required but I have made validation for not being able to downgrade the status :)

@julkascript I have corrected the bash syntaxis, I have missed it because I have already a superuser in my DB. Please let me know your comments. Thanks.

[RyotaMitaraiWeb]

My concerns seem to have been addressed (looking at the Figma wireframes, it looks like outright rejection is possible, so good thing that only completion has been restricted to after being sent).

With that said, I found another issue that is hopefully an easy fix. Currently, the backend treats "received" and "completed" as different statuses, but according to the Figma wireframes, they are actually the same. As a matter of fact, if you mark an order as received, you will be unable to mark it as completed, because the backend will not allow you to mark it as sent and thus will not allow you to complete it either. Funnily enough, you can actually reject the order at this point. For consistency, we should probably stick to only one of the statuses (I would prefer "received" as it matches what is shown on the frontend)

[milensski]

My concerns seem to have been addressed (looking at the Figma wireframes, it looks like outright rejection is possible, so good thing that only completion has been restricted to after being sent).

With that said, I found another issue that is hopefully an easy fix. Currently, the backend treats "received" and "completed" as different statuses, but according to the Figma wireframes, they are actually the same. As a matter of fact, if you mark an order as received, you will be unable to mark it as completed, because the backend will not allow you to mark it as sent and thus will not allow you to complete it either. Funnily enough, you can actually reject the order at this point. For consistency, we should probably stick to only one of the statuses (I would prefer "received" as it matches what is shown on the frontend)

Totally agree with you, while checking the figma my thoughts were the same and funny enough I chose completed status as more intuitive for me. It will be more easy if we consider completed for absolute, but its not hard to rewrite for received.

[julkascript]

My concerns seem to have been addressed (looking at the Figma wireframes, it looks like outright rejection is possible, so good thing that only completion has been restricted to after being sent). With that said, I found another issue that is hopefully an easy fix. Currently, the backend treats "received" and "completed" as different statuses, but according to the Figma wireframes, they are actually the same. As a matter of fact, if you mark an order as received, you will be unable to mark it as completed, because the backend will not allow you to mark it as sent and thus will not allow you to complete it either. Funnily enough, you can actually reject the order at this point. For consistency, we should probably stick to only one of the statuses (I would prefer "received" as it matches what is shown on the frontend)

Totally agree with you, while checking the figma my thoughts were the same and funny enough I chose completed status as more intuitive for me. It will be more easy if we consider completed for absolute, but its not hard to rewrite for received.

Your remarks make sense, guys. To me, both options would be equally as fine. Maybe you can align between the both of you and come to a conclusion.

[RyotaMitaraiWeb]

whether it's "completed" or "received" isn't a hill I am willing to die on, I am perfectly fine with whichever is selected. I suggested "received" purely because I felt it made more sense to match it with the frontend.

The main point is that the backend should probably use only one of them, since having both will make things more complex. Sticking with "completed" is perfectly fine, seeing as the validations for it have already been written

[milensski]

@julkascript is there anything else to change in the code? SInce we have agreed to work with completed, the logic in the serializer for updating is validating by status completed and there is nothing to change.

Please review and have your comments if I missed something or I need to add.

[julkascript]

No more comments. I was waiting for the removal of the received value from the status.

[milensski]

No more comments. I was waiting for the removal of the received value from the status.

I have deleted the unused statuses and made a migration since they are in the deployed version.

[RyotaMitaraiWeb]

I think the status flow now works appropriately (or, at the very least, I have not found a way to break it).

The only concern that I have at this point is that the PATCH endpoint can theoritically edit the delivery address. I could maybe see this as a feature, especially if the upcoming modal allows the buyer to change the address, but I will need a clarification on this matter to be sure.

If this is not a concern, then I will approve the PR, since I cannot think of issues with the API at the moment.

[julkascript]

I think the status flow now works appropriately (or, at the very least, I have not found a way to break it).

The only concern that I have at this point is that the PATCH endpoint can theoritically edit the delivery address. I could maybe see this as a feature, especially if the upcoming modal allows the buyer to change the address, but I will need a clarification on this matter to be sure.

If this is not a concern, then I will approve the PR, since I cannot think of issues with the API at the moment.

Yes, correct observation. The delivery address shall not be able to be modified from this endpoint (as well as the receiver user).

[milensski]

I think the status flow now works appropriately (or, at the very least, I have not found a way to break it). The only concern that I have at this point is that the PATCH endpoint can theoritically edit the delivery address. I could maybe see this as a feature, especially if the upcoming modal allows the buyer to change the address, but I will need a clarification on this matter to be sure. If this is not a concern, then I will approve the PR, since I cannot think of issues with the API at the moment.

Yes, correct observation. The delivery address shall not be able to be modified from this endpoint (as well as the receiver user).

Your comments have been addressed, and I have edited the update method for this endpoint restricting any updates different than the status. I have not thought about it but you have a point. The order should not be able to be changed once it is created. This means if its not correctly placed it should be rejected and then re-ordered.

@julkascript @RyotaMitaraiWeb Please let me know if you have other concerns as well.

[RyotaMitaraiWeb]

It doesn't seem like not received is implemented as a valid status

(speaking of which, does not received terminate the order like completed and rejected do? Also, is the buyer capable of marking an order as not received before it's sent? This is moreso a question for @julkascript since I am currently integrating the solution on my end as well)

[julkascript]

Hey. When we were discussing the statuses, we thought of “not received” as a status where something went wrong with the package. However, we can skip this status for now - we can implement later.