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.

Sign up for GitHub

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

Jump to bottom

Import app branch #243

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Import app branch #243

wants to merge 14 commits into from

Conversation

shahidajmeri786
Copy link
Collaborator

Added import app.

  • Verify file format.
  • Verify file size.
  • Can import multiple XML files with multiple post imports.
  • Can able to import post with metadata and categories.

Suggestion:

  • needs app icon
  • how can I redirect to apps? I want to redirect to post list after import complete.

Please check for quick view: https://www.screencast.com/t/oMIo0lrtN

@fastlinemedia
Copy link
Collaborator

fastlinemedia commented Apr 1, 2020 •
edited
Loading

Thanks, Shahid!

I did a test import and got this error...

Array to string conversion in /Server/wordpress/wp-content/plugins/assistant-git/backend/src/Controllers/PostsController.php on line 445

I believe that is because wp_upload_dir returns an array but you're using it as a string. Let me know when that's fixed and I'll continue testing.

For your questions...

needs app icon

Brent can get that going when this is a little closer to release.

how can I redirect to apps? I want to redirect to post list after import complete.

All of the main app components get passed the history object. That is a React Router object and can let you navigate to other apps using history.push( '/fl-content' ).

@brentjett can you still get history from Nav.Context as well?

@shahidajmeri786
Copy link
Collaborator Author

Thanks, Shahid!

I did a test import and got this error...

Array to string conversion in /Server/wordpress/wp-content/plugins/assistant-git/backend/src/Controllers/PostsController.php on line 445

I believe that is because wp_upload_dir returns an array but you're using it as a string. Let me know when that's fixed and I'll continue testing.

For your questions...

needs app icon

Brent can get that going when this is a little closer to release.

how can I redirect to apps? I want to redirect to post list after import complete.

All of the main app components get passed the history object. That is a React Router object and can let you navigate to other apps using history.push( '/fl-content' ).

@brentjett can you still get history from Nav.Context as well?

Hi @fastlinemedia ,

Thank you for the test. I checked and updated the string variable into array one. the funny part is my PHP server did not give the error at all :D

I also add the history object and pushed to a list of posts after import complete.

I pushed code so you will check it out for the further test.

@fastlinemedia
Copy link
Collaborator

Thanks, @shahidajmeri786! It looks like it's working. Only issue I noticed is that it's only importing one post. I exported two pages using the WordPress exporter but only one imports when using the Assistant importer. Can you check that out?

@shahidajmeri786
Copy link
Collaborator Author

Thanks, @shahidajmeri786! It looks like it's working. Only issue I noticed is that it's only importing one post. I exported two pages using the WordPress exporter but only one imports when using the Assistant importer. Can you check that out?

Hi @fastlinemedia ,

Thanks for testing. Multiple import issue fixed. code pushed. can you please check again?

@fastlinemedia
Copy link
Collaborator

@shahidajmeri786 I did some testing and ran into a few things. I also merged with master so be sure to npm install!

  1. I'm getting an error that says "Sorry, your file is too large." My max upload size should be enough. Here's a link to my export file if you want to try that.
  2. When there's an error it's still redirecting me to the post list. Maybe it should stay in the import app on error.
  3. Can you break out the import logic into its own REST controller? You'll see I already did that for the export logic. The post controller is getting quite large.

One question: does the import app handle importing attachments?

@shahidajmeri786
Fixed Large size upload issue.
c8200a9 
Fixed attachment import
Fixed error redirected to post list
Break import logic into separate controller.
@shahidajmeri786
Copy link
Collaborator Author

@shahidajmeri786 I did some testing and ran into a few things. I also merged with master so be sure to npm install!

  1. I'm getting an error that says "Sorry, your file is too large." My max upload size should be enough. Here's a link to my export file if you want to try that.
  2. When there's an error it's still redirecting me to the post list. Maybe it should stay in the import app on error.
  3. Can you break out the import logic into its own REST controller? You'll see I already did that for the export logic. The post controller is getting quite large.

One question: does the import app handle importing attachments?

  1. Fixed Large size upload issue.
  2. Fixed attachment import
  3. Fixed error redirected to post list
  4. Break import logic into a separate controller.

One question: does the import app handle importing attachments?
Answer: Yes now can able to import attachments too

@fastlinemedia
Copy link
Collaborator

fastlinemedia commented May 25, 2020 •
edited
Loading

Thanks, @shahidajmeri786! However, I'm getting an error when trying to import this file...

https://share.getcloudapp.com/04uP4KW4

There's nothing in the PHP error log, but the console is showing this...

POST http://localhost/wordpress/wp-json/fl-assistant/v1/posts/import net::ERR_EMPTY_RESPONSE

Can you check that out? Also be sure to pull, I merged in master.

@shahidajmeri786
Copy link
Collaborator Author

Hi @fastlinemedia ,

I tested and checked the import is working fine. check here: https://www.screencast.com/t/ahSFrLxL

there is an import message fix. added post type in the message so user know what successfully imported and what failed.

Can you please tell me your WordPress version which you tested?

@fastlinemedia
Copy link
Collaborator

Thanks, @shahidajmeri786! I just pulled and it seems to be working now. Looks like it's coming together! I have some code review notes I'll leave on the PR. Also, I'm seeing a few notices in the PHP error log now. Can you look into those?

[01-Jun-2020 17:54:03 UTC] PHP Notice:  Undefined index: DESCRIPTION in /wordpress/wp-content/plugins/assistant-git/backend/src/Controllers/PostsImportController.php on line 118
[01-Jun-2020 17:54:03 UTC] PHP Notice:  Undefined index: EXCERPT:ENCODED in /wordpress/wp-content/plugins/assistant-git/backend/src/Controllers/PostsImportController.php on line 119
[01-Jun-2020 17:54:03 UTC] PHP Notice:  Undefined index: CATEGORY in /wordpress/wp-content/plugins/assistant-git/backend/src/Controllers/PostsImportController.php on line 181
[01-Jun-2020 17:54:03 UTC] PHP Warning:  Illegal string offset 'WP:META_KEY' in /wordpress/wp-content/plugins/assistant-git/backend/src/Controllers/PostsImportController.php on line 186
[01-Jun-2020 17:54:03 UTC] PHP Warning:  Illegal string offset 'WP:META_VALUE' in /wordpress/wp-content/plugins/assistant-git/backend/src/Controllers/PostsImportController.php on line 187
[01-Jun-2020 17:54:03 UTC] PHP Warning:  Illegal string offset 'WP:META_KEY' in /wordpress/wp-content/plugins/assistant-git/backend/src/Controllers/PostsImportController.php on line 186
[01-Jun-2020 17:54:03 UTC] PHP Warning:  Illegal string offset 'WP:META_VALUE' in /wordpress/wp-content/plugins/assistant-git/backend/src/Controllers/PostsImportController.php on line 187
[01-Jun-2020 17:54:03 UTC] PHP Warning:  count(): Parameter must be an array or an object that implements Countable in /wordpress/wp-content/plugins/assistant-git/backend/src/Controllers/PostsImportController.php on line 193

fastlinemedia
fastlinemedia requested changes Jun 1, 2020
View reviewed changes
Copy link
Collaborator

@fastlinemedia fastlinemedia left a comment

Choose a reason for hiding this comment

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

Hey @shahidajmeri786 let me know if you have any questions about this review.

backend/src/Controllers/PostsImportController.php Outdated Show resolved Hide resolved
backend/src/Controllers/PostsImportController.php Outdated Show resolved Hide resolved
backend/src/Controllers/PostsImportController.php Outdated
return rest_ensure_response(
[
'error' => true,
'message' => 'Sorry, only XML files are allowed.',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you use WordPress translation functions (e.g __ or sprintf) anywhere there is human readable text? We need to be sure to always do that on the front or backend. There are a number of places that need it in this file.

backend/src/Controllers/PostsImportController.php Outdated Show resolved Hide resolved
backend/src/Controllers/PostsImportController.php Outdated

} else {
$fail_imp_count++;
return rest_ensure_response(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really want to cancel the entire import if an attachment fails? It might be better to fail silently for attachments and let the import proceed.

backend/src/Controllers/PostsImportController.php Outdated Show resolved Hide resolved
@shahidajmeri786
improvement of:
8f827fc 
Resolved some php warnings
Cleanup codelines which are unused
Removed Upload ok as not needed anymore
Added WP translate for alert text messages
Added Dynamic wp allowed file size
Fixed post import failed due to terms and metas
@shahidajmeri786
Copy link
Collaborator Author

Worked on feedbacks:
Resolved some PHP warnings.
Cleanup code lines which are unused
Removed unused variable
Added WP translate for alert text messages
Added Dynamic WP allowed file size
Fixed post-import failed due to terms and meta unset case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fastlinemedia fastlinemedia fastlinemedia requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@shahidajmeri786 @fastlinemedia