Export user preferences on project export

[kleintom]

I tested this locally with 1) default (reset) preferences, and 2)

{"layout"=>
  {"tasks::digitize::collectionObjects::hideBuffered"=>true,
   "tasks::digitize::collectionObjects::hideCatalogNumber"=>true,
   "tasks::digitize::LeftColumnOrder"=>["BiologicalAssociation", "TaxonDeterminationLayout", "TypeMaterial"],
   "tasks::extract::componentsOrder"=>["ByComponent", "MadeComponent", "ProtocolsComponent", "OriginComponent", "IdentifierComponent", "RepositoryComponent", "CustomAttributes"]},
 "disable_chime"=>false,
 "items_per_list_page"=>20,
 "taxon_name_autocomplete_redirects_to_valid"=>false,
 "default_hub_tab_order"=>["tasks", "data", "favorite"]}

Two caveats, please send back to me if there's any concern:

• I didn't try all of the preferences, maybe there are some that are more complicated?
• as far as I could tell the quoting gymnastics are necessary, but I didn't check/don't know if they do the right thing if there's already an escaped " in the preferences (since I'm expecting that never happens...)

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.78%. Comparing base (de5c9f0) to head (b764c70).
Report is 7 commits behind head on development.

❗ Current head b764c70 differs from pull request most recent head f994a57

Please upload reports for the commit f994a57 to get more accurate results.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@               Coverage Diff                @@
##           development    #3968       +/-   ##
================================================
+ Coverage        58.17%   72.78%   +14.60%     
================================================
  Files             1655     1935      +280     
  Lines            52473    66524    +14051     
================================================
+ Hits             30527    48419    +17892     
+ Misses           21946    18105     -3841     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[LocoDelAssembly]

Because of this (that I totally forgot about) then would be nice to add a warning that it is not a good idea at all to build a public TW instance out of the generated dump.

[LocoDelAssembly]

Have you tried preferences: "'#{conn.escape_string(user.preferences.to_json)}'"? I think ' is the char that needs to be escaped, not ".

3.3.1 :017 > puts ActiveRecord::Base.connection.raw_connection.escape_string(%["hello" 'bye'])
"hello" ''bye''

[LocoDelAssembly]

As general comment, my concern with this would be if there a potential to leak semi-sensitive (or private) information from other project members. If not a problem, after doing some revisions I think it is good to merge, otherwise I'd seek to make the user model tolerant to empty preferences and initialize them properly if so.

[LocoDelAssembly]

Correction, it would export preferences for all users, not just project members (although non-members have their email and name fields redacted).

[mjy]

I think we can review preferences to ensure nothing sensitive is there. There shouldn't be, if there is we need to strike it from there and handle it elsewhere. We should never store credentials, or any personal information beyond things like "I want the form layed out this way".

[jlpereira]

We don't save any sensitive information there, just layout preferences and copy paste shortcuts. Maybe we could add a checkbox option for this?
Like:

[x] Export user preferences

If it is not checked, initialize the users preferences. I'm not sure if export projects will always need export user preferences or not

[LocoDelAssembly]

The initial motivation for this I guess was that @kleintom experienced software crashes on some tasks because preferences were not initialized. If that is the main goal here, then I think would be best to instead fix the model so User's preferences attribute returns a default set of preferences if none are stored in the database or have an invalid structure.

[mjy]

. If that is the main goal here, then I think would be best to instead fix the model so User's preferences attribute returns a default set of preferences if none are stored in the databa

Yes, that was intended already. Might just be that we have to re-initialize a legal JSON base, not sure, '{}'

[kleintom]

The initial motivation for this I guess was that @kleintom experienced software crashes on some tasks because preferences were not initialized. If that is the main goal here, then I think would be best to instead fix the model so User's preferences attribute returns a default set of preferences if none are stored in the database or have an invalid structure.

Defaults are merged into any preferences being saved on before_save here:

taxonworks/app/models/user/preferences.rb

Line 8 in aa82f29

before_save :fill_preferences

taxonworks/app/models/user/preferences.rb

Lines 23 to 32 in aa82f29

	def fill_preferences
	if preferences.empty?
	reset_preferences
	else
	BASE_PREFERENCES.keys.each do |k|
	preferences[k] = BASE_PREFERENCES[k] if send(k).nil?
	end
	end
	true
	end

