solidus

solidus copied to clipboard

In our business, the semantics of availability and discontinuation are not aligned with Solidus' nomenclature: A product can both be available and discontinued (i.e. we still have stock of it, but won't be producing any more).

When communicating the meaning of the discontinue_on column, it's often hard (because there is a date when we decide to discontinue, and there's another date when we sell out and the product becomes unavailable).

From reading the code, Solidus' discontinue_on is actually a mirror of available_until.

What this commit does is it adds an available_until column, populates that with the data from discontinue_on and changes all scopes within Solidus to reference the new column. In the next major upgrade, we can then remove the discontinue_on column.

Checklist:

• [x] I have followed Pull Request guidelines
• [x] I have added a detailed description into each commit message
• [x] I have updated Guides and README accordingly to this change (if needed)
• [x] I have adapted tests to cover this change (if needed)

Agree, but technically that's not a backward compatible change, as queries that go directly to the database, bypassing alias_attribute will be broken. Also, I haven't checked it, but I'm afraid some other features, like dirty attributes or method #attribute_names, won't pick aliases, so it could also break some code.

We could put it on hold after the next major or create a soft deprecation strategy adding instead of renaming a column.

If we do that, how do we deal with the two columns going out of sync? And how would things be different than now after the next major version? When is the next major release planned?

It took years for Solidus 3 to be released. I appreciate the commitment to stability in this project, but in cases like this where there's cheap progress to be had at the expense of two search and replace operations in the host app, I sometimes think the deprecation strategies we try to find up with generate a large (and tbh frustrating) burden on contributors.

Solidus apps are complicated beasts with many interfaces (as ruby allows metaprogramming). Which parts of the interface do we consider stable API, and where do we allow ourselves change? Is this documented somewhere?

Solidus apps are complicated beasts with many interfaces (as ruby allows metaprogramming). Which parts of the interface do we consider stable API, and where do we allow ourselves change? Is this documented somewhere?

Due to the nature of the project, as you said, this is left to the core team common sense. I think we can do an effort, trying to document it a little bit, at least as a series of principles.

I know how frustrating this could be because I was on the other side (and @mamhoff, you were on this side 🙂) but I think stability and doing our best to be SemVer compliant is still one of the best things about Solidus and what distinguishes us from the other platforms.

On a side note, let me also add that in the past, we were releasing new Solidus major versions based on some more marketing initiatives and I think it's time to stop doing that and release when we have something breaking. We were discussing releasing the next major soon because we want to dismiss some old things like the fronted as a gem. With soon, I think this could be right after the next minor, so after 3.2 there could be Solidus 4.

That said, we discussed this in the core team meeting and we agreed that this can definitely wait for the next major in this form. If we find a way to have this at least compatible with the Rails public methods (like dirty etc) we can re-evaluate this.

But let me thank you on the behalf of the whole community for the great changes you are lately proposing to the platform, we really value your contributions and the last thing we want is to make you frustrated about continuing contributing. We will do our best to have these changes in Solidus as soon as possible.

I've reworked the PR to not rename the column, but to add an additional available_until column that is updated alongside the discontinue_on column. That way we can safely deprecate and delete the old column with the next major version. What do you think?

I was discussing this offline with @waiting-for-dev and we realized that this still won't work with update_column and update_all. Not saying it's a trade-off we won't accept but reporting here for completeness.

I was discussing this offline with @waiting-for-dev and we realized that this still won't work with update_column and update_all. Not saying it's a trade-off we won't accept but reporting here for completeness.

For more context, this is a script that reproduces how syncing columns can be bypassed using public AR API. Just execute with ruby file.rb. The second assertion will fail:

# frozen_string_literal: true

require 'bundler/inline'

gemfile(true) do
  source 'https://rubygems.org/'
  gem 'activerecord'
  gem 'sqlite3'
end

require 'active_record'

ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: 'bypassing.db')

ActiveRecord::Schema.define do
  create_table :dummies, force: true do |t|
    t.string :field_one
    t.string :field_two
    t.timestamps
  end
end

class Dummy < ActiveRecord::Base
  def field_one=(value)
    write_attribute(:field_two, value)
    super
  end
end

require 'minitest/autorun'

class BypassingTest < Minitest::Test
  def test_bypass
    dummy = Dummy.create(field_one: 'foo')
    dummy.update_column(:field_one, 'bar')
    dummy.reload

    assert_equal(
      'bar',
      dummy.field_one
    )
    assert_equal(
      'bar',
      dummy.field_two
    )
  end
end

I'm fine adding the attribute to the API, no problem. Regarding the deprecation strategy though: Do you have a suggested solution?

I don't think there's a solution here, or I don't see it now. I think it's a matter to understand if we want to accept this trade-off or wait for the next major. I will let you know tomorrow.

Hey @mamhoff, thanks again for this work and for adjusting the PR to try to make it backward-compatible. We think that this still needs to wait for the next major to be merged. We are discussing releasing very soon (let's say in the next couple of months, just to give you an idea). Thanks for understanding.

Should I revert to the previous version of this PR then for the major release? It is unfortunate that users won't be warned about the disappearance of the column, but I don't think I'm willing to keep updating this - or a related - PR until Solidus 5 comes out :)

@mamhoff Yes, can you please revert, please. We won't make any deprecation warning for this change, but I think it's legit to expect this kind of change in a major release.

Reverted. I've left the alias_attribute and deprecation warnings in, so at least in some circumstances people get deprecation warnings.

Would Solidus 4 then have this version, or the previous iteration?

Alberto Vena @.***> schrieb am Fr., 6. Mai 2022, 12:02:

Hey @mamhoff https://github.com/mamhoff, thanks again for this work and for adjusting the PR to try to make it backward-compatible. We think that this still needs to wait for the next major to be merged. We are discussing releasing very soon (let's say in the next couple of months, just to give you an idea). Thanks for understanding.

— Reply to this email directly, view it on GitHub https://github.com/solidusio/solidus/pull/4363#issuecomment-1119454238, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFLXKOVNQIDJKF4SZINAE3VITU4FANCNFSM5UMRV2BQ . You are receiving this because you were mentioned.Message ID: @.***>

@mamhoff, yes, this is on hold until v4

@mamhoff hello! With a lot of late, but we are close to releasing Solidus 4.0. Could you please let me know if you are still interested in having this merged? Please, let me know if you have time to work on rebasing this, otherwise we'll see if we can move ahead on our own. Thanks again for your patience!

Rebased from master. In our store, we have now settled on retired_on with a discontinue_on state before that, but for Solidus core I think that having the two states mirror each other makes a lot of sense still.

Given that you seem to have found a solution "good enough", we would feel more confident not merging this.

The reality is that we couldn't find a safe way to rename a column in use with a simple migration, and the change here, although important, doesn't look to be that critical. We'll put some thoughts on finding a generic solution for this problem, and apply that here.

Sorry for "promising" to have this merged in v4! 🙏

Ok, closing.