Fix DocType._get_actions preventing delete

[seljin]

Hi everybody, thank you for the good work here.

There is a case when ES index is not updated on instance deletion :

If we base our should_index_object method on manytomany field, when we delete an object, the related objects are deleted before the object itself and doesnt exists anymore when should_index_object method is called.

Thus, should_index_object return is incoherent in this case and the ES index is not updated.

I think we should not call should_index on delete action, what do you think ?

I also corrected some flake8 errors.

[safwanrahman]

Interesting find. Can you let me know how to reproduce the issue? I was the person who implemented should_index_object API and it might be a good learning for me.
Thanks @seljin for finding it out. 🚀

[seljin]

Of course 👍 you can use this example : https://github.com/seljin/ded-should-index-example

I stay available if you need anything :)

[saadmk11]

Great Find @seljin! 💯

Not calling should_index_object() for delete action should fix the issue with related objects.

But I am concerned about people who are already overriding should_index_object() to customize delete action as well. So, not calling should_index_object() for delete action will break their implementation.

Another way might be passing the action variable as a parameter to should_index_object() so that the users can handle different actions differently but that will also break other people's implementation who are overriding should_index_object().

[seljin]

Thank you for your response :)

Could you show me an example of how and why you customize should_index_object() for delete action please ?

I can't think of a case that you need to do that because when you delete an object in the database you need to delete it in the index no matter what. If you dont do that the index is not synced with your database.

Maybe there is something I cant see here, it might be a good learning for me if you could show me an example

[safwanrahman]

@seljin Good point. What do you think @saadmk11 ?

[codecov-commenter]

Codecov Report

Merging #370 (e0b9361) into master (6470dd7) will increase coverage by 0.14%.
The diff coverage is 100.00%.

❗ Current head e0b9361 differs from pull request most recent head e54b237. Consider uploading reports for the commit e54b237 to get more accurate results

@@            Coverage Diff             @@
##           master     #370      +/-   ##
==========================================
+ Coverage   67.85%   68.00%   +0.14%     
==========================================
  Files          12       12              
  Lines         756      747       -9     
==========================================
- Hits          513      508       -5     
+ Misses        243      239       -4     

Impacted Files	Coverage Δ
django_elasticsearch_dsl/documents.py	76.66% <100.00%> (ø)
django_elasticsearch_dsl/fields.py	61.95% <100.00%> (+0.29%)	⬆️
django_elasticsearch_dsl/registries.py	74.16% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6470dd7...e54b237. Read the comment docs.

[saadmk11]

Yup, @seljin is right. I was thinking about something different which is not the case for this package. I am 👍 with this.

[seljin]

Great, thank you for your time @saadmk11 and @safwanrahman 👍

[safwanrahman]

Looks Good! Thanks @seljin and @saadmk11