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.

Sign up for GitHub

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

Jump to bottom

Single service mode #68

Merged
merged 7 commits into from Aug 31, 2018
Merged

Single service mode #68

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
58e90c7
fixed dropbox default callback route
lexoyo Aug 24, 2018
20815d5
fixed detect a unifile service thx to isLoggedIn since this attribute…
lexoyo Aug 24, 2018
958e60e
single service mode closes #62
lexoyo Aug 24, 2018
369e97a
added isLoggedIn info returned by server for services, added logout
lexoyo Aug 26, 2018
f378ba5
initDone flag in the state of CloudExplorer component
lexoyo Aug 31, 2018
21f1e66
initDone flag in the state of CloudExplorer component
lexoyo Aug 31, 2018
c559016
fixed some linter errors
lexoyo Aug 31, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions lib/index.js
View file
Open in desktop
Expand Up @@ -31,18 +31,18 @@ const router = new Router({
dropbox: {
clientId: process.env.DROPBOX_APP_ID || '8lxz0i3aeztt0im',
clientSecret: process.env.DROBOX_APP_SECRET || 'twhvu6ztqnefkh6',
redirectUri: process.env.DROPBOX_APP_REDIRECT || `${rootUrl}/dropbox/oauth_callback`,
redirectUri: process.env.DROPBOX_APP_REDIRECT || `${rootUrl}/ce/dropbox/oauth_callback`,
state: 'aaathub'
},
fs: {
sandbox: Os.homedir(),
showHiddenFile: false
},
ftp: {redirectUri: `${rootUrl}/ftp/signin`},
ftp: {redirectUri: `${rootUrl}/ce/ftp/signin`},
github: {
clientId: process.env.GITHUB_APP_ID || 'f124e4148bf9d633d58b',
clientSecret: process.env.GITHUB_APP_SECRET || '1a8fcb93d5d0786eb0a16d81e8c118ce03eefece',
redirectUri: process.env.GITHUB_APP_REDIRECT || `${rootUrl}/github/oauth_callback`,
redirectUri: process.env.GITHUB_APP_REDIRECT || `${rootUrl}/ce/github/oauth_callback`,
state: 'aaathub'
}
});
Expand Down
9 changes: 9 additions & 0 deletions src/js/CloudExplorer.jsx
View file
Open in desktop
Expand Up @@ -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*/) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think initDone should be part of the component state.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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...

this.unifile.cd([files[0].name], true)
.then(path => {
this.props.onCd(path);
});
}
this.initDone = true;
}
})
.catch((e) => this.onUnifileError(e));
Expand Down
7 changes: 4 additions & 3 deletions src/js/UnifileService.js
View file
Open in desktop
Expand Up @@ -124,9 +124,9 @@ export default class UnifileService {
});
}

cd (path) {
cd (path, preventAuth=false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

CloudExplorer2/src/js/UnifileService.js

Line 56 in 958e60e

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this check is weird (it was weird already)

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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

this.auth(path[0])
.then(() => {
this.currentPath = path;
Expand Down Expand Up @@ -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';
Copy link
Contributor

Choose a reason for hiding this comment

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

weird, pls comment

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}
}