Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Repo info
Activity
    Keisuke Izumiya
    @syguer
    #822 's spec patch can reproduce it.
    Abdelkader Boudih
    @seuros
    you should sanitize your tag list with a parser
    Keisuke Izumiya
    @syguer
    hmm... Parser should know validation rules?
    Abdelkader Boudih
    @seuros
    Parser will delete anything that don’t match your rules
    like the format
    also you have to have validations in the frontend before it reach the server
    Who is tagging with more than 255 char ?
    Keisuke Izumiya
    @syguer
    I agree we should have validations in the frontend. And I know nobody tagging with more than 255 characters(without cracker :worried: ).
    But the issue I said is clearly weird behavior. can't I fix this on gem code?
    If you can't accept, I'm going to give up :cry:
    Abdelkader Boudih
    @seuros
    It will break people code. They don’t expect a exception there.
    Keisuke Izumiya
    @syguer
    I understand. OK, thank you for discussion :smile:
    Piero Dotti
    @ProGM

    @seuros I'm having lots issues with the ActsAsTaggableOn::Taggable::Cache module.
    I honestly think that injecting it in "when required" is indeed a BAD idea.

    I'm talking about this:
    https://github.com/mbleigh/acts-as-taggable-on/blob/8e64c3d4a81cfbb8af621228eae36a65c1f94501/lib/acts_as_taggable_on/taggable/cache.rb#L38

    a few months ago I submitted a PR to fix a bug about it:
    mbleigh/acts-as-taggable-on#806

    But still is creating problems

    Abdelkader Boudih
    @seuros
    Let me check this
    Piero Dotti
    @ProGM
    With certain conditions, the caching methods do not trigger (still investigating)
    Abdelkader Boudih
    @seuros
    i was planing in releasing a new version now. Let do it now.
    I dropped support for old versions of AR, so we can cleanup the code.
    can you test master with your test suite ? @ProGM
    Piero Dotti
    @ProGM
    ok, let's try :)
    Radhames Brito
    @rbritom
    Good morning
    Piero Dotti
    @ProGM
    It's not resolving...
    I'm trying to create a test case for this.
    Radhames Brito
    @rbritom
    I saw this issue
    I can investigate
    and provide a fix
    Abdelkader Boudih
    @seuros
    What do you think about having a outout config to remove cache ?
    opt-out
    Piero Dotti
    @ProGM
    Or maybe a opt-in
    acts_as_taggable_on :tags, caching: true ?
    Abdelkader Boudih
    @seuros
    i can’t do opt-in, that will break the upgrades.
    Piero Dotti
    @ProGM
    I see
    correct
    It makes sense
    Ok, an opt-out is ok, if well documented
    Radhames Brito
    @rbritom
    @ProGM you fix seemed to make sense, what new problem are you getting now?
    I mean after you PR was merge
    Piero Dotti
    @ProGM
    Indeed it fixes that bug
    But the caching module is still creating issues because it's loaded in runtime
    For instance:
    with "certain conditions" (I'm investigating), the "cached_tag_list" column is ignored
    Abdelkader Boudih
    @seuros
    i want to test master in spree codebase.
    Radhames Brito
    @rbritom
    that should not be hard
    Piero Dotti
    @ProGM
    I think I found a way to demostrate it:
    mbleigh/acts-as-taggable-on#830
    @seuros
    Piero Dotti
    @ProGM
    Uhm, I think I discovered the problem.
    Here
    Before trying to check if the cached_tag_list column is defined, add_custom_context is called