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

Bulk status change: Fix bulk rejection #1486

Merged
merged 2 commits into from
Sep 19, 2022
Merged

Bulk status change: Fix bulk rejection #1486

merged 2 commits into from
Sep 19, 2022

Conversation

dd32
Copy link
Contributor

@dd32 dd32 commented Sep 19, 2022 •
edited
Loading

What?

As a follow up to #1451, it looks like the action POST variable was looked at incorrectly, causing a PHP Notice:

E_NOTICE: Undefined variable: new_status in wp-content/plugins/glotpress/gp-includes/routes/translation.php:438

Based on https://github.com/amieiro/GlotPress-WP/blob/develop/gp-includes/routes/translation.php#L375-L381 it looks like the correct is reject, which is confirmed from the POST data attached to the above notice containing 'reject'.

I believe this is potentially causing bulk rejections to fail, based on re-attempts after failure.

Additionally, $new_status should probably be pre-set to rejected before the switch.

Why?

How?

Testing Instructions

Screenshots or screencast

@dd32 dd32 added the [Type] Bug An existing feature is broken. label Sep 19, 2022
@dd32 dd32 requested a review from amieiro September 19, 2022 04:48
@dd32 dd32 changed the title Use the correct action name Bulk status change: Fix bulk rejection Sep 19, 2022
@dd32
Copy link
Contributor Author

dd32 commented Sep 19, 2022

It appears this is probably the underlying cause of https://meta.trac.wordpress.org/ticket/6488 as it'll have set the Database rows to have an empty translation status field.

amieiro
amieiro approved these changes Sep 19, 2022
View reviewed changes
Copy link
Member

@amieiro amieiro left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I have modified another lines, because we have the same error in the "_bulk_approve" function.

@amieiro amieiro merged commit 1022594 into develop Sep 19, 2022
@amieiro amieiro deleted the fix/1451-reject branch September 19, 2022 12:02
@NekoJonez
Copy link
Contributor

Related: https://meta.trac.wordpress.org/ticket/6488

@ocean90 ocean90 added this to the 4.0 milestone Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amieiro amieiro amieiro approved these changes

Assignees

@amieiro amieiro

Labels
[Type] Bug An existing feature is broken.
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
4 participants
@dd32 @NekoJonez @amieiro @ocean90