-
Notifications
You must be signed in to change notification settings - Fork 344
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
Improve js client #931
Improve js client #931
Conversation
@@ -1,6 +1,154 @@ | |||
/** | |||
* db functions: client data repository | |||
*/ | |||
|
|||
|
|||
selfoss.dbOnline = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand the name. Under database, I usually imagine something that stores data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here it fetches data. I'm open to other propositions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was there any particular motivation for moving it into a separate file?
The main issue I have with this is probably the entangledness of the architecture, though refactoring it is a long term goal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes the base file a little less long and makes sense regarding my work in https://github.com/niol/selfoss/blob/ppr/offline/public/js/selfoss-db.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I see it, these three objects form the model layer. Optimally, the dbOnline
object should only take care of fetching/uploading data from the server. It could then be called synchronizer
. The dbOffline
should only be a nice wrapper above dexie
, I would call it storage
. Last, the db
object could be an envelope above synchronizer
and storage
, providing an opaque model for the rest of the app. What I find the most important, none of the object should have any access to DOM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not really convinced the whole functions should be moved here. I would only move the promise of AJAX request and keep the rest in selfoss-base
. In future, the promise would also internally handle the database but the DOM manipulation does not really fit here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay then you can rebase this patch out later.
controllers/Index.php
Outdated
} | ||
|
||
if ($error !== null) { | ||
$view->jsonSuccess([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we use jsonSuccess
instead of jsonError
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It made not much sense but the login API was like that before so I did ont change et.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I will take care of this in the refactoring pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jsonError adds a 400 Bad Request
error. If password or username are missing, it is a bad request, so you are right, I'll fix this. This is not the case for the auth error.
@@ -4,6 +4,60 @@ | |||
selfoss.ui = { | |||
|
|||
|
|||
showLogin: function(error) { | |||
var error = (typeof error !== 'undefined') ? error : ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe taking care of this in css would be simpler:
#nav-refresh, … {
display: none;
}
body.publicmode #nav-refresh, body.loggedin #nav-refresh, … {
display: block;
}
public/js/selfoss-base.js
Outdated
@@ -75,7 +90,9 @@ var selfoss = { | |||
window.setInterval(selfoss.dbOnline.sync, 60*1000); | |||
|
|||
window.setInterval(selfoss.ui.refreshEntryDatetimes, 60*1000); | |||
}); | |||
|
|||
selfoss.ui.showMainUi(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can still reproduce this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot manage to reproduce. Can you please describe the steps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After you log in it seems to work fine but if you refresh the page, it overflows. You need to have enough tags or items to fill the whole area, or resize the window before reloading the page. I can reproduce it in both Firefox and Chomium.
public/js/selfoss-base.js
Outdated
@@ -75,7 +90,9 @@ var selfoss = { | |||
window.setInterval(selfoss.dbOnline.sync, 60*1000); | |||
|
|||
window.setInterval(selfoss.ui.refreshEntryDatetimes, 60*1000); | |||
}); | |||
|
|||
selfoss.ui.showMainUi(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can still reproduce this.
public/js/selfoss-db.js
Outdated
type: 'GET', | ||
dataType: 'json', | ||
data: { | ||
since: selfoss.db.lastUpdate.toISOString(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am getting TypeError: selfoss.db.lastUpdate is undefined
here.
templates/home.phtml
Outdated
@@ -52,7 +52,7 @@ | |||
</head> | |||
<body class=" | |||
<?= $this->publicMode === true ? 'publicmode' : ''; ?> | |||
<?= $this->loggedin === true ? 'loggedin' : 'notloggedin'; ?> | |||
<?= $this->authEnabled === true ? 'auth' : ''; ?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still getting the auth form even when the authentication is disabled.
public/css/style.css
Outdated
} | ||
|
||
#loginform.loading { | ||
background: url('images/fancybox_loading.gif') center center no-repeat; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use fancybox’s loading animation instead of the selfoss one from #content.loading
?
controllers/Tags.php
Outdated
* | ||
* @return string | ||
*/ | ||
public function tagsAddColors($itemTags, $tags = null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you document the arguments?
controllers/Tags.php
Outdated
* | ||
* @return array tag color array | ||
*/ | ||
private function getTagsWithColors($tags) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add array
type hint?
|
||
|
||
hasSession: function() { | ||
selfoss.loggedin = Cookies.get('onlineSession') == 'true'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about when PHP session expires (simulated by deleting the PHPSESSID
cookie).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This cookie is a hint that there was an online session when last loaded, and will be removed at startup when PHPSESSID is cleared. When that PHPSESSID is cleared, you get like before the 403 error after each ajax request. The next step is to fix #919 .
|
||
|
||
logout: function() { | ||
selfoss.clearSession(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should be done only after the user was logged out, to make the deauthentication errors more visible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, this is all done with offline in mind and is only a hint that there was an online session last time.
@@ -1,6 +1,154 @@ | |||
/** | |||
* db functions: client data repository | |||
*/ | |||
|
|||
|
|||
selfoss.dbOnline = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not really convinced the whole functions should be moved here. I would only move the promise of AJAX request and keep the rest in selfoss-base
. In future, the promise would also internally handle the database but the DOM manipulation does not really fit here.
* | ||
* @return void | ||
*/ | ||
sync: function(force) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for this function.
templates/home.phtml
Outdated
<li id="nav-filter-unread" class="nav-filter-unread" role="link" tabindex="0"><?= \F3::get('lang_unread')?> <span class="unread-count"></span></li> | ||
<li id="nav-filter-starred" class="nav-filter-starred" role="link" tabindex="0"><?= \F3::get('lang_starred') ?> <span></span></li> | ||
<div id="loginform"> | ||
<form action="<?= $this->base; ?>?login=1" method="post"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The login
query string parameter is no longer needed.
@@ -37,20 +37,6 @@ | |||
</head> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file can be renamed.
e73da33
to
3a1c79e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not really like the dbOnline but splitting it in a clean way won’t be possible without a significant refactoring. I will merge it as is for now.
Pull Request #931 moved the authentication client side, which worsened the issue #919 by displaying error instead of the login form on opening selfoss when user did not log out and their session expired on the server. This patch redirects user to login form whenever client side session is registered and 403 Forbidden error is received. This is a temporary fix before offline support is merged.
Pull Request #931 moved the authentication client side, which worsened the issue #919 by displaying error instead of the login form on opening selfoss when user did not log out and their session expired on the server. This patch redirects user to login form whenever client side session is registered and 403 Forbidden error is received. This is a temporary fix before offline support is merged.
Some minor enhancements and refactoring, use API for login and update item datetimes client-side.