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

FIX: Return actual errors if PostCreator fails #7096

Merged
merged 2 commits into from Mar 6, 2019

Conversation

5 participants
@maulkin
Copy link
Contributor

commented Mar 3, 2019

https://meta.discourse.org/t/insufficient-checking-of-postcreator-in-importer-base/110570
Extract:
It seems that if (for example) you mistakenly try and add
target_usernames to a post with user_ids rather than usernames,
create_post in scripts/import_scripts/base.rb won’t return an error.

This is because it returns: post ? post :
post_creator.errors.full_messages. However, post has already been
initialised as a Post type on line 564. Checking for the post_creator
errors instead returns the correct behaviour.

Neil McGovern
Return actual errors if PostCreator fails
https://meta.discourse.org/t/insufficient-checking-of-postcreator-in-importer-base/110570
Extract:
It seems that if (for example) you mistakenly try and add
target_usernames to a post with user_ids rather than usernames,
create_post in scripts/import_scripts/base.rb won’t return an error.

This is because it returns: post ? post :
post_creator.errors.full_messages. However, post has already been
initialised as a Post type on line 564. Checking for the post_creator
errors instead returns the correct behaviour.
@discoursebot

This comment has been minimized.

Copy link

commented Mar 3, 2019

You've signed the CLA, maulkin. Thank you! This pull request is ready for review.

@maulkin maulkin changed the title Return actual errors if PostCreator fails FIX: Return actual errors if PostCreator fails Mar 3, 2019

@discoursebot

This comment has been minimized.

Copy link

commented Mar 4, 2019

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/insufficient-checking-of-postcreator-in-importer-base/110570/3

@eviltrout
Copy link
Member

left a comment

Can you add a test to show the case this handles that was not handled before?

@maulkin

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

Sure - Did you want it adding as a test case specifically, or should I just add some code here to show what happens/should happen?

@eviltrout

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

@maulkin I would add a test that fails if your patch is not present.

@gschlager

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

@eviltrout We don't really have tests for import scripts and the few that we have are not executed by either Jenkins nor Travis.

@eviltrout

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

Ah my bad @gschlager I thought the change was in PostCreator which absolutely needs tests. If not, no big deal.

@gschlager gschlager merged commit 655a08d into discourse:master Mar 6, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@gschlager

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

Thanks! 👍

@gschlager

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

For the record: Your first solution was the correct one. 😉
I fixed it in 941e096

@maulkin

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

Wonderful, thanks folks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.