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

Update provides in the registry accept an array of strings #2930

Merged
merged 11 commits into from
Sep 24, 2017

Conversation

spencern
Copy link
Contributor

This PR resolves #2897 by enabling the use of multiple provides values in an array, making registry entries such as provides: ["paymentSettings", "marketplacePaymentSettings"] valid.

Going forward we will use a method based on this to determine if a given plugin is for ready for marketplace. This PR does not contain any functionality to identify marketplace ready packages itself, but it does provide the structure for that.

It does not accept both a String and an Array as was suggested in the issue. That would have required using a blackbox type in Simple Schema which felt like the wrong direction. Instead a migration is provided which will upgrade any packages that currently exist on the system from using (using "dashboard" for example) provides: "dashboard" to provides: ["dashboard"].

The update to Reaction Apps which filters packages to determine if a given plugin registry entry is a match has been updated to check against the simple schema type of a registry property. If the schema property type and the existing registry property are both arrays then we will check to see if the filter matches an element in the array rather than checking to see if there is an exact match.

This maintains backwards compatibility to any package that manages to avoid getting upgraded.

@aaronjudd
Copy link
Contributor

@reactioncommerce/core any idea why imports/plugins/included/discount-rates/disabled.register.js is disabled?

@aaronjudd
Copy link
Contributor

aaronjudd commented Sep 22, 2017

Ah, nevermind... that's the "custom rates", not "discount-codes"... and I did that.. because we don't yet have "custom rates".... (which is pricing by "tag",etc.)

@aaronjudd
Copy link
Contributor

aaronjudd commented Sep 22, 2017

Are we expecting that there are still some packages running deprecated provides values?

(warnings working great!).:

20:24:48.463Z  WARN Reaction: Plugin reaction-checkout is using a deprecated version of the provides property for the cart/checkout registry entry. Since v1.5.0 registry provides accepts an array of strings.
20:24:48.463Z  WARN Reaction: Plugin reaction-checkout is using a deprecated version of the provides property for the cart/completed registry entry. Since v1.5.0 registry provides accepts an array of strings.
20:24:48.729Z  WARN Reaction: Plugin reaction-orders is using a deprecated version of the provides property for the dashboard/pdf/orders registry entry. Since v1.5.0 registry provides accepts an array of strings.
20:24:49.001Z  WARN Reaction: Plugin reaction-ui is using a deprecated version of the provides property for the dashboard/uiThemeDetails registry entry. Since v1.5.0 registry provides accepts an array of strings.
20:24:49.336Z  WARN Reaction: Plugin reaction-marketplace is using a deprecated version of the provides property for the shop registry entry. Since v1.5.0 registry provides accepts an array of strings.
20:24:49.369Z  WARN Reaction: Plugin reaction-notification is using a deprecated version of the provides property for the notifications registry entry. Since v1.5.0 registry provides accepts an array of strings.
20:24:49.477Z  WARN Reaction: Plugin reaction-paypal is using a deprecated version of the provides property for the /paypal/done registry entry. Since v1.5.0 registry provides accepts an array of strings.
20:24:49.477Z  WARN Reaction: Plugin reaction-paypal is using a deprecated version of the provides property for the /paypal/cancel registry entry. Since v1.5.0 registry provides accepts an array of strings.
20:24:49.522Z  WARN Reaction: Plugin reaction-stripe is using a deprecated version of the provides property for the /stripe/connect/authorize registry entry. Since v1.5.0 registry provides accepts an array of strings.
20:24:49.620Z  WARN Reaction: Plugin product-detail-simple is using a deprecated version of the provides property for the product registry entry. Since v1.5.0 registry provides accepts an array of strings.
20:24:49.672Z  WARN Reaction: Plugin reaction-product-variant is using a deprecated version of the provides property for the tag registry entry. Since v1.5.0 registry provides accepts an array of strings.```

Copy link
Contributor

@aaronjudd aaronjudd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when I searched for provides: " I didn't find any instances that had not yet been converted to using the array format, but still received warnings indicated this was in use, on a fresh installation, no existing data.

@spencern
Copy link
Contributor Author

spencern commented Sep 22, 2017

@aaronjudd interesting - do you have any packages in your custom folder?
Looking into this now.

@spencern
Copy link
Contributor Author

I think this may have be an issue where the warning is triggering on any registry entry that does not have a provides definition. Building a fix now

@spencern
Copy link
Contributor Author

@aaronjudd Good catch on that review, looks like I was catching instances where provides was undefined in addition to instances where provides was a string. Fixed now.

Copy link
Contributor

@aaronjudd aaronjudd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • reaction reset in marketplace
  • login as admin
  • view orders panel

reaction_and_reactioncommerce_reaction_and_services_agreement_template_-outbound-complete-open_source-_v20160115_1010_final-reaction

  • stop the initial running instance
  • switch to this branch

Server console warns

5:57:45.435Z  WARN Reaction: Plugin reaction-accounts is using a deprecated version of the provides property for the accounts registry entry. Since v1.5.0 registry provides accepts an array of strings.
15:57:45.436Z  WARN Reaction: Plugin reaction-accounts is using a deprecated version of the provides property for the /dashboard/account/settings registry entry. Since v1.5.0 registry provides accepts an array of strings.
15:57:45.436Z  WARN Reaction: Plugin reaction-accounts is using a deprecated version of the provides property for the dashboard/accounts registry entry. Since v1.5.0 registry provides accepts an array of strings.
15:57:45.436Z  WARN Reaction: Plugin reaction-accounts is using a deprecated version of the provides property for the account/profile registry entry. Since v1.5.0 registry provides accepts an array of strings.
15:57:45.491Z  WARN Reaction: Plugin reaction-catalog is using a deprecated version of the provides property for the undefined registry entry. Since v1.5.0 registry provides accepts an array of strings.
15:57:45.491Z  WARN Reaction: Plugin reaction-catalog is using a deprecated version of the provides property for the catalog/settings registry entry. Since v1.5.0 registry provides accepts an array of strings.
15:57:45.587Z  WARN Reaction: Plugin reaction-connectors is using a deprecated version of the provides property for the connectors registry entry. Since v1.5.0 registry provides accepts an array of strings.
15:57:45.587Z  WARN Reaction: Plugin reaction-connectors is using a deprecated version of the provides property for the settings/connectors registry entry. Since v1.5.0 registry provides accepts an array of strings.
15
.....
  • Login in again as admin,
  • view orders (or accounts)

reaction_and_reactioncommerce_reaction

  • stop reaction
  • run again
  • warnings cleared, registry updated

The registry warnings being gone on restart is what I expected, but because the warning in that case will not appear again, should the messaging reflect this?

However, there appears to a bug in the UI logic for the admin panel that was probably tied to checking the "provides" type.. (see screenshots)..

@spencern
Copy link
Contributor Author

Interesting thought on the warnings being a one-time thing. They are only triggered when we find a registry entry with a string value for provides Do we have a mechanism we could use to persist an error in this case?

I see the error on the UI, and that does seem like it could be triggered by a provides issue. I'll try your reproduction steps and see what I can fix.

Currently there aren't any permissions that use the `provides` as a label, but this be pretty if it gets used and has multiple provides values
Guards inclusion checks for provides being undefined
@spencern
Copy link
Contributor Author

spencern commented Sep 23, 2017

@aaronjudd I've updated the warning message to include

Please update any plugins you maintain to use an array for provides. You will not see this message again for these plugins.

I've also identified and fixed the issue that was causing the dashboard panel to render in the small view. There are a few other spots in our code that were checking for provides equality. I've updated all of those instances to use prototype includes and to guard against provides being undefined.

Array.prototype.includes has the side-affect of checking for string inclusion as well as array inclusion. I think in this case this is a positive thing, though there are instances where it could be detrimental.

"dashboard".includes("dashboard") => true
["dashboard"].includes("dashboard") => true

the downside here would be any cases where something like this could occur:

"marketplace-dashboard".includes("dashboard") => true

We don't currently have any instances where this would be possible and since we're forcibly moving to arrays, I think it's okay, but we could do an equality check on string types, and use includes only on Arrays if there is concern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants