Codecov Report

Merging #1699 (1af44ce) into main (194ee01) will decrease coverage by 0.00%.
The diff coverage is 96.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 allows approver_ids and approved_by_ids to work for ONE 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 with create() to ensure we don't use key[] in payloads.

I think we need to transform "parameters'. Whether it is a GET/POST/PUT. Now handling values in the data 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 with create() to ensure we don't use key[] in payloads.

I think we need to transform "parameters'. Whether it is a GET/POST/PUT. Now handling values in the data 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.py
index 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 example scopes 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 the data was not being sent as params in the POST/PUT methods 😔

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