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

Hotfix/api umbrella methods sync #1062

Merged
merged 33 commits into from
Jun 7, 2016
Merged

Conversation

frenchbread
Copy link
Contributor

@frenchbread frenchbread commented Jun 7, 2016

@frenchbread
Copy link
Contributor Author

@apinf/developers Please review & test

@jykae
Copy link
Contributor

jykae commented Jun 7, 2016

@frenchbread Overall code and solution looked good. Settings file usage won't be anymore supported I suppose?

@frenchbread
Copy link
Contributor Author

Yes. But basically, as I recall correctly there were some feature that transfers settings data from setting file to collection.

@jykae jykae self-assigned this Jun 7, 2016
@brylie
Copy link
Contributor

brylie commented Jun 7, 2016

@frenchbread yes. We should probably still allow settings to be provided by settings.json, so in startup we could write the Meteor.settings object to the Settings collection. Everywhere else in the app, though, we should be getting settings from the Settings collection.

@jykae
Copy link
Contributor

jykae commented Jun 7, 2016

Not syncing, should I wait 5 mins, or still bug?

@@ -0,0 +1,14 @@
export function apiUmbrellaSettigsValid (settings) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add letter 'n' for everywhere this function gets called and function name.

apiUmbrellaSettigsValid => apiUmbrellaSettingsValid

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch.

@jykae
Copy link
Contributor

jykae commented Jun 7, 2016

Generally:
On startup with fresh database I guess we don't have Settings collection, only possible is settings file that could have been passed.

@jykae
Copy link
Contributor

jykae commented Jun 7, 2016

Decided together to leave settings file case outside of this.

@frenchbread
Copy link
Contributor Author

Added some more refactoring & checks

@brylie brylie added this to the Sprint 24 milestone Jun 7, 2016
@jykae
Copy link
Contributor

jykae commented Jun 7, 2016

Tested again after meteor reset, sync runs now without errors.

@jykae
Copy link
Contributor

jykae commented Jun 7, 2016

Added one more close to the description. Merging and closing.

If one minute syncing interval causes issues for example in production when there might be more latency let's make it longer.

Great co-working guys @frenchbread @brylie

@jykae jykae merged commit 79353ab into develop Jun 7, 2016
@jykae jykae deleted the hotfix/api-umbrella-methods-sync branch June 7, 2016 13:14
@jykae jykae removed the in progress label Jun 7, 2016
@frenchbread
Copy link
Contributor Author

@jykae @brylie 👍

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.

None yet

3 participants