These are chat archives for fossasia/open-event-server

19th
Jun 2018
Bhavesh Anand
@bhaveshAn
Jun 19 2018 06:44
:+1:
Mario Behling
@mariobehling
Jun 19 2018 14:51
@schedutron When are you following up on your blog posts?
Max Lorenz
@maxlorenz
Jun 19 2018 16:40
short question: how are discount codes calculated? does the order amount reflect the discount or do I have to subtract the discounts? what I got so far is revenue = event.orders.sales - fees
Saurav Tiwary
@srv-twry
Jun 19 2018 16:58

@maxlorenz @mayank8318 @schedutron @Kreijstal @bhaveshAn Please review: fossasia/open-event-server#4908, fossasia/open-event-server#4930

Both of them are straightforward ones and can be reviewed very easily.

Max Lorenz
@maxlorenz
Jun 19 2018 17:57
@srv-twry first looks good, second also but is this really what we want to do? deleted_at needs to be integrated into every query now, is that correct, e.g. (select all queries where deleted_at is not none)?
Saurav Tiwary
@srv-twry
Jun 19 2018 18:07
Yes @maxlorenz that's an unfortunate outcome of the way it has been implemented.
select all queries where deleted_at is None actually :smile:
Saurabh Chaturvedi
@schedutron
Jun 19 2018 18:10
@mariobehling I'm having a hard time finding images that are explicitly labelled for re-use. But since this is a recurring issue in my drafts, I have already added an entirely custom-made image for my latest blog post.
Saurav Tiwary
@srv-twry
Jun 19 2018 18:18

Yes @maxlorenz that's an unfortunate outcome of the way it has been implemented.

But FYI, we have already customized the flask-rest-json api to meet our needs. So by default it only returns the ones that aren't deleted i.e. whose deleted_at column in None.
You've to pass in is_trashed=True as the query parameter in the url in order to get the deleted ones too.

@maxlorenz
Now as far the query statements that we have written inside the application. They don't need to change IMO.
Saurav Tiwary
@srv-twry
Jun 19 2018 18:40
Giving it a second thought, I think we have to change the queries inside the application.
for e.g: Let's say we have an event. Now we create a tax associated with the event. Now we delete the tax. Now if we try to create a tax again for the event, then it won't allow us saying that "a tax already exists for the event" but it should because that tax was already deleted. :confused:
@maxlorenz
Max Lorenz
@maxlorenz
Jun 19 2018 18:45
Yeah difficult to see all implications. IMHO, a separate table makes the decision very explicit (you have to include another table to get deleted entities) while implicit behavior in a central place, like removing certain rows based in column names and values, can be a “surprise” when you forget such effects and cause hidden bugs
I prefer the extra tables but both ways can work fine I guess :)
Saurav Tiwary
@srv-twry
Jun 19 2018 18:54
Yes both can work but I am kind of confused because of the fact that if we try to add separate tables for each model's deleted entries, it would take a lot of time as well. We then have to modify our rest library accordingly as well which is well setup to handle the first way i.e. the deleted_at column way for almost an year now. I'm not sure we have that much time to refactor it since releasing the v2 version is a priority.
Right now my pick would be to create a common helper method very similar to the safe_query method which would take the model, query conditions and an additional parameter take_trashed or something like that which would indicate whether the query results should include soft deleted entries or not. We can use this method throughout the project and that would work for now.
Saurav Tiwary
@srv-twry
Jun 19 2018 19:01
@SaptakS @niranjan94 What are your thoughts on this ?
This is in reference to this PR: fossasia/open-event-server#4908
SaptakS @SaptakS checking
Saptak Sengupta
@SaptakS
Jun 19 2018 19:59
@srv-twry as far as I remember all the APIs are soft deleted anyways. Because @shubham-padia changed it to that in his fork.
@shubham-padia can you provide some input about this?
Saurav Tiwary
@srv-twry
Jun 19 2018 20:01
Yes, they are but the issue is that he only added it in the event, user and sessions model.
Recently I asked @niranjan94 about it and he said that if we are allowing the user to recover event, then we should also recover the speaker, track, stripe etc. i.e. everything associated with that event. So we will need to add it to every table associated with an event or user.

This is what he said:

any model and its related models that can be deleted by the user would need soft deletes.
So, basically event model + all its related resources.
user model and and all of user's related resources.
see ... for example, sponsors is directly related to an event.
If a user deletes an event, sponsors should also be deleted. And when the user restores the event, sponsors should also be restored.
It doesn't make sense to just restore the event and end up losing everything else does it.

So I opened the PR: fossasia/open-event-server#4908 in order to add it in every user and event related model.
Saptak Sengupta
@SaptakS
Jun 19 2018 20:03
hmm
Looks good code wise.
Let me check in a little more details
Saurav Tiwary
@srv-twry
Jun 19 2018 20:13

Now the issue is that, let's say we have an event. Now we create a tax for that event. Since tax is also an event related field,so it will also have a deleted_at column.
Now for some reason, I decide to delete the tax. Since by default we have soft deletion enabled. The tax would be soft deleted but the entry would still be in the database.
Now let's say I i.e. the event organizer wants to add tax again. So I will make another post request. Now before creating a tax object for an event, we check if a tax object already exists in the db for that event or not like this :

if self.session.query(Tax).filter_by(event_id=data['event']).first():
            raise ConflictException({'pointer': '/data/relationships/event'}, "Tax already exists for this event")

Now even though the tax entry was deleted by the user, this snippet would still raise ConflictException since that deleted entry is still present in the db. Hopefully you can see the issue here now.

To resolve this we have to change this snippet to:

if self.session.query(Tax).filter_by(event_id=data['event'] and deleted_at=None).first():
            raise ConflictException({'pointer': '/data/relationships/event'}, "Tax already exists for this event")

We have to change this everywhere in the project otherwise you can imagine the consequences.

Let me check in a little more details

Yes sure, but the issue that me and @maxlorenz are discussing here is more about this overall approach that we have to soft deletion.

Saurav Tiwary
@srv-twry
Jun 19 2018 20:22

Now we have two ways to resolve this:

  1. Change every query to filter based on deleted_at=None. PROS: Can be done quickly. We can create a separate helper and use it everywhere.
    CONS: If the developer forgets about this soft deletion case then we will have bugs which are really hard to find.

  2. As suggested by @maxlorenz . We should have separate tables for each model for eg. deleted_events, deleted_tracks etc.
    PROS: Devs don't have to worry about taking care of soft deletion that much.
    CONS: We'll have to start almost from scratch again. Change everything that @shubham-padia did in his fork of the library. Do we have that much time when we want to release the software ASAP ? Time is the only problem here.

Max Lorenz
@maxlorenz
Jun 19 2018 20:42
how many queries must return deleted events/tracks etc?
Saurav Tiwary
@srv-twry
Jun 19 2018 20:43
Not a lot. It is only required in the admin section I guess where we want to provide the admin an option to recover an event. I am not very sure though.