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

Feature/admin settings page #682

Merged
merged 72 commits into from
Dec 30, 2015
Merged

Feature/admin settings page #682

merged 72 commits into from
Dec 30, 2015

Conversation

55
Copy link
Contributor

@55 55 commented Dec 4, 2015

Closes #327

@55 55 removed the WIP label Dec 15, 2015
@55 55 assigned jykae and unassigned brylie Dec 17, 2015
@jykae
Copy link
Contributor

jykae commented Dec 17, 2015

Getting type errors to console are confusing when starting the application.
Also there are TODO's remaining in the code.

As for the Settings page, imho, and admin user experience, if it's possible to get current URL where application is running("Host") and for Umbrella settings, I would like enter only the Umbrella URL and API key/token information.

Possibly redirect to Settings page if we are getting undefined settings values in the startup.
However the requirements for this task are met. Could have been split to redesigning settings & implementing Settings page.

I will open up suggestion for redesigning settings.

@jykae jykae mentioned this pull request Dec 17, 2015
@jykae
Copy link
Contributor

jykae commented Dec 17, 2015

Cannot enter localhost with port.

nayttokuva 2015-12-17 kello 15 12 09

@jykae
Copy link
Contributor

jykae commented Dec 17, 2015

Cannot merge this yet. Let's look this together @elnzv and @brylie

@brylie
Copy link
Contributor

brylie commented Dec 17, 2015

Ah, interesting. We need to figure out if port is a requirement of this task.

@brylie
Copy link
Contributor

brylie commented Dec 17, 2015

In fairness here, we are using the SimpleSchema URL RegEx. URL regular expressions are typically complex, and might add undue complexity to this task.

Here are a couple of other possibilities we can pursue:

  • File an upstream feature request that URL RegEx support port number
  • Refactor the API Umbrella settings object to break the URL into components
    • base url
    • port
    • path

@bajiat bajiat added the WIP label Dec 30, 2015
@bajiat
Copy link
Contributor

bajiat commented Dec 30, 2015

@jykae has promised to make the changes

@brylie
Copy link
Contributor

brylie commented Dec 30, 2015

Cool. For a simple solution, we can just remove the URL Regex. However, then we are not guaranteed to have a valid URL, since there will be no validation.

@jykae
Copy link
Contributor

jykae commented Dec 30, 2015

Looks like Illya already removed the validation regex after my prev. testing, so this seems to work now, as the validation is removed from URL. Though I had chat today with @brylie in Skype and we decided to fix it rather than remove the check.

The planned fix is to add field for the port number to settings schema and keep the regex for URL.

EDIT: The port is not actually the issue here, but using "localhost". Validation is expecting REAL URL. Port numbers work with other URLs. My suggestion would be that we add the regex back, and I solve the conflict here.

I will make "enhancement" issue about this, but this is quite minor detail and only affects developers. If starting app with settings.json like before, using "localhost" in Apinf host URL works. And for development it should be fine. For UX when setting up Apinf first time or for production, having settings page is great.

@jykae jykae merged commit f1bef69 into develop Dec 30, 2015
@jykae jykae removed the in progress label Dec 30, 2015
@jykae
Copy link
Contributor

jykae commented Dec 30, 2015

Minor enhancement issue opened: https://github.com/apinf/api-umbrella-dashboard/issues/734

@jykae jykae deleted the feature/admin-settings-page branch December 30, 2015 15:30
@brylie
Copy link
Contributor

brylie commented Dec 30, 2015

@jykae good catch. I didn't realize 'localhost' was causing the regex to fail.

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

4 participants