Fix problem in registering and querying custom rendition model

[engAmirEng]

This feature implemented for (#16) and wasn't working up to now and the issue (#311) is refrencing this. As I was exploring this I found some serious bugs and architectural Issues that I'm going to open later.

[engAmirEng]

@zerolab Do you know about this CI failure?

[zerolab]

It is a latest Wagtail main branch issue. Will sort it out. In the meantime, any chance you can add a test?

[engAmirEng]

It is a latest Wagtail main branch issue. Will sort it out. In the meantime, any chance you can add a test?

That's kinda tricky, until now rendition model was beeing registered with register_image_model instead of register_image_rendition_model when the model's app is in the APPS setting and it worked fine Because of #313 , ironic hah? I found the issue when I tried to call register_image_rendition_model myself. Plus writing tests is not my strength

[dopry]

@engAmirEng It's important we try to get tests in to avoid regressions. Tests might have prevented this bug in the first place. Admittedly the test in grapple are not well organized at the moment and are fully dependent on the example app.

I think there are two things we would want to test.

1. Is there an image rendition type in the schema?

   we should add a test_graphql_schema.py in example/home/test. From on of my projects I have https://gist.github.com/dopry/37c6bee35eec0d28483ad8a29fa793bf that could serve as a template.

2. Can we still query renditions?

   I think this could be added in https://github.com/torchbox/wagtail-grapple/blob/main/example/home/test/test_blog.py in test_blog_body_imagechooserblock and test_blog_body_imagechooserblock_in_streamblock to ensure renditions can be queried in both contexts.

I'm not sure if testing should hold up this particular PR given it doesn't already have test coverage and we're fixing a pretty obvious production bug. It would be great if you could take a stab at the test. Ultimately whether tests are required is @zerolab's call.

[zerolab]

I feel this sort of bug fixes should at least attempt to add a few tests, especially if there's none. Otherwise it risks falling through the cracks

[engAmirEng]

I'll take care of that, might take some time

[dopry]

@engAmirEng can you rebase your branch, I want to see if the pre-commit tests are still failing.

[dopry]

@engAmirEng It's awesome the existing tests are passing. The last thing blocking this getting in is some tests. Do you have time to work on those? I'd love to get this in along with the hostname/multitenancy work you've been doing.

[engAmirEng]

@engAmirEng It's awesome the existing tests are passing. The last thing blocking this getting in is some tests. Do you have time to work on those? I'd love to get this in along with the hostname/multitenancy work you've been doing.

Honestly writing tests are not my strong suit, I tried to write the test you are asking for but I couldn't figure it out

[dopry]

I understand it can be difficult to get used to writing tests... Normally the approach I take is to write a test with the old code to demonstrate that the thing I am fixing is broken. Then I develop my code against that test. Would you be open to some guidance on the test development, and/or another contributor submitting tests to your branch?

[engAmirEng]

I understand it can be difficult to get used to writing tests... Normally the approach I take is to write a test with the old code to demonstrate that the thing I am fixing is broken. Then I develop my code against that test. Would you be open to some guidance on the test development, and/or another contributor submitting tests to your branch?

I will give it another shot today and give you the feedback

[zerolab]

I've folded this into #337 Thank you @engAmirEng

[engAmirEng]

I've folded this into #337 Thank you @engAmirEng

Happy to see the repo is going forward