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

DEV: Refactor xmlrpc_publish_post_to_discourse #491

Conversation

scossar
Copy link
Contributor

@scossar scossar commented Oct 23, 2023

Attempts to improve the xmlrpc_publish_post_to_discourse method. That method exists for historical reasons. I'm reluctant to remove it entirely, but I suspect that it's not being used on any sites.

This PR also:

  • removes the call from publish_post_after_save to update_post_meta( $post_id, 'publish_post_category', intval( $this->options['publish-category'] ) ); for force published posts. That is already being handled in the sync_to_discourse_work method.
  • moves the call to sanitize_title( $title ) from publish_post_after_save to the sync_to_discourse_work method
  • adds a boolean return value to publish_failure_notification to allow that method to be easily tested
  • changes the EmailNotification object that's used in the DiscoursePublishTest setUp method so that it's a partial mock ($this->email_notifier = \Mockery::mock( EmailNotification::class )->makePartial();)
  • updates the tests I've added to DiscoursePublishTest to avoid overwriting self::$plugin_options for subsequent tests (there are still tests in this class that have that issue. I can update them.)

Todo:

  • add doc comment to DiscoursePublishTest email_notifier property
  • ...

@scossar
Copy link
Contributor Author

scossar commented Oct 24, 2023

Updated the PR. It now seems complete to me.

@scossar scossar changed the title WIP: Refactor xmlrpc_publish_post_to_discourse DEV: Refactor xmlrpc_publish_post_to_discourse Oct 24, 2023
@scossar scossar marked this pull request as ready for review October 24, 2023 00:47
Copy link
Collaborator

@angusmcleod angusmcleod left a comment

Choose a reason for hiding this comment

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

@scossar Some very minor changes and I'll merge this. Thanks again 👍

lib/discourse-publish.php Outdated Show resolved Hide resolved
tests/phpunit/test-discourse-publish.php Outdated Show resolved Hide resolved
…; Use '__return_true' in add_filter callback.
@angusmcleod
Copy link
Collaborator

angusmcleod commented Oct 25, 2023

@scossar Thanks again.

@Jailyard90Grad This is also ready for final review / merge. Thanks!

@Jailyard90Grad Jailyard90Grad merged commit d5d84d9 into discourse:main Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants