Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Spree Upgrade] Update spree revision to include last package_factory fix #3641

Conversation

Projects
None yet
4 participants
@luisramos0
Copy link
Contributor

commented Mar 22, 2019

What? Why?

Closes #3607

ok, I figured this one out. With the new spree revision, the package shipping methods get filtered by distributor, so to get a green green build, we need to adapt the factories and specs to always set the order distributor on the shipping methods used in the test. This was already partially done in #3604

What should we test?

For a green build, this PR requires #3604 and also #3667 (due to duplicate order-charges html id).

We need to verify issue #3607 is fixed.

Dependencies

This PR can only be tested and merged after 3604 and 3667 but can be reviewed now.

@luisramos0 luisramos0 added the pr-wip label Mar 22, 2019

@luisramos0 luisramos0 self-assigned this Mar 22, 2019

@luisramos0 luisramos0 force-pushed the luisramos0:2-0-update-spree-2-0-4-revision branch from 77c0984 to aac0291 Mar 22, 2019

@sauloperez sauloperez self-assigned this Mar 26, 2019

@luisramos0 luisramos0 force-pushed the luisramos0:2-0-update-spree-2-0-4-revision branch 2 times, most recently from 8bf8a19 to 834eac5 Apr 3, 2019

@luisramos0 luisramos0 changed the title [Spree Upgrade] WIP Update spree revision to include last package_factory fix [Spree Upgrade] Update spree revision to include last package_factory fix Apr 3, 2019

@luisramos0 luisramos0 removed the pr-wip label Apr 3, 2019

@@ -59,7 +59,7 @@ GIT
activemerchant (~> 1.34)
acts_as_list (= 0.2.0)
awesome_nested_set (= 2.1.5)
aws-sdk (~> 1.11.1)
aws-sdk (~> 1.3.4)

This comment has been minimized.

Copy link
@sauloperez

sauloperez Apr 5, 2019

Contributor

that is quite a big change. Have you checked why it happened? I find it weird to have these Gemfile.lock changes at this point. The revision moves us a bunch of commits forward only, right? 🤔

This comment has been minimized.

Copy link
@luisramos0

luisramos0 Apr 5, 2019

Author Contributor

this is just the upload/download of images from s3 right?
we are moving one commit forward only.
I dont understand why this happens.
shall I lock 1.11 on our side, in our Gemfile?

This comment has been minimized.

Copy link
@sauloperez

sauloperez Apr 9, 2019

Contributor

this is just the upload/download of images from s3 right?

I guess so as it's defined as a spree_core dependency. I can't think of any other Spree feature that might need aws.

I dont understand why this happens.

Me neither. I'll try to reproduce it in my machine.

This comment has been minimized.

Copy link
@sauloperez

sauloperez Apr 9, 2019

Contributor

Ok, same outcome on my machine. For some reason when resolving spree's dependencies again (through a bundle update spree bundler thinks these versions are better suited 🤷‍♀️ . So locking aws-sdk to 1.11 or above sounds good IMO.

This comment has been minimized.

Copy link
@sauloperez

sauloperez Apr 9, 2019

Contributor

I tried to do it myself but I found out why bundler was doing it in the first place:

Bundler could not find compatible versions for gem "aws-sdk":
  In Gemfile:
    aws-sdk (>= 1.11.1)

    spree was resolved to 2.0.4, which depends on
      spree_core (= 2.0.4) was resolved to 2.0.4, which depends on
        aws-sdk (~> 1.3.4)

So it turns out the Gemfile.lock was wrong. Maybe a wrongly solved merge conflict?

Nothing to do here then.

This comment has been minimized.

Copy link
@luisramos0

luisramos0 Apr 9, 2019

Author Contributor

ah, nice, thanks for having a look!
spree 204 depends on 1.3.4 in spree repo here and in ofn spree repo here.
What I just found is that our current spree branch step-6a is on 1.11.1 since a long time ago.
So I think we need to open a new PR in ofn/spree/2-0-4 and change it to depend on 1.11.1
Then update this PR to use that new revision of ofn/spree/2-0-4 and keep 1.11.1. Makes sense?

This comment has been minimized.

Copy link
@luisramos0

luisramos0 Apr 9, 2019

Author Contributor

ok, I went for it.
There's only the update of the spree revision in this PR now.
I'll update aws-sdk in a separate PR.

This comment has been minimized.

Copy link
@sauloperez

sauloperez Apr 10, 2019

Contributor

Glad that you carried on with the investigation. Sounds good 👍

I wonder if there will be any more cases like this one.

This comment has been minimized.

Copy link
@luisramos0

luisramos0 Apr 10, 2019

Author Contributor

yeah, this is the only one breaking a fresh bundle install

@luisramos0 luisramos0 force-pushed the luisramos0:2-0-update-spree-2-0-4-revision branch 2 times, most recently from 460e88d to 0dd53b0 Apr 9, 2019

@sauloperez

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

I gave CI a second chance by rebuilding it

@luisramos0

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

The build is good, I tested it thoroughly.
The last one was only broken in one orders_spec because I dont want to put #3667 in this PR.
All good.

@luisramos0

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

this is fixed in #3667


 1) 
    As an administrator
    I want to manage orders
 as an enterprise manager viewing the edit page shows the order tax adjustments
     Failure/Error:
       within('tbody#order-charges') do
         expect(page).to have_selector "td", match: :first, text: "Tax 1"
         expect(page).to have_selector "td.total", text: Spree::Money.new(10)
       end
     
     Capybara::Ambiguous:
       Ambiguous match, found 2 elements matching visible css "tbody#order-charges"
     # ./spec/features/admin/orders_spec.rb:274:in `block (4 levels) in <top (required)>'

luisramos0 added some commits Mar 22, 2019

@luisramos0 luisramos0 force-pushed the luisramos0:2-0-update-spree-2-0-4-revision branch from 0dd53b0 to 46278e3 Apr 10, 2019

@luisramos0

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

great, thanks for the quick reviews guys!

both dependencies are merged now. I rebased so we have a green build.

moving to test ready.

@luisramos0

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

@RachL apart from testing #3607, because this is rebased it includes #3668 and so this is the right PR to test #3625 as well
and maybe we can check that the PRs that were tested in master are also working in v2: #3662, #3646 and #3644

@luisramos0

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

@RachL staging Katuma is ready on v2. I ended up using this PR because it is the last major v2 PR. and I wanted to test the data migration with live data and all major changes in, it worked perfectly 🎉
you can stage other PRs in semaphore ;-)

@RachL RachL self-assigned this Apr 12, 2019

@RachL

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

@luisramos0 thx! I've checked them all and everything is fine. I didn't see the spanish translation on "producer profile" but I guess it is not done on transifex. Anyway it shouldn't postpone this PR to be merged. So this is ready to go https://docs.google.com/document/d/14fB7poxkCBKRX2o7wOivSG32IRsPEOWtbUzxmf7LILs/edit#

@RachL RachL removed the pr-staged-es label Apr 12, 2019

@luisramos0

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

nice!! thanks!
"I've checked them all" 💪

@luisramos0 luisramos0 merged commit 1e85bf6 into openfoodfoundation:2-0-stable Apr 12, 2019

1 check passed

semaphoreci The build passed on Semaphore.
Details

@luisramos0 luisramos0 deleted the luisramos0:2-0-update-spree-2-0-4-revision branch Apr 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.