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

Move all /server files to plugins #4366

Merged
merged 30 commits into from Jul 2, 2018

Conversation

Projects
None yet
3 participants
@aldeed
Copy link
Member

commented Jun 25, 2018

Resolves #4363
Impact: breaking
Type: refactor

Changes

Move all files from within /server to various plugins

Breaking changes

For any custom code or community plugins, all imports with paths that begin with /server will need to be changed.

Testing

General smoke testing to catch any refactor errors. No substantial code or logic changes were made.

@aldeed aldeed self-assigned this Jun 25, 2018

@aldeed aldeed changed the base branch from feat-aldeed-nometeor-address-book-add to release-1.14.0 Jun 26, 2018

@aldeed aldeed force-pushed the refactor-aldeed-server-to-plugin branch to db55943 Jun 26, 2018

@aldeed aldeed changed the title [WIP] Move all /server files to plugins Move all /server files to plugins Jun 26, 2018

@aldeed aldeed requested a review from spencern Jun 26, 2018

@spencern
Copy link
Member

left a comment

Looks like some of the startup code is able to run twice.

image

Seeing doubles of plugins
image

I upgraded from a 1.13.0 branch with data in the app. Started the app using docker-compose.

image

Working on nailing down a reproduction now

This appears to have been done by using mongorestore to load data. Retracting this request for changes

@spencern
Copy link
Member

left a comment

Updated review because I was on the wrong branch :/

Reviewed master branch accidentally

@spencern
Copy link
Member

left a comment

Seeing issues with checkout, orders are not getting created.

Using the example payment method and flat rate shipping, add a product to your cart and step through guest checkout.

I'm seeing issues when adding an address, there's a delay before the address shows up and the create address form is shown again.

Upon placing an order, the user is taken to the cart/completed route but the order is never created.

After attempting to place an order using Stripe, the charge was authorized, but the order was never created.

fixed issues

@aldeed

This comment has been minimized.

Copy link
Member Author

commented Jun 29, 2018

@spencern I fixed the order completion issue. One of the method hooks wasn't firing, maybe because of timing of when it was added, but it made more sense to just move that code into the method it was hooking. I verified that the other method hooks are running.

I see the address slowness thing, too, but I don't see how my changes would have impacted that. It does work eventually, but it's slow. I feel like we debugged and fixed that issue awhile back. Not sure it is worth our time.

I also noticed errors because Twilio and Nexmo were not being imported correctly, and adjusted those imports. You may want to retest sms sending.

@spencern

This comment has been minimized.

Copy link
Member

commented Jul 2, 2018

Issues with checkout have been resolved.
Going to see if I can determine the reason that the address book is responding differently

@spencern spencern referenced this pull request Jul 2, 2018

Closed

Checkout address issue #4394

@spencern
Copy link
Member

left a comment

Looks like the issues I'm seeing with addresses exist in the release-1.14.0 branch already.

Possible were introduced in #4167

@spencern spencern merged commit 8304435 into release-1.14.0 Jul 2, 2018

10 of 11 checks passed

License Compliance 6 issues found
Details
WIP ready for review
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: docker-build Your tests passed on CircleCI!
Details
ci/circleci: docker-push Your tests passed on CircleCI!
Details
ci/circleci: dockerfile-lint Your tests passed on CircleCI!
Details
ci/circleci: eslint Your tests passed on CircleCI!
Details
ci/circleci: snyk-security Your tests passed on CircleCI!
Details
ci/circleci: test-app Your tests passed on CircleCI!
Details
ci/circleci: test-unit Your tests passed on CircleCI!
Details
security/snyk - package.json No dependency changes
Details

@spencern spencern deleted the refactor-aldeed-server-to-plugin branch Jul 2, 2018

@zenweasel

This comment has been minimized.

Copy link
Member

commented Jul 3, 2018

@spencern Not sure if it's the same thing, but seeing really bad performance from account/addressBookAdd when doing load testing, which we are doing on 1.13

@spencern spencern referenced this pull request Jul 11, 2018

Merged

Release 1.14.0 #4338

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.