The issue is that in the export/import case, users are created at the pg level directly by psql, and the default preferences in that case are {}.

With those empty preferences we get exceptions like here:

taxonworks/app/javascript/vue/tasks/collecting_events/new_collecting_event/components/CollectingEventForm.vue

Line 85 in aa82f29

const store = this.preferences.layout[this.keyStorage]

where we try to do preferences.layout.

I don't think we can make the default for User preferences be BASE_PREFERENCES (can we?); if preferences are non-sensitive/will stay non-sensitive, then always importing preferences, for everyone, seems to me like the easiest solution here. (I'm not clear on why non-project users are exported, maybe so you're guaranteed to have an existing administrator account?) Other thoughts?

[kleintom]

Have you tried preferences: "'#{conn.escape_string(user.preferences.to_json)}'"?

Based on what I'm seeing online I expected that to work, but it didn't.

With the current release code, if I do select preferences from users (where I only have one user), it shows
"{\"layout\":{},\"disable_chime\":false,\"items_per_list_page\":20,\"taxon_name_autocomplete_redirects_to_valid\":false,\"default_hub_tab_order\":[\"tasks\",\"data\",\"favorite\"]}".

If I import using "'#{conn.escape_string(user.preferences.to_json)}'" and then do the select it shows {"layout":{},"disable_chime":false,"items_per_list_page":20,"taxon_name_autocomplete_redirects_to_valid":false,"default_hub_tab_order":["tasks","data","favorite"]}. (Which is what you would expect to see in a json field, no?)

But with that second imported preferences value, if I try to load a page that uses those preferences, or from the rails console I do User.first.preferences, I get an error no implicit conversion of Hash into String (TypeError) - that error hangs the app when you try to load a page that uses preferences.

It seems to expect the db value of preferences to be serialized json (which is what it appears to be with release code), not a json hash (though it's not clear to me in what sense the db value is a hash in this case). Schema.rb says t.json "preferences", default: {} for preferences.

If you update preferences to be {} (no quotes) it's fine with that, even though it's a "hash", not a double-quoted string...

I haven't been able to make sense of all of that yet; thoughts?

[LocoDelAssembly]

The initial motivation for this I guess was that @kleintom experienced software crashes on some tasks because preferences were not initialized. If that is the main goal here, then I think would be best to instead fix the model so User's preferences attribute returns a default set of preferences if none are stored in the database or have an invalid structure.

Defaults are merged into any preferences being saved on before_save here:

taxonworks/app/models/user/preferences.rb

Line 8 in aa82f29

before_save :fill_preferences

taxonworks/app/models/user/preferences.rb

Lines 23 to 32 in aa82f29

	def fill_preferences
	if preferences.empty?
	reset_preferences
	else
	BASE_PREFERENCES.keys.each do |k|
	preferences[k] = BASE_PREFERENCES[k] if send(k).nil?
	end
	end
	true
	end

The issue is that in the export/import case, users are created at the pg level directly by psql, and the default preferences in that case are {}.

In this case I would add this in user/preferences.rb

# Assuming this method will be instance method of User, if not the case then needs to be relocated (if possible)
def preferences
  prefs = read_attribute(:preferences)
  return prefs unless prefs.empty?

  reset_preferences
  read_attribute(:preferences)
end

What do you think? (cc @mjy)

With those empty preferences we get exceptions like here:

taxonworks/app/javascript/vue/tasks/collecting_events/new_collecting_event/components/CollectingEventForm.vue

Line 85 in aa82f29

const store = this.preferences.layout[this.keyStorage]

where we try to do preferences.layout.

I don't think we can make the default for User preferences be BASE_PREFERENCES (can we?); if preferences are non-sensitive/will stay non-sensitive, then always importing preferences, for everyone, seems to me like the easiest solution here. (I'm not clear on why non-project users are exported, maybe so you're guaranteed to have an existing administrator account?) Other thoughts?

Non project users are exported because housekeeping of community data might reference them.

[kleintom]

That fixes the exception issue, though it doesn't persist the new prefs to the database. I don't know, would you want to call save in the accessor in that case?

Do we want to not export preferences then? (Could still export plus add the reader for future-proofing in other export cases.)

[mjy]

would you want to call save in the accessor in that case?

No, never :).

[LocoDelAssembly]

