Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Triple approval status that allows disapproved posts to be revised #273

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Triple approval status that allows disapproved posts to be revised #273

wants to merge 15 commits into from

Conversation

BoPeng
Copy link
Contributor

@BoPeng BoPeng commented Apr 23, 2022

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 initial None (unknown) status, so that only None posts are moderated to be True or False. Most of the changes are from

filter(approved=True)

to

exclude(approved=False)

In addition,

  1. Post.delete() will set approved=False before the posts can be permanently deleted. This effectively allows for soft-deletion.
  2. Consequently, the self.object.delete() call in PostDisapproveView will only set the approved status, instead of actually deleting the post immediately.
  3. Posters are allowed to view and edit disapproved posts, or permanently delete it.

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.

@BoPeng BoPeng changed the title Allow approval status to have three states Soft deletion and revision of disapproved posts Apr 23, 2022
@BoPeng
Copy link
Contributor Author

BoPeng commented Sep 30, 2022

@ellmetha Please let me know if this feature is potentially acceptable so that I can either close or improve it. Thanks.

@ellmetha
Copy link
Owner

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:

  • the ability to see the posts that were disapproved
  • the ability to undo the disapproval of a post

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.

@BoPeng
Copy link
Contributor Author

BoPeng commented Oct 14, 2022

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,

  1. Posts can have default 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.
  2. Admin moderates all 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:

  1. Posts should have status approved (the same as approved=True), pending moderation (the same as approved=False above), and disapproved (viewable only to poster).

  2. Admin moderates all pending moderation posts, and change them to approved or disapproved status.

The PR therefore has

  1. approved=True as approved
  2. approved=None as pending moderation
  3. 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.

@BoPeng BoPeng changed the title Soft deletion and revision of disapproved posts Triple approval status that allows disapproved to be revised Oct 25, 2022
@BoPeng BoPeng changed the title Triple approval status that allows disapproved to be revised Triple approval status that allows disapproved posts to be revised Oct 25, 2022
@BoPeng
Copy link
Contributor Author

BoPeng commented Oct 25, 2022

@ellmetha I have updated the PR

  1. use approve() and disapprove() instead of overriding delete().
  2. add an additional setting to control if pending posts are displayed.

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.

@BoPeng BoPeng closed this by deleting the head repository Mar 17, 2023
@BoPeng BoPeng reopened this Mar 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants