api: fix or not fix the aspect ratio

[egabancho]

This commit 1fac6ee attempted to fix the problem with the aspect ratios which are not in the configuration.

I would either:

• Create a decorator which we can use in every single function which gets display_aspect_ratio as parameter and it will change it to the correct one if needed.
  Take it with a grain of salt, probably we need to order correctly the function arguments to make it work or use the display_aspect_ratio always as a keyword argument.

def fix_aspect_ratio(f):
    def wrapper(*args, **kwargs):
        kwargs['display_aspect_ratio'] = get_closest_aspect_ratio()
        return f(*args, **kwargs)
    return wrapper

• let if fail if we pass the wrong aspect ratio and make the "client", in our case webhooks/tasks.py, handle the error (or maybe send already the correct aspect ratio).

• or use the function we have already and call it every time we have an aspect ratio related calculation.

--

There is also a problem with the commit itself, this is how it should be otherwise it will return the wrong type of object

-        raise InvalidAspectRatioError(display_aspect_ratio)
+        if max_height and max_width:
+            display_aspect_ratio = get_closest_aspect_ratio(max_height, max_width)
+            aspect_ratio = current_app.config['CDS_SORENSON_PRESETS'][
+                display_aspect_ratio]
+        else:
+            raise InvalidAspectRatioError(display_aspect_ratio)

[zzacharo]

@egabancho I will check it and yes the commit needs fixing! Sorry for that, I missed it.

[zzacharo]

Actually if I fix only the missing line on my last commit your hack https://github.com/CERNDocumentServer/cds-videos/blob/a6891098800954ae316e4e38222f352bfe672a99/cds/modules/webhooks/tasks.py#L589 is not needed. Is there any other place in your knowledge that we need the aspect ratio other than inside the get_preset_id function? In my opinion is if someone needs the closest aspect ratio in a different case to call it explicitly to avoid going around the code and search what happens.

[egabancho]

I think it is still needed, otherwise it will fail here https://github.com/CERNDocumentServer/cds-videos/blob/cdslabs/cds/modules/webhooks/tasks.py#L630

@switowski what do you think?

[zzacharo]

@egabancho ok in that case we can abstract it a bit.

[egabancho]

I don't understand, I don't think it does not correct the aspect ratio https://github.com/CERNDocumentServer/cds-sorenson/blob/master/cds_sorenson/api.py#L227.

That's why I opened this issue, to decide whether we correct the aspect ratio before calling any of the API functions or we correct it inside the API.

[zzacharo]

I think that fixing it inside is safer from the point of refactoring, but let's wait what the others think also.

[switowski]

Can we discuss it IRL once @egabancho is back? I don't really follow all this aspect ratio problems and I don't like where this is going (patch on top of other patches).