Codecov Report

Merging#1699 (1af44ce) intomain (194ee01) willdecrease coverage by0.00%.
The diff coverage is96.42%.

@@            Coverage Diff             @@##             main    #1699      +/-   ##==========================================- Coverage   95.54%   95.53%   -0.01%==========================================  Files          81       81                Lines        5296     5309      +13     ==========================================+ Hits         5060     5072      +12- Misses        236      237       +1

Flags with carried forward coverage won't be shown.Click here to find out more.

So in my testing this does not break anything. It also allowsapprover_ids andapproved_by_ids to work forONE specified user (before it just failed) in listing merge requests. But it does not work for two or more, as it just returns the match for the first one.

I'm not a fan of the silent failure as that is likely worse than current situation where it dies.

@nejch This is ready for review now. Interested to hear your feedback. Thanks.

Should we try to revive this one@JohnVillalovos? Seems like an important fix. Probably fixes a few more issues down the line, will need to check those.

Should we try to revive this one@JohnVillalovos? Seems like an important fix. Probably fixes a few more issues down the line, will need to check those.

Yeah, probably. Let me see if I get some time this weekend. Thanks for reminding me!

@nejch It is ready for review again. Thanks for your patience.

I have to take that back - seems like this also transforms the key for POST/PUT payloads which shouldn't happen. This maybe needs a test withcreate() to ensure we don't usekey[] in payloads.

I think we need to transform "parameters'. Whether it is a GET/POST/PUT. Now handling values in thedata portion is a different thing we need to figure out.

I have to take that back - seems like this also transforms the key for POST/PUT payloads which shouldn't happen. This maybe needs a test withcreate() to ensure we don't usekey[] in payloads.

I think we need to transform "parameters'. Whether it is a GET/POST/PUT. Now handling values in thedata portion is a different thing we need to figure out.

Yes, that's right :) sorry for the confusion. payload = no transform, query params = transform.

@JohnVillalovos I think this doesn't work quite right yet. Can you try this test in this patch?

diff --git a/tests/unit/mixins/test_mixin_methods.py b/tests/unit/mixins/test_mixin_methods.pyindex 69cdb78..e8870b9 100644--- a/tests/unit/mixins/test_mixin_methods.py+++ b/tests/unit/mixins/test_mixin_methods.py@@ -314,6 +314,26 @@ def test_create_mixin_custom_path(gl):     assert responses.assert_call_count(url, 1) is True++@responses.activate+def test_create_mixin_with_attributes(gl):+    class M(CreateMixin, FakeManager):+        _types = {"my_array": gl_types.ArrayAttribute}++    url = "http://localhost/api/v4/tests"+    responses.add(+        method=responses.POST,+        headers={},+        url=url,+        json={},+        status=200,+        match=[responses.matchers.json_params_matcher({"my_array": ["1", "2", "3"]})],+    )++    mgr = M(gl)+    mgr.create({"my_array": [1, 2, 3]})++ def test_update_mixin_missing_attrs(gl):     class M(UpdateMixin, FakeManager):         _update_attrs = gl_types.RequiredOptional(

The reason we might want this is, for examplescopes as an array in create mixins for#780.

@JohnVillalovos I think this doesn't work quite right yet. Can you try this test in this patch?

Thanks@nejch I totally missed that thedata was not being sent asparams in the POST/PUT methods 😔

Updated the code to only modify the data for thelist() method.