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

Single service mode #68

Merged
merged 7 commits into from Aug 31, 2018

Conversation

Projects
None yet
3 participants
@lexoyo
Member

lexoyo commented Aug 24, 2018

closes #62

it is a simplified version of #62

@lexoyo lexoyo referenced this pull request Aug 24, 2018

Closed

Single service mode #62

@lexoyo lexoyo requested review from JbIPS and clemos Aug 24, 2018

@clemos

pretty difficult to follow :|

@@ -299,6 +299,7 @@ export default class UnifileService {
}
static isService (file) {
return typeof file.isLoggedIn !== 'undefined';
console.log('isService', file, file.isLoggedIn);
return typeof file.mime === 'application/json';

This comment has been minimized.

@clemos

clemos Aug 25, 2018

Contributor

weird, pls comment

This comment has been minimized.

@lexoyo

lexoyo Aug 25, 2018

Member

the console log is a mistake

do you think this is ok for a comment?

this mime type is what the server returns when listing services... and I think it never will be the mime type of any other files

Do you think I should rather add a specific field "isService" to the data returned by the server side?

This comment has been minimized.

@JbIPS

JbIPS Aug 25, 2018

Contributor

I see a lot of use case when a regular file will have a application/json MIME type, so yes, it should already be in the data without duck-typing

This comment has been minimized.

@lexoyo

lexoyo Aug 26, 2018

Member

Ok, so do you think I should re-open this PR?
#50

This comment has been minimized.

@lexoyo

lexoyo Aug 26, 2018

Member

I see a lot of use case when a regular file will have a application/json MIME type, so yes, it should already be in the data without duck-typing

for a static file this would be strange
if you remember we decided that this would be the mime type of services, which make them apear with an icon of service

This comment has been minimized.

@JbIPS

JbIPS Aug 26, 2018

Contributor

You're thinking too much with Silex, not CE. As a developer, my FTP might be full of JSON config files I saved here.

You wouldn't want that to mess up CE

@@ -124,9 +124,9 @@ export default class UnifileService {
});
}
cd (path) {
cd (path, preventAuth=false) {

This comment has been minimized.

@clemos

clemos Aug 25, 2018

Contributor

I think you should add it as a flag a the service level, around

nameMap.set(service.name, service.displayName);

service map should be Map<String, {displayName, requiresAuth}>

This comment has been minimized.

@lexoyo

lexoyo Aug 25, 2018

Member

in fact some services won't require auth, but this is about another topic here: when CE starts, if the path is "/" it will list services, and if there is only one service we want to cd into this folder. But without auth because we don't want a popup to be opened and blocked (not in a onclick). What happen here is we cd the service and if the user is not logged in it will return a 403 forbidden and we will be back to "/"

The best thing would be to send a isLoggedIn field with the services, so if the user is loggedin we cd in it, and when we cd a service to which we are logged in, CE should not bother opening a popup for auth. I made a PR for that but I think it died forgotten.. Maybe it was wrong :-/

return new Promise((resolve, reject) => {
if (path.length === 1 && path[0] !== this.currentPath[0]) {
if (!preventAuth && path.length === 1 && path[0] !== this.currentPath[0]) {

This comment has been minimized.

@clemos

clemos Aug 25, 2018

Contributor

this check is weird (it was weird already)

This comment has been minimized.

@lexoyo

lexoyo Aug 25, 2018

Member

yeah.. it's a way to check if we are opening a service or a folder... we should probably use isService or something
but we don't have the files yet at this level, only the path
any suggestions?

This comment has been minimized.

@JbIPS

JbIPS Aug 25, 2018

Contributor

We could retrieve the service with the path, right? If it's the first part of the path, use the service Map to get the service and fetch the service auth strategy (I think that's what @clemos was trying to say)

This comment has been minimized.

@lexoyo

lexoyo Aug 26, 2018

Member

ok, then that's what path.length === 1 does

@@ -162,6 +162,15 @@ export default class CloudExplorer extends React.Component {
files,
loading: false
});
// the first display, single service mode, try to enter
// this is useful when CE is used by hosting companies to display the user files, and the user has logged in their system
if(!this.initDone && files.length === 1 && path.length === 0/* && files[0].isLoggedIn === true*/) {

This comment has been minimized.

@clemos

clemos Aug 25, 2018

Contributor

I think initDone should be part of the component state.

This comment has been minimized.

@clemos

clemos Aug 25, 2018

Contributor

but more generally, I don't quite understand this part.

This comment has been minimized.

@lexoyo

lexoyo Aug 25, 2018

Member

yes again that's a way to open a service when CE starts, so only the 1st time and if it is "/" which is ls
maybe i should put all this somewhere else...

@JbIPS

I agree with @clemos

return new Promise((resolve, reject) => {
if (path.length === 1 && path[0] !== this.currentPath[0]) {
if (!preventAuth && path.length === 1 && path[0] !== this.currentPath[0]) {

This comment has been minimized.

@JbIPS

JbIPS Aug 25, 2018

Contributor

We could retrieve the service with the path, right? If it's the first part of the path, use the service Map to get the service and fetch the service auth strategy (I think that's what @clemos was trying to say)

@@ -299,6 +299,7 @@ export default class UnifileService {
}
static isService (file) {
return typeof file.isLoggedIn !== 'undefined';
console.log('isService', file, file.isLoggedIn);
return typeof file.mime === 'application/json';

This comment has been minimized.

@JbIPS

JbIPS Aug 25, 2018

Contributor

I see a lot of use case when a regular file will have a application/json MIME type, so yes, it should already be in the data without duck-typing

@lexoyo

This comment has been minimized.

Member

lexoyo commented Aug 26, 2018

Ok so

  • I have fixed the issues you raised
  • I have added a "isLoggedIn" info and "isService" info as I previously wanted to do in the old PR
  • since this prevented any call to Dropbox or Github when the user had been logged in once, I had to add a logout button - which I did at the service level in the UI, but a "logout all" button somewhere might be needed by @godzone for the single service use case
@lexoyo

This comment has been minimized.

Member

lexoyo commented Aug 26, 2018

ce2-logout-single-service

Show resolved Hide resolved src/js/CloudExplorer.jsx Outdated

@lexoyo lexoyo merged commit 2ebea1a into silexlabs:master Aug 31, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@lexoyo

This comment has been minimized.

Member

lexoyo commented Aug 31, 2018

oh there is an old issue about the linter: #45

@lexoyo

This comment has been minimized.

Member

lexoyo commented Sep 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment