comments: browser display of context around url #targets should only be used for diff comments
24d01c64c5f3 had the unintended side-effect of not only highlighting the linked line in file view but also the spacing above it. For now, instead of changing the markup, restrict the fancy #target offsets to comments.
pullrequests: avoid unnecessary scroll-bar on short lists of available updates with Chrome
The surrounding div will apparently take size after the content but miss that there also is 1 pixel border ... and when drawing it realizes that there isn't enough space and it adds scroll bars anyway.
Work around that by giving the surrounding box 1 pixel padding.
The canvas will be set to the right size when the page has loaded. Until the page has been loaded, Chrome has been seen to set the canvas to be so wide that it would cover other visible links, thus preventing navigating away from the page before it has been fully loaded.
This will make sure the canvas never takes up more space than intended.
comments: previous/next links should only be to first comment for a given line
Before, each comment had its own link to previous/next comment - also if it was right before/after in the same thread. Comments right before/after in the same thread are however easy to navigate to with scrolling or arrow keys or pageup/down. Navigating to the next thread could take a lot of clicks to cycle through every single comment ever made.
Instead, just show the link at the top of each thread and let the links navigate between threads.
This also make long threads use less vertical space.
One downside of this change is that it for long threads takes a bit more work to get from the last comment in a thread to the next thread.
comments: rework/rewrite javascript for inline comment handling
There shouldn't be any functional changes here, but the focus has been on doing the right thing instead of looking at how it was before.
Incremental cleanup did not seem feasible. I think this is more readable and maintainable than before ... at least for me.
Existing snippets has been reused when reimplementing. The new implementation focus on being as simple as possible and well-structured. jQuery and data attributes are used instead of custom implementations. Some existing functions have been modified or renamed, others have been removed when they no longer were needed.
e-mail: properly handle no recipients when there is no email_to set
When the configuration file does not contain a value for email_to, and no recipients are specified in a call to send_email, recipients would be set to [None, admins] which causes an error when logging this list as ' '.join(recipients).
e-mail: revive dead code that checks for unspecified recipients
Commit 609e06b6c52f6a8ea9581372805c4bbb60db81a1 introduced dead code: in the beginning of send_email an assert verifies that recipients is of type 'list', so checking it for None later is useless and that branch can never be reached.
Instead of removing the dead code, revive it and add a test case.
privacy: don't tell users what is the reason for a failed login
Makes it harder for strangers to probe the instance for presence of certain users. This can make it harder to break in, as it is now harder to tell is a username or a password are wrong, so bruteforcing should probably take a bit longer if you don't know what exactly are you doing.
The result of db.User.get_dict never contains the keys 'api_keys' or 'permissions'. The keys returned by get_dict are 1) all the User table columns, 2) the keys explicitly defined in User.__json__, and 3) the keys defined in User.get_api_data, none of which include the two blacklisted keys.
'api_keys' would be returned if __json__ called get_api_data with argument details=True; but currently that is not the case.
In case there's a reason why these two keys must never appear in an AuthUser object, the check has not been removed entirely; instead, it's been turned into an assertion. This way, it will be noticed if __json__ is later modified to request detailed API data, for instance.
Because the method is only used by AuthUser, and also because it's an awful thing to do (copying a large but ill-defined set of attributes from one object to another), and we don't want usage to spread.
auth: construct AuthUser from either user_id or db.User object
If the caller already has the database User object, there's no reason for AuthUser to look it up again.
The `api_key` lookup functionality is dropped, because 1) it's only used in one place, and 2) it's simple enough for the caller to do the lookup itself.
The `user_id` lookup functionality is kept, because 1) it's frequently used, and 2) far from a simple `User.get(id)` lookup, it has a complex interaction with UserModel. (That cleanup will have to wait for another day.)
All calls of the form `AuthUser(user_id=x.user_id)` can be replaced with `AuthUser(dbuser=x)`, assuming `x` is a db.User. However, verifying that assumption requires a manual audit of every call site, since `x` might also be another `AuthUser` object, for instance. Therefore, only the most obvious call sites have been fixed here.
auth: refactor user lookup in AuthUser constructor for clarity
First, note that `fill_data` checks that the specified `db.User` is `active` before copying anything, and returns False if not.
Now, previously when calling e.g. `AuthUser(user_id=anonymous_user_id)`, `_propagate_data` would explicitly refuse to look up the anonymous user, but then fall back to the anonymous user anyway (if `active`), or use None values (if not `active`).
Given the same situation, the new code simply looks up the anonymous user like it would any other user, and copies data using `fill_data`. If the anonymous user is not `active`, we fall back to the existing code path and behave as before (that is, use None values).
auth: remove username from AuthUser session cookie
There's no reason to store the username when we store the user ID. We have load the user from database anyway under all circumstances, to verify e.g. that the user is (still) active.
This does not impact application code, but does impact a number of test cases which explicitly checks the username stored in the session.
AuthUser looks up the user based on the first of `user_id`, `api_key` and `username` that is not None *and* does not refer to the default (anonymous) user. In practice, the arguments always refer to the same user, so there's no point in specifying more than one of them.
auth: have fill_data take User object, not lookup key
Simplify the UserModel.fill_data interface by passing the actual db.User object instead of a key by which it can be looked up. This also saves a lookup in case the db.User is already loaded (which for now is only the case in one place, but that'll change in the following changesets).
UserModel: remove methods that are redundant with db.User
UserModel().get_by_api_key is exactly equivalent to User.get_by_api_key.
UserModel's get_by_username and get_by_email are not exactly identical to their db.User counterparts, due to a difference in the order of optional arguments. Fortunately, these optional arguments are never used.
admin: e-mail: don't repeat the e-mail settings from the .ini file
None of the settings from the .ini file are currently listed in the admin interface, except for e-mail. There is no need for such an exception: either we list all settings or none.
The repo "show" controller didn't do anything and was unused. There was a routing GET entry for it but it was only used for generating URLs for DELETE and PUT operations that have separate controllers that happen to have the same URL.
Use the right routing entries when generating URLs and drop the dead code.
cleanup: check for None object identity in cases where that is what the 'contract' says
Avoid applying bool() on complex objects - it might return something unexpected such as the key (which might be 0 and thus be false). Checking for None is more safe and faster.
e-mail: remove unused setting error_message from ini files
The ini files and templates contain a commented setting of 'error_message' which does not seem to be used. It is referring to the error_message variable in Paste, which has as description (Paste:ErrorMiddleware):
When debug mode is off, the error message to show to users.
However, setting this value apparently made no effect at all in Kallithea.