Convert active record queries to ARel

[dmitry]

With this change it will be possible to use JOINs and WHEREs separately in custom SQLs, and it will allow to change most_recent_ prefix in real time, rather than fixing with static name on a class level.

[dmitry]

@thom-oman can you please check this PR?

[thom-oman]

Slight outside of the diff but can we ensure that this method hasn't changed, from memory this is covered by tests though

[dmitry]

@thom-oman Do you mean it should return SQL string? Arel is more powerful, but I understand your point. Do you have any idea how we can return non-sql version as well, to make it possible to change via arel public methods?

[dmitry]

Lets decide whether you want to use SQL string everywhere or arel in some cases.

[thom-oman]

My concern is that it'll be a breaking change for those expecting this method to return a string. One approach would be to have an optional parameter that returns arel, but defaults to string. Not sold on that idea though haha.

[dmitry]

Another solution is to add breaking change to the CHANGELOG. I think in 95% of the cases it will still work with arel, as those objects are still usable in where, having and joins. I don't think anyone concatenated string of a JOIN.

[thom-oman]

Yeah I don't think it will be a concern for many, I can't think of many uses but I'm just thinking of Hyrums laws and that not everyone checks the change log before bumping gems.

Also I just checked and this would break something in the GoCardless API, only one instance but you can assume it would break for others. Hence we should avoid making this a breaking change, don't think this justifies a major bump either.

[dmitry]

ARel considered as private interface for activerecord, so I don't think it should be publicly accessible by the Hyrums law. Going to make it to return SQL with a .to_sql call, so it will not to brake anyone code. Rebased with push force.

[thom-oman]

hey @dmitry sorry for the delay, not been able to dedicate much time to Statesman! This looks great, thanks for opening a PR. First thing, can we fix the Rubocop errors so that we can see that the tests pass (I need to fix this if I can find the time), my Arel isn't that great. One small concern I have is adding in_state_conditions that returns Arel to the public interface, though I'm undecided how strongly I feel that.

[dmitry]

@thom-oman tried to fix as many rubocop as I can. Regarding the most_recent_transition_join, in_state_conditions and not_in_state_conditions they are returning an arel values, and in some situations it might be really nice to have this possibility, especially when you are using sub-queries and you want to add alias for the table. In this case it will be possible to do that. Can be written .to_sql, but for my use case arel will be really useful (the reason I made this change actually: in real app I need to make a subquery with an alias in FROM command).

[dmitry]

@thom-oman just let me know what should I do for the next steps.

[thom-oman]

hey @dmitry sorry again for the slow responses, not been able to dedicate much time to this.

There are still a few rubocop issues that prevent us from seeing how the tests are looking. Though right now, I can't see any behaviour change. The proposed benefit is to allow the join transitions to use arel and the flexibility that brings. Would you be able to show how this would be used from the developer side? I think you mean that most_recent_transition_join could be used? This is why I was suggesting that we maybe allow that method to return both arel (for flexibility) and the SQL string (to avoid breaking the current API).

I also don't think we should consider arel a private interface of Rails since Statesman is calling it directly and I think many people still use it when it makes more sense vs AR.

Does this make sense?