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

embl taxonomy plugin sync fixes #1287

Merged
merged 6 commits into from
Oct 19, 2023
Merged

embl taxonomy plugin sync fixes #1287

merged 6 commits into from
Oct 19, 2023

Conversation

dbushell
Copy link
Collaborator

This should stop the timeout issue #520

@sandykadam
Copy link
Collaborator

Thanks @dbushell
This looks very good improvement. Do you think it is good to show the count of records processing in popup window.

Also on other note, if you remember we built the microsite dyanamically whenever there is new website or if we need to update latest version. We have deployment scripts where we use wpcli commands to do all those changes required for deployment.

  • With your above change, will following command still work.
  • Is there way we can have custom wpcli command to update taxonomy from cli, which will also work in similar batch process way.
# Configure taxonomy
echo "=== Taxonomy ==="
echo "Taxonomy - configuring"
wp option update embl_taxonomy $VF_API_URL"pattern.json?pattern=embl-ontology&source=contenthub"
wp option update _options_embl_taxonomy field_embl_taxonomy
wp option update _options_embl_taxonomy_term_who field_embl_taxonomy_term_who
wp option update _options_embl_taxonomy_term_what field_embl_taxonomy_term_what
wp option update _options_embl_taxonomy_term_where field_embl_taxonomy_term_where

# Sync with the latest taxonomy from the contentHub
echo "Updating Taxonomy from ContentHub"
wp eval 'embl_taxonomy()->register->sync_taxonomy();'

@dbushell
Copy link
Collaborator Author

Hi @sandykadam yes I will add some kind of UI update to show a progress number

The wp option update commands will still work - nothing has changed there

wp eval 'embl_taxonomy()->register->sync_taxonomy();

This one wont work anymore.

Do you know if the bash script was timing out? Or was the timeout just syncing from the WP admin?

If the bash script synced without any timeouts I can add this back quickly so it continues to work the same.

Otherwise we'd have to write a new script to perform multi requests. I need to update the plugin first to allow that.

@dbushell
Copy link
Collaborator Author

Actually it would be easier if I wrote a PHP script to handle the multiple sync requests, then the bash part can remain a single wp eval

but if it was never timing out here I can revert to the old method quickly

@sandykadam
Copy link
Collaborator

Yes this is working fine no issues wp eval 'embl_taxonomy()->register->sync_taxonomy();'

@dbushell
Copy link
Collaborator Author

Yes this is working fine no issues wp eval 'embl_taxonomy()->register->sync_taxonomy();'

@sandykadam Just to clarify, I mean before this PR, was the old version syncing without timeout?

with this PR the sync will silently fail at the moment (I think) it won't complete the whole sync anyway

@sandykadam
Copy link
Collaborator

Yes this is working fine no issues wp eval 'embl_taxonomy()->register->sync_taxonomy();'

@sandykadam Just to clarify, I mean before this PR, was the old version syncing without timeout?

with this PR the sync will silently fail at the moment (I think) it won't complete the whole sync anyway

Yes from CLI it was working, it was only the admin UI was timing out, which I think will get fix with your batch process.

@dbushell
Copy link
Collaborator Author

The CLI sync now works like before:
wp eval 'embl_taxonomy()->register->sync_taxonomy();'

I've added sync progress:
sync-progress

And term select should now be filtered by the parent term:
embl-terms-filter

@kasprzyk-sz
Copy link
Contributor

Hey @david, I noticed that for the pages we have two fields for taxonomy. One at the bottom and one on the sidebar:
Screenshot 2023-10-16 at 15 22 21

For pages, I think the bottom one is used to overwrite default breadcrumbs, right @sandeep?

For the posts, currently there is only the one on the sidebar. We use this field to tag articles with relevant terms.
Screenshot 2023-10-16 at 15 22 45

Would you be able to improve it in the same way as the one on pages and move it to the bottom?

@sandykadam
Copy link
Collaborator

sandykadam commented Oct 16, 2023

I was checking this change on local, I can see progress bar, cli change but I don't see the UI change for the taxonomy fields in settings page.
Screenshot 2023-10-16 at 14 54 13

@dbushell
Copy link
Collaborator Author

@kasprzyk-sz I'm not sure that sidebar panel should be there. It certainly shouldn't say "ADD NEW..." because it's not possible to add new. Can you remember if this was always visible?

@kasprzyk-sz
Copy link
Contributor

kasprzyk-sz commented Oct 16, 2023

@dbushell yes, it was always there. We use this one though only for tagging posts on the news theme. I think @sandykadam meant actually this picker.
Screenshot 2023-10-16 at 16 02 56

@dbushell
Copy link
Collaborator Author

@sandykadam it should look like this and be filtered by Who/What/Where:

settings

If it's not updating can you check this page:

/wp-admin/edit.php?post_type=acf-field-group

and if "EMBL Taxonomy Terms" is listed under the default "All" view then hover over it and click "Trash"

acf-settings

it should then load properly from the JSON config file. Those settings don't need to be sync to the database, only when editing them.

@dbushell
Copy link
Collaborator Author

@kasprzyk-sz ah that one I haven't changed. It's just the default tag picker. There's no way to improve that.

It would be possible to make an alternate UI using ACF to manage tags, but it'd take a while to develop and test.

@kasprzyk-sz kasprzyk-sz merged commit aa4c1b1 into master Oct 19, 2023
2 checks passed
@kasprzyk-sz kasprzyk-sz deleted the fix/taxonomy-sync branch October 19, 2023 08:22
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.

3 participants