REFACTOR: Migrate InstagramAuthenticator to use ManagedAuthenticator (PR #7081)

GitHub

You’ve signed the CLA, jjaffeux. Thank you! This pull request is ready for review.

@davidtaylorhq Will give it another look tomorrow, but I think this is already working well.

Can we move this to before to a let(:user) above the it .., I can see we are using another fabricate(:user) below.

It will be great to have a description here for this piece of work, just in case someone would like to get a little context of where this all fits, or was looking at this PR to gain some historical context.

Same as above, I think we might use want to fabricate for user1 once at the top of this spec.

Should this be_empty instead of eq("")?

I’m kind of reluctant to do this, I followed the same patterns we have in related tests files for other authenticators

same comment it’s same pattern we have in all other specs of this kind. If we want to change this, let’s change it all in one go after this PR. We could probably refactor it to be kind of agnostic.

@khalilovcmded the context here is https://meta.discourse.org/t/future-social-authentication-improvements/94691/3

In terms of tests, I think there is very little point testing every different provider to such a large extent - we end up just testing ManagedAuthenticator repeatedly. We’re not even testing the omniauth parts of the authenticators, we trust they are tested upstream. However, I agree @jjaffeux that we can tidy this up later in one go. I’ll add it to the todo list on Meta

Looks great to me :tada: