Open BoPeng opened 2 years ago
@ellmetha Please let me know if this feature is potentially acceptable so that I can either close or improve it. Thanks.
Hey @BoPeng! 👋
I think that the idea of not deleting disapproved posts right away could be explored, but I don't think that this should be treated as a regular soft-deletion case. I see soft-deletion as a low-level mechanism that you can choose to use (or not use) for your models depending on the requirements of your application and how critical the data you are dealing with is.
In the present case I think that what we want is a new feature that would allow to better handle disapproved posts, but this should not translate in an override of the standard delete()
method in my opinion. Indeed, with your proposal you are making the assumption that the deletion of a Post
record that is not disapproved translates into the disapproval of it, but delete()
is a very low-level method that shouldn't introduce such side effects in my opinion (because this could lead to some unwanted consequences).
Maybe the approved
field should be replaced by something more powerful like a proper status
field that could accept more values (such as approved
, pending_approval
, disapproved
). Using a boolean for this can be tempting but this is very limiting and not future-proof (what if more status values have to be added later on?).
This would also require a few more things like:
So to answer your question I would be open with to addition of such feature, but I am against leveraging a soft-deletion mechanism for this: it appears that what we want is to "disapprove" posts, and not delete them. So the current Pull Request would need to be revisited.
Also, this is a very big feature that would probably introduce backward incompatible changes, which would translate into a new major release. I won't have time to work on this personally as I am very involved with another project at the moment, but if you want to keep working on this and implement the full solution I'll definitely review your Pull Request.
Hi, @ellmetha
This PR is essentially a quick patch to support my own website so the implementation was certainly not finalized (and overriding delete()
was just a less-invasive change compared to replacing delete()
with disapprove()
everywhere it is called). Let us start from deciding what approval status we should have. Currently,
approved=True
status and somehow be assigned approved=False
(perhaps reported by users), or posts can have default approved=False
so all posts need to be moderated before going public.approved=False
posts. Either approve them or delete them.So essentially speaking, approved=False
is a misnomer that actually means pending moderation
.
To allow users to revise disapproved posts, the moderation process has to be changed:
Posts should have status approved
(the same as approved=True
), pending moderation
(the same as approved=False
above), and disapproved
(viewable only to poster).
Admin moderates all pending moderation
posts, and change them to approved
or disapproved
status.
The PR therefore has
approved=True
as approved
approved=None
as pending moderation
approved=False
as disapproved
There could be another status revised
but I treat it the same as pending moderation
. I cannot think of another moderation-related status.
Maybe the approved field should be replaced by something more powerful like a proper status field that could accept more values (such as approved, pending_approval, disapproved). Using a boolean for this can be tempting but this is very limiting and not future-proof (what if more status values have to be added later on?).
So we agree with each other on the three states here but disagree on reusing the approved
field. I much prefer reusing approved
because the changes are much less invasive, and are very likely to be good enough. Actually, my project already adds an status
field for transaction status so adding status
is out of the question for me.
@ellmetha I have updated the PR
approve()
and disapprove()
instead of overriding delete()
.I continued to use the approved
field to keep backward compatibility. If you believe that another more general field could be used, we can expose APPROVED_FILTER
(which is currently derived from other settings) so that users can define it using their own fields.
I can work on tests if you agree with the direction this PR is going.
Currently, disapproved posts are deleted, which does not allow posters to improve the posts and re-publish
This PR allows
Post.approved
to have an initialNone
(unknown) status, so that onlyNone
posts are moderated to beTrue
orFalse
. Most of the changes are fromto
In addition,
Post.delete()
will setapproved=False
before the posts can be permanently deleted. This effectively allows for soft-deletion.self.object.delete()
call inPostDisapproveView
will only set theapproved
status, instead of actually deleting the post immediately.The behavior is controlled by a setting
MACHINA_DEFAULT_APPROVAL_STATUS
to keep backward compatibility.This is a quick PR in response to #246. I will improve it (check tests, templates etc) if @ellmetha shows enough interest.