Skip to content

Feature/setup wizard#925

Merged
jykae merged 50 commits intodevelopfrom
feature/setup-wizard
Jun 2, 2016
Merged

Feature/setup wizard#925
jykae merged 50 commits intodevelopfrom
feature/setup-wizard

Conversation

@Alapan
Copy link
Copy Markdown
Contributor

@Alapan Alapan commented Apr 27, 2016

Closes #922

Important changes:

  • Created a server method to check the value of a flag called initialSetupComplete. If this returns false, an initial setup wizard is to be shown.
  • The wizard allows an admin to configure settings for the instance.

};

Router.onBeforeAction(function() {
var configRequired = Meteor.call('checkRequiredSettings');
Copy link
Copy Markdown
Contributor

@brylie brylie Apr 27, 2016

Choose a reason for hiding this comment

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

Since this code is relying on a server method, it is asynchronous. So, we need to handle the return value from checkRequiredSettings in a callback:

// Instead of
// var configRequired = Meteor.call("checkRequiredSettings");
// It should read

// Get a named reference to current router
// for use inside the server method callback
const currentRouter = this;

Meteor.call("checkRequiredSettings", function (error, result) {
  // if error....
  // throw error message

  // check the result of the method call
  if (result === false) {
    // If checkRequiredSettings returns false 
    Router.go("settingsWizard");
  } else {
    // call this.next() using the currentRouter variable from above
    currentRouter.next()
  }
});

@brylie
Copy link
Copy Markdown
Contributor

brylie commented Apr 29, 2016

Good work so far. Just a couple of minor details to catch at this point.

},
'click #save-settings': function() {
// when configuration is done, call server method to set initialSetupComplete to true, so that the settings alert is no longer shown
Meteor.call("initialSetupCompleteTrue");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Move this method call to the AutoForm.onSuccess callback.

@Alapan
Copy link
Copy Markdown
Contributor Author

Alapan commented Apr 29, 2016

@brylie I made the changes you suggested. However, I made a slight change in the logic - I have introduced a new field initialSetupComplete in the Settings collection. Otherwise, a setting created through the program is not persisting across multiple templates. So if initialSetupComplete is set to true when the relevant method is called through Template 1, the value is becoming undefined when it is checked through Template 2.

I have also removed the alert logic from the master layout file. Currently the alert is not being shown anywhere. Please review.

@brylie
Copy link
Copy Markdown
Contributor

brylie commented Apr 29, 2016

Ok, make sure that the problem is not related to meteor restarting when editing the project sourcecode. I.e. Meteor.settings may be reset each time we edit a project file, but should persist between page views.

If you take the collection approach, be sure to create a publication that only publishes the initialSetupComplete field from the settings collection document. Otherwise, we may inadvertently publish sensitive information from our settings collection.

@Alapan
Copy link
Copy Markdown
Contributor Author

Alapan commented Apr 29, 2016

I made the changes.

@brylie
Copy link
Copy Markdown
Contributor

brylie commented May 2, 2016

Great! :-) Is this ready for review/merge?

@Alapan
Copy link
Copy Markdown
Contributor Author

Alapan commented May 2, 2016

@brylie Yes, this is ready for review.

@shaliko
Copy link
Copy Markdown
Contributor

shaliko commented May 4, 2016

@Alapan project stop works for me if I do meteor reset and try open any page. Looks like infinite loop.

@Alapan Alapan force-pushed the feature/setup-wizard branch from 24140db to 95387ca Compare May 5, 2016 22:22
@Alapan
Copy link
Copy Markdown
Contributor Author

Alapan commented May 5, 2016

@shaliko I made a small change. Could you please test and let me know if you are still getting the same error?

@brylie
Copy link
Copy Markdown
Contributor

brylie commented May 6, 2016

When starting Apinf without any settings, i.e. without the --settings ... flag, I get the following error:

Error: After filtering out keys not in the schema, your object is now empty

One goal of this task is that Apinf should start with no initial settings, so the Wizard can display.

@brylie
Copy link
Copy Markdown
Contributor

brylie commented May 6, 2016

It seems like /server/startup.js may need significant re-work/review here. Specifically,

  • Lines 11 and 24 attempt to insert the Meteor.settings object into the settings collection without verifying the structure of that object
    • initially, Meteor.settings has the following structure { public: {} }, which doesn't contain any relevant settings
  • There are two large sections of that file with duplicate functionality (lines 4-12 and lines 20-24)
    • refactor this code, so that it only runs once
  • We should not call Meteor.call("createApiUmbrellaWeb") without first verifying that API Umbrella settings have been stored in the Settings collection
    • I.e. all of code may be refactored to first use the Settings collection as opposed to the Meteor.settings object
    • The Meteor.settings object will be a temporary placeholder for settings, so they can be persisted in the Settings collection
  • Any API Umbrella related methods should only run if there are valid API Umbrella settings in the Settings collection
    • The 'Sync API Umbrella Users and API Backends' SyncedCron job should only start if API Umbrella settings are present
      • the job, including any sub-methods, should check the Settings collection, rather than the Meteor.settings object
    • Meteor.call("syncApiUmbrellaUsers"); and Meteor.call("syncApiBackends"); should also only run if API Umbrella settings are stored in the Settings collection.

@brylie
Copy link
Copy Markdown
Contributor

brylie commented May 6, 2016

We cannot figure out how to get the settings wizard to display.

@Alapan
Copy link
Copy Markdown
Contributor Author

Alapan commented May 8, 2016

Currently, after running meteor reset and then meteor (without the settings file), when a user registers successfully, he is redirected to the wizard. However, I am facing a problem setting the Host name to http://localhost:3000 in the wizard, as the application returns an Unknown validation error. Setting the host name to something like http://example.com works, though.

Also, even after configuring the settings successfully, the following errors are being seen in the console:

  1. [TypeError: Cannot read property 'clientId' of undefined]
  2. [TypeError: Cannot read property 'username' of undefined]

@brylie
Copy link
Copy Markdown
Contributor

brylie commented May 9, 2016

Ah, yes. We had the same validation issue, possibly because of the port number.

As a second thought, it is probably unnecessary to provide the HOST value for the Apinf platform, since Meteor is already aware of this value, so lets deprecate that from the setup wizard/settings.

@brylie
Copy link
Copy Markdown
Contributor

brylie commented May 9, 2016

@frenchbread, what is your recollection of why we add the host settings in config.json? I think it had something to do with the dashboard.

@brylie
Copy link
Copy Markdown
Contributor

brylie commented May 9, 2016

@Alapan regarding the 'clientId' and 'username' errors, we need to check the existance of these fields before trying to use them. Try commenting out lines 11 and 24 of server/startup.js, as mentioned above.

@bajiat
Copy link
Copy Markdown
Contributor

bajiat commented May 9, 2016

@Alapan and @shaliko Could you do some pair programming on this one to try solve the issues Alapan is facing? Sometimes even just explaining the problem to someone else might already help. Shaliko is not available 9 May, but should be back at work 10 May.
@brylie said he was just quessing to a certain extent what the problem was.

@Alapan
Copy link
Copy Markdown
Contributor Author

Alapan commented May 10, 2016

I tried commenting the lines as @brylie suggested, but it didn't help. Please note, there was no problem in running the application - just that those 2 TypeErrors are present after filling in the settings details.

@brylie
Copy link
Copy Markdown
Contributor

brylie commented May 10, 2016

Ah, I understand now. Are they happening when you submit the form? It may still be a validation problem, where the schema is not passed in the fields it needs for validation.

@brylie
Copy link
Copy Markdown
Contributor

brylie commented May 10, 2016

@shaliko are you available today to help troubleshoot this task?

@shaliko
Copy link
Copy Markdown
Contributor

shaliko commented May 11, 2016

@brylie Today joining to this task.

@Alapan Alapan force-pushed the feature/setup-wizard branch from e3756c4 to 9d187c3 Compare June 2, 2016 12:04
@Alapan
Copy link
Copy Markdown
Contributor Author

Alapan commented Jun 2, 2016

@frenchbread Done.

@jykae
Copy link
Copy Markdown
Contributor

jykae commented Jun 2, 2016

@frenchbread As discussed I assign myself to check out this as you have work on the other PR.

@jykae jykae assigned jykae and unassigned frenchbread Jun 2, 2016
@frenchbread
Copy link
Copy Markdown
Contributor

Thanks @Alapan @jykae

@jykae
Copy link
Copy Markdown
Contributor

jykae commented Jun 2, 2016

@Alapan Reviewing

@jykae
Copy link
Copy Markdown
Contributor

jykae commented Jun 2, 2016

@Alapan This works and looks good. I am merging in.

Editor and Mail cannot be enabled because some autoform validation bugs. Discussed with @bajiat We have issue #961 for email and raised now in planning. Editor is then another case what we decide to do with that in future.

@jykae jykae merged commit ad2b11c into develop Jun 2, 2016
@jykae jykae removed the in progress label Jun 2, 2016
@jykae jykae deleted the feature/setup-wizard branch June 2, 2016 13:26
@jykae jykae removed the WIP label Jun 2, 2016
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.

8 participants