Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Activity
  • Jan 23 08:37
    sedubois commented #385
  • Jan 23 05:25
    Dragonicity commented #385
  • Jan 21 15:19
    bruno-berchielli opened #555
  • Jan 14 12:48
    shioyama commented #554
  • Jan 14 12:48
    shioyama closed #554
  • Jan 14 06:40
    michaelvu812 edited #554
  • Jan 14 06:39
    michaelvu812 opened #554
  • Jan 09 20:06
    machty opened #21
  • Dec 24 2021 06:01
    shioyama closed #549
  • Dec 24 2021 06:01
    shioyama commented #549
  • Dec 24 2021 06:00
    shioyama commented #552
  • Dec 24 2021 06:00
    shioyama closed #552
  • Dec 24 2021 06:00
    shioyama commented #552
  • Dec 24 2021 05:45
    shioyama commented #553
  • Dec 24 2021 05:44
    shioyama commented #553
  • Dec 24 2021 05:43
    shioyama commented #553
  • Dec 24 2021 05:43
    shioyama closed #553
  • Dec 24 2021 05:43
    shioyama commented #553
  • Dec 24 2021 05:38
    shioyama labeled #553
  • Dec 24 2021 05:38
    shioyama labeled #552
Bram Jetten
@Bramjetten
No arguments
I'm seeing these errors in a lot of places in my apps
I reverted all of our apps to 0.8.11
Tim Krins
@timkrins
hmm weird. sorry I can't help more. that change in itself I really would not expect anything to happen.
you could try and find out what the second argument that is being passed in actually is.
because that error seems to me to indicate that options in the fallthrough accessor actually has some value (otherwise it would be empty and effectively would not be passed to the super)
Bram Jetten
@Bramjetten
Yes, I'll try to figure it out
Bram Jetten
@Bramjetten
I modified _read_attribute to show me what's being passed:
Schermafbeelding 2020-05-20 om 13.49.03.png
Schermafbeelding 2020-05-20 om 13.49.10.png
Happens everywhere I set attributes using AR select
Another example:
Bram Jetten
@Bramjetten
@products.select("COUNT(*) AS count_all, spina_shop_products.properties->'manufacturer' AS filter").group("filter")
This query uses jsonb, but seems unrelated to the issue
Also, these properties (manufacturer/filter) have nothing to do with mobility, other than that products have a couple of translated attributes
Chris Salzberg
@shioyama
Hmm just catching up
So this is the change in 0.8.12: shioyama/mobility#364
Chris Salzberg
@shioyama

Ok, so I have some insight into this problem actually. I noticed you're aliasing query results in that SQL (as coming_30_days etc). That means those attributes will be actually fetched using method_missing rather than defined methods, which explains the relation to the change in 0.8.12.

For background on that (purely discussion of ActiveRecord), watch my RailsConf talk on the topic: https://www.youtube.com/watch?v=PNNrmNTQx2s&feature=emb_logo

