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

376: Schema: Disallow NULL for the `original_id`, `translation_set_id`, and `project_id` fields #420

Open
wants to merge 3 commits into
base: develop
from

Conversation

Projects
None yet
2 participants
@ocean90
Member

ocean90 commented Apr 20, 2016

[WIP]

See #376 and #376 (comment).

There is currently one failing unit test:

1) GP_Test_Thing_Translation_set::test_copy_translations_from_should_copy_into_empty_set

/www/wp-develop/svn/tests/phpunit/includes/testcase.php:394
/www/glotpress-plugin/wordpress/wp-content/plugins/glotpress/tests/phpunit/testcases/tests_things/test_thing_translation_set.php:14

@ocean90 ocean90 changed the title from Schema: Disallow NULL for the `original_id`, `translation_set_id`, and `project_id` fields to 376: Schema: Disallow NULL for the `original_id`, `translation_set_id`, and `project_id` fields Apr 20, 2016

@@ -27,7 +27,7 @@
*/
define( 'GP_VERSION', '2.1.0-alpha' );
define( 'GP_DB_VERSION', '940' );
define( 'GP_DB_VERSION', '941' );

This comment has been minimized.

@toolstack

toolstack Apr 21, 2016

Contributor

Since #410 is going to bump this to 950 in 2.0.1, should this be 951?

@toolstack

This comment has been minimized.

Contributor

toolstack commented Apr 28, 2016

The test is failing because the factory call for creating a translation does not fully populate the translation object, only the original (see https://github.com/GlotPress/GlotPress-WP/blob/develop/tests/phpunit/lib/factory.php#L77).

A quick fix is to add the original_id field in as well like so:

$this->default_generation_definitions = array(
    'translation_0' => new GP_UnitTest_Generator_Sequence( 'Translation %s' ),
    'original_id' => 0,
);

@toolstack toolstack force-pushed the 376-update-schema-default-values branch from 244d08d to e38fd47 Apr 28, 2016

@ocean90 ocean90 force-pushed the 376-update-schema-default-values branch from 3b9a413 to da97125 May 22, 2016

@ocean90

This comment has been minimized.

Member

ocean90 commented May 22, 2016

This schema change invalidates the indices of #421, so we have to make sure that this runs first.

@toolstack

This comment has been minimized.

Contributor

toolstack commented May 24, 2016

I switched back and forth between develop, this PR and develop again (and fudging the db version number) and the index updated each time correctly.

@ocean90 ocean90 self-assigned this Jun 7, 2016

@ocean90 ocean90 force-pushed the 376-update-schema-default-values branch from da97125 to 6e4e83f Jun 7, 2016

@ocean90

This comment has been minimized.

Member

ocean90 commented Jun 7, 2016

It looks like dbDelta() doesn't support such schema changes and the current updates are only caused by accident, see #461.

@ocean90

This comment has been minimized.

Member

ocean90 commented Jun 7, 2016

PR needs a refresh to add custom ALTER TABLE queries to gp_upgrade_data().

@toolstack toolstack force-pushed the 376-update-schema-default-values branch from 6e4e83f to 76c2255 Jun 20, 2016

@ocean90

This comment has been minimized.

Member

ocean90 commented Jun 23, 2016

@toolstack Look like the schema change got lost during the rebase.

@toolstack

This comment has been minimized.

Contributor

toolstack commented Jun 23, 2016

Doh! I double checked it I guess I need to triple check it :(

@ocean90 ocean90 modified the milestones: 2.2, 2.1 Jun 29, 2016

@toolstack toolstack force-pushed the 376-update-schema-default-values branch from cdd64e1 to 4d982c9 Jul 12, 2016

@ocean90 ocean90 modified the milestones: Future, 2.2 Sep 6, 2016

@ocean90 ocean90 removed their assignment Jun 4, 2018

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