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

Prevent adding of terms to distributed posts & modifying discussion settings. #1069

Merged
merged 3 commits into from
Jun 12, 2023

Conversation

peterwilsoncc
Copy link
Collaborator

@peterwilsoncc peterwilsoncc commented Jun 6, 2023

Description of the Change

Modifies the CSS used on distributed posts to disable to modification of terms to prevent the labels from being clicked on. As a side effect, this also prevents modifying the discussion settings too (as it also changed by clicking on the label).

Closes #468

How to test the Change

Changelog Entry

Fixed - Prevent the modification of terms on Distributed content.

Credits

Props @peterwilsoncc, @turtlepod, @jeffpaul.

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@peterwilsoncc peterwilsoncc marked this pull request as ready for review June 7, 2023 01:11
@peterwilsoncc peterwilsoncc requested a review from a team as a code owner June 7, 2023 01:11
@peterwilsoncc peterwilsoncc requested review from faisal-alvi, ravinderk and a team and removed request for a team June 7, 2023 01:11
@peterwilsoncc peterwilsoncc added this to the 2.0.0 milestone Jun 7, 2023
Copy link
Contributor

@ravinderk ravinderk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@peterwilsoncc This fix only works if the admin uses the mouse. Admin can still use the keyboard (TAB) to edit categories, tags, and other settings.

I can see potential side effects with CSS and javascript approaches to resolve it.

Another approach:
We want to allow the admin to switch post status draft <---> publish and trash, right? In this case, I suggest hiding the Update button instead of turning off other actions. We should also display a notice that the admin is not allowed to update post content and metadata for distributed posts.

@jeffpaul and @dkotter, Can you provide your feedback on this?

@kmgalanakis-sage
Copy link

This is the first time I'm working with this plugin so please correct me if I'm saying something wrong.

I have a similar suggestion to the one from @ravinderk. Instead of disabling all the checkboxes and text areas, or even hiding the Update button we can do the following. Let the user do whatever modification he needs to the distributed post and right before saving/publishing we can detect at the level of the editor what has been modified and which of those modifications are in the allowed or the non-allowed list of modified things. If modified things are all in the allowed list, it's fine and we proceed with the selected action. If there are modifications from within the non-allowed list then we can display a notice that modifying the X, Y, or Z field is not allowed and this can only be done if the distributed post is unlinked from the origin post.

What do you think about that @jeffpaul, @dkotter and @ravinderk ?

@ravinderk
Copy link
Contributor

This is the first time I'm working with this plugin so please correct me if I'm saying something wrong.

I have a similar suggestion to the one from @ravinderk. Instead of disabling all the checkboxes and text areas, or even hiding the Update button we can do the following. Let the user do whatever modification he needs to the distributed post and right before saving/publishing we can detect at the level of the editor what has been modified and which of those modifications are in the allowed or the non-allowed list of modified things. If modified things are all in the allowed list, it's fine and we proceed with the selected action. If there are modifications from within the non-allowed list then we can display a notice that modifying the X, Y, or Z field is not allowed and this can only be done if the distributed post is unlinked from the origin post.

What do you think about that @jeffpaul, @dkotter and @ravinderk ?

@kmgalanakis-sage I agree that we should allow whitelisted post data for distributed posts and for others we should return an error, a display notice, etc.

@peterwilsoncc
Copy link
Collaborator Author

@peterwilsoncc This fix only works if the admin uses the mouse. Admin can still use the keyboard (TAB) to edit categories, tags, and other settings.

I can see potential side effects with CSS and javascript approaches to resolve it.

Yes, I've noticed this too with the existing code, for example hitting return on a confirmation box works but clicking the button does not.

To avoid the perfect being the enemy of the good, I think we can continue with the CSS approach and consider a JavaScript approach in an subsequent release.

The editor.PostTaxonomyType filter can be used to replace the taxonomy UI with a read only version, for other settings and meta data the approach may be more complicated.

Copy link
Contributor

@ravinderk ravinderk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@peterwilsoncc I agree with your comment #1069 (comment), and this makes sense to continue with the CSS approach and update it in the subsequent release.

@peterwilsoncc peterwilsoncc merged commit 876f4b6 into develop Jun 12, 2023
@peterwilsoncc peterwilsoncc deleted the fix/468-adding-terms branch June 12, 2023 23:04
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.

Can Still Assign Term to Linked Post
3 participants