Would be nice to figure out exactly how is ActiveRecord serializing the preferences attribute to make sure gsub is the correct solution. If we can be certain the export is done properly, then I guess it is fine to start exporting preferences. Would also include the custom getter even if the export includes preferences.

[mjy]

I remember battling this. I just realized this is a json not jsonb field, IIRC the latter was my preference. So first we should probalby unify all our json to one type. Can check migrations for pattern/comments.

[kleintom]

The issue with user preferences showing up in the database as double-quoted strings: "{\"layout\":{},\"disable_chime\":false,\"items_per_list_page\":20,\"taxon_name_autocomplete_redirects_to_valid\":false,\"default_hub_tab_order\":[\"tasks\",\"data\",\"favorite\"]}" seems to be because there's a store property defined on that field:

taxonworks/app/models/user/preferences.rb

Line 7 in 1263c7e

store :preferences, accessors: BASE_PREFERENCES.symbolize_keys.keys, coder: JSON

From https://api.rubyonrails.org/classes/ActiveRecord/Store.html "Store gives you a thin wrapper around serialize", and the coder: JSON allows the serialization to be stored on a JSON field.

Seems strange that you can't insert actual json into your json column anymore (and how does it work if you only have a store accessor on one of your many json keys?). In any case, the link suggests that if your underlying field is already json, then you should use store_accessor instead of store. I confirmed that does work: you can insert "normal" json such as '{"a": true}' and still read it as User.a. But if you make the switch to store_accessor without migrating your old serialized data then you get errors lik a 500 in user/preferences.json.jbuilder when you try to do json.merge! @user.preferences on a string.

So I'm skipping exporting user preferences for now on project export, but let me know if there's more you'd like me to work on here.

[kleintom]

Oops, I'll check out the errors.

[kleintom]

I modeled the json -> jsonb migration that I wrote on previous migrations doing the same thing:

taxonworks/db/migrate/20180322031114_sqed_depiction_hstore_to_jsonb.rb

Lines 3 to 4 in 1263c7e

	change_column :sqed_depictions, :metadata_map, :jsonb, using: 'metadata_map::jsonb', null: false, default: '{}'
	change_column :sqed_depictions, :specimen_coordinates, :jsonb, using: 'specimen_coordinates::jsonb', null: false, default: '{}'

taxonworks/db/migrate/20180529170201_update_project_workbench_settings_to_preferences_jsonb.rb

Line 3 in 1263c7e

change_column :projects, :workbench_settings, :jsonb, using: 'workbench_settings::jsonb', null: false, default: '{}'

Note the default set to '{}' and not {} in all cases.

Most of the failing tests from my previous push are at

taxonworks/app/models/user.rb

Line 397 in 1263c7e

fp['recently_visited'] ||= []

The issue seems to be that we're trying to read the value of the footprints hash with key recently_visited when the current value of the hash is "{}" (not {}). This would work if footprints was a stored field, but it's not (as determined by User.stored_attributes). So in this case the default value of footprints should be {} and not '{}'. (Yuk.) Doing that fixes those tests. (Thank goodness for the tests!)

The Project migration linked above (which also renames workbench_settings to preferences) should be fine/correct because preferences in Project is also stored.

The sqed_depiction migration I think might be in error. specimen_coordinates is currently unused and unaccessed, so no errors there, but if you create two sqed depictions and then visit /tasks/accessions/breakdown/buffered_data/thumb_navigator/2.html you get a 500 on undefined method 'inject' for an instance of String here:

taxonworks/app/models/sqed_depiction.rb

Line 218 in 1263c7e

metadata_map.inject({}){|hsh, i| hsh.merge(i[0].to_i => i[1].to_sym)}

because metadata_map is the string "{}" (the default value) and you're trying to do an inject on it. If I change those default metadata_map to '{}' instead of "{}" then the page loads without error.

So I think the way forward is to do another migration to:

• migrate user :footprint's default to {} (not '{}')
• migrate sqed_depiction's metadata_map and specimen_coordinates defaults to {} (not '{}')
• maybe migrate all existing sqed_depiction metadata_map and specimen_coordinates default values already in the database to {} (not '{}').

I can keep working on this - in which case let me know if everything sounds right - but it's a little hinky with the quotes vs unquotes and now the migrations are changing more, so feel free to take it if that works better.