So if you watch that talk, you'll understand (hopefully!) why calling coming_30_days on your model will end up hitting method_missing. For a model with Mobility enabled, and using fallthrough accessors (which are there if you're using dirty tracking), you will pass through that code that was changes. So that's the connection.
Chris Salzberg
@shioyama
Next question is: why did that code change anything?
Chris Salzberg
@shioyama
I'm wondering if part of the problem here is that the method signature for method_missing is not supposed to actually include keyword arguments:
method_missing(m, *a, &b)
So maybe what mobility is doing here, catching a method call with keyword arguments using method_missing, is actually breaking the API. Previously it was hiding that by not passing the keyword arguments to super, but now since it is it can break other stuff. Ironically that was what the original PR was trying to fix.
Chris Salzberg
@shioyama
Ok here's a failing test for the issue (on master branch, but same problem): shioyama/mobility@6730424
Basically it's a keyword arugment / hash thing. When you define method_missing without any **options, the options are being converted to a hash, which does not match the signature that ActiveRecord expects.
Chris Salzberg
@shioyama
Ok here's a tentative fix: shioyama/mobility#374
Chris Salzberg
@shioyama
Basically, we assume that if the method matches a Mobility locale accessor call, the last argument is an options hash (keyword arguments or hash object). This is safer since we can confine our thinking to what Mobility does. Then if it doesn't match, we just pass all the arguments along. This should satisfy both expectations.
Chris Salzberg
@shioyama
Ok Travis seems to be dead :disappointed: but tests pass locally for me. @Bramjetten can you try that PR branch in one of your repos? I think it should work for both scenarios.
Bram Jetten
@Bramjetten
I'm far from as well versed in ActiveModel/Record as you are, but I think I follow! Sadly, your fix introduced two new bugs for me.
Using a default for translated attributes stopped working and somehow the query_method is unavailable when saving a translated model.
I posted screenshots on your PR
Chris Salzberg
@shioyama
Ah sorry, the fix should be fine but that PR was off of the master branch, which has many other (unreleased) changes. I'll make a separate PR for 0.8.x and you can try that one.
Chris Salzberg
@shioyama
Unfortunately I won't be able to finish this until Monday, so I'm going to yank 0.8.12 for now.
Ok yanked 0.8.12. I'm pretty sure the fix in that PR will work on the 0-8-stable branch but until we're sure better just to go back to 0.8.11 for now.
Bram Jetten
@Bramjetten
Thanks Chris!
Chris Salzberg
@shioyama
Ok, decided to try and switch to Github Actions. If anybody here has experience, please lend a hand! Mostly not to do with internals of Mobility, just migrating from Travis.
Chris Salzberg
@shioyama
Almost done with Github Actions setup, will aim to fully switch sometime this week.
Chris Salzberg
@shioyama
@Bramjetten Give this one a try: shioyama/mobility#384
Chris Salzberg
@shioyama
Ok I've released 0.8.13 with the updated change to the method_missing patch. I'm pretty confident this one should avoid the issues of the previous (yanked) version.
Bram Jetten
@Bramjetten
Sorry for my late reply. It works!
Thanks a bunch :)
Bogdan
@HuntsmanX
Hello. Can you please tell about the configuration of the Mobility gem with Sequel. I created config/mobility.rb required it there but I see the issue
Gem::ConflictError: Unable to activate activerecord-6.0.2.1,
Olivier
@olimart
@sedubois Salut. Was wondering if you have a functional fork with ActionText? I'm in the midst of migrating to ActionText but experiencing the same problem. Can you please let me know if you're willing to share a workaround. Thank you.
Chris Salzberg
@shioyama
@HuntsmanX I don't think that's a Mobility issue, since Mobility does not depend on ActiveRecord or Sequel.
S├ębastien Dubois
@sedubois
Salut @olimart, sorry for the late reply. I've now configured to receive email notifications when I am mentioned. I have been preparing my app for this change, now I am actually running in prod with Mobility, but still using the table backend. However I made a demo here that shows that it can work: sedubois/mobility@7ec6bdf (I've linked to it from the issue as well: https://github.com/shioyama/mobility/issues/385#issuecomment-637082724). Does that help? Hopefully I will progress with migrating to the key-value backend with localizations in the next weeks.
Chris Salzberg
@shioyama
Just a quick update: I'm almost ready to finally release a an alpha version of Mobility v1.0, with a bunch of changes around configuration (but behavoiur should mostly stay the same). I'll create a quick wiki page with the alpha release to explain what you need to change, but it's not a whole lot. I've cleaned a lot of stuff up internally so there's much less hard-coded, and plugins are now really the core element in Mobility (more so than even backends).
I know it's been confusing because 0.8.x, the versions everybody is running, are pretty far out of sync with master, and have been for a long time. So I'd like to get this out asap and start moving to a stable release. Once I get the alpha out I'd be eager to get feedback on any issues people encounter (after updating configs).
Brandon Zylstra
@brandondrew
@shioyama I'm excited to hear about 1.0, and sorry to distract you with a small issue, but I cannot find documented anywhere how to specify different names for the tables that Mobility creates. Can you let me know how to do that?
Brandon Zylstra
@brandondrew
I would expect to put something like this in an initializer:
require 'mobility'
Mobility::ActiveRecord::TextTranslation.table_name   = 'translated_texts'
Mobility::ActiveRecord::StringTranslation.table_name = 'translated_strings'
(and of course edit the migrations before running them to use the chosen names)
Chris Salzberg
@shioyama
You can also subclass the default classes or create your own, and pass class_name: "..." to translates, which will allow you to tell Mobility which class to use.
So for example you could create a class TranslatedString and pass that name to class_name:
class TranslatedString < ActiveRecord::Base
end

class Post < ActiveRecord::Base
  extend Mobility
  translates :title, backend: :key_value, class_name: TranslatedString
end