Fix bug affecting sanitization version of sync tables #15885
Conversation
| @@ -268,8 +268,7 @@ def import_cleanup(schema_name, table_name) | |||
| # but is kept because there may be existing syncs for which this double sanitization | |||
| # (version 1 sanitization which wasn't idemponent) had the effect of altering some | |||
| # satinizated names (e.g. __1 -> _1). | |||
| table = ::Table.new(name: @table_name, user_id: @user.id) | |||
| # we could as well: table = Carto::UserTable.find(@user.tables.where(name: @table_name).first.id).service | |||
| table = Carto::UserTable.find(@user.tables.where(name: @table_name).first.id).service | |||
There was a problem hiding this comment.
Could you replace:
@user.tables.where(name: @table_name).first.idper:
@user.tables.find_by(name: @table_name).idIt's a bit more idiomatic 🙏
There was a problem hiding this comment.
This change can affect some users with sync tables and break their visualizations, since column names where uppercase letters have been replaced by underscores will change. But we can prevent the users we know about (in the clubhouse ticket) and I think the alternative, to create a new sanitization version and preserve the odd behaviour in the current one, is worse.
There was a problem hiding this comment.
I thougt @user.tables returned an AR relationship, but per what I see in the CI log I think i was wrong:
Step #9: undefined method `find_by' for #<#<Class:0x000055f7aaf4eaf0>:0x000055f7aa1d9460>
There was a problem hiding this comment.
Yes, I had just found that: I tested it in a console and it worked because I had an AR Carto::User, but the tests seem to use some Sequel ::User, and I guess we should cover all bases.
There was a problem hiding this comment.
I'm going to leave the original @user.tables.where(name: @table_name).first.id even if ugly because it seems to work regardless of @user being AR or Sequel, and because we may have more customers affected
This reverts commit 6d1a878. The reason is that the change didn't work if user was a Sequel object (::User) rather than an ActiveRecord (Carto::User).
|
Now we have: |
|
Step #9: Failures: |
jgoizueta
deleted the
feature/ch108990/geoagro-admin-problem-with-the-field-name
branch
See https://app.clubhouse.io/cartoteam/story/108990
In connector syncs, when sanitization is (redundantly, see #15231 (comment)) applied, the applied sanitization version was being fetched by a new
Tableobject for which a new, unsavedDataImportwas generated, and that data import was used to obtain the sanitization version. Until a DataImport is actually created, the version defaults to the legacy one, so that's we were getting