API lacks done/left strings progress count

[sushkov]

Without this, people might download a project .mo file that has 0 localized strings, resulting into a completely broken UI that uses localization.

I think var $non_db_field_names might be a more appropriate place for these. $field_names specifically applies to DB names, and gets used in various DB methods such as prepare_fields_for_save().

There is a GP_Thing::fields() method that the API endpoint uses to figure out which fields go in. I can't find it used anywhere but in gp_array_of_things_to_json(), and its name is not DB-specific, so we should be able to make a change here.

Patch attached. Let's see what Nikolay thinks.

Agree!
I will take a look at tests to cover this, since I planed this for a while already.

Thanks.

If you add it to $non_db_fields it won't get into the JSON representation.

Here, I think we can add a method to GP_Thing which returns its API representation and defaults to return $this->fields(), but each thing can override it. This way we can balance between the fields, the non_db_fields and custom stuff without putting the logic outside of the thing itself.

I added the patch to cover a basic testcase.

Since this is the first patch to introduce unit tests for API response, I also felt like adding some basic assertions would be handy, please review those.

Thanks.

1. The assertion method are over-complicated and their usage is cryptic. It's not at all obvious what does this mean:

   $this->assertApiResponseContainsAt( 0, 'name', $this->set->project->name );
   

It would be better if you just have a helper function to retrieve you the JSON-decoded response (and check for bad responses) and do the checks manually:

$response = $this->get_response();
$this->assertEquals( $this->set->project->name, $response[0]['name'] );

This way it's both obvious what you are checking for and you'll get the assertion error in a lot more accessible place in the code, not buried deep in an assertion method.

1. I would use API instead of Api in all identifiers.

1. The tests are very fragile. All this fields names might change and this would invalidate your tests without good reason. Your goal is to test if you get the same via the API as in the real request.

1. Every test method should test for something very specific and spell it out in the method name. I know the GlotPress test-base doesn't follow this rule all the time (I was lame in the beginning, I learned later), but have a look at the latest tests added like in ​test_urls.php. This helps with two things:
   1. If something breaks, you will have very specific information of what exactly broke and why it broke.
   2. It forces you to define what exactly are you testing for, which forces you to define you functionality better.

• I wouldn't say it's cryptic (compared to lets say [1]). I will refactor this, my main reason to add that helper to have as little code inside a test as possible. Tbh, your example is not good too, since the tests should validate the presence of the key first, than it's value (I'm testing the structure of the response mainly, secondary the value).

• Agree on API vs Api.

• Not agree, simply because my patch is not meant to cover the whole API specifications! I needed to add a test for this ticket. You are free to remove the testcases you believe are fragile

• Agree on method name, though not sure if the context I'm testing is worth splitting a bunch of tests into smaller ones since this is more an integration test, where a project API response should validate those assertions.

[1]: ​https://github.com/buddypress/glotpress/blob/master/t/lib/testcase-route.php#L51

• The cryptic part are the indices, then names, then values. It's very logical when you write it, but for me reading it, it was quite hard to get it in the beginning.
• I agree some of the methods in testcase-route.php do not have the best names :-)
• With a check like $this->assertEquals( …, $response[0]['name'] ); we check for both value and structure. You can't get the right value without having the right structure. What does your approach add?
• On the fragility: what will happen if we rename a field in the translation set? Our code doesn't depend on those, why should the tests depend?
• After having a second look, I agree that in this case separating the checks wouldn't have any benefit.

Thanks for reviewing it again.
I updated the patch, hopefully its ok now :)

(In [740]) Adding current_count, untranslated_count and waiting_count to API. Props nacin and sushkov. Fixes #163