helpers.py imported tons of unused names. Most of these are assumed to be due to historical changes without cleanup. Several webhelpers names were imported as a group, but this is not deemed useful.
helpers: move Page/RepoPage to a separate file page.py
The Page and RepoPage classes are not used from templates and thus do not belong in helpers. They could have been put in utils.py, but because they are completely standalone we can easily split them to a separate file.
model: rename db.BaseModel to avoid confusion with base.BaseModel
There exist two BaseModel classes in kallithea.model: - kallithea.model.base.BaseModel (previously kallithea.model.BaseModel) - kallithea.model.db.BaseModel
Needless to say, this is very confusing, so rename one of them.
Having too much code, in particular too much imports, inside a package's __init__.py is a recipe for circular imports, and considered bad practice in Python [1]
Move out everything from kallithea/model/__init__.py to a new file kallithea/model/base.py and adapt the existing imports.
helpers: allow to set class for repo_link breadcrumb links
This helps Bootstrap support.
(Since this is a high level function that returns several links and not just a single html element, the patch has been modified to only allow specifying a class that is used for all the links.)
Don't take a database object, grab its ID, then use the ID to load the same database object once again. This also applies to creating AuthUser objects, which will happily take a User object instead of a user_id.
cleanup: don't check if currently logged-in user exists
Don't check if the currently logged-in User exists. Every request runs in a database transaction; if the object existed at the start, it still exists (unless we add a way for a user to delete themselves).
Rename the 'user_id' field to 'author_id' and replace other references to the comment 'user' throughout the model. The database column name 'user_id' remain unchanged for now; a later Alembic script can fix the name of these and other columns to match their Python name.
db: PullRequest/Repository/RepoGroup/UserGroup: change 'user' to 'owner'
Rename the 'user' and 'user_id' fields on the four classes to something more informative. The database column names remain unchanged for now; a later Alembic script can fix the name of these and other columns to match their Python name.
This might break rcextensions, though, and external scripts that use the HTML form interface.
Don't re-add objects to the SQLAlchemy Session just because they were modified. Session.add is only for freshly constructed objects that SQLAlchemy doesn't know about yet.
The rules are quite simple:
When creating a database object by calling the constructor directly, it must explicitly be added to the session.
When creating an object using a factory function (like "create_repo"), the returned object has already (by convention) been added to the session, and should not be added again.
When getting an object from the session (via Session.query or any of the utility functions that look up objects in the database), it's already added, and should not be added again. SQLAlchemy notices attribute modifications automatically for all objects it knows about.
db: do case-insensitive explicit sorting of RepoGroup names
This does not change the implicit sorting enabled via __mapper_args__, which is a bad idea anyway (and now deprecated), and will have to be dealt with in a later changeset.
The use of __mapper_args__ implicitly adds sorting to every query of RepoGroup objects throughout the code (including implicit queries via relationships). For the relationships, __mapper_args can be replaced with "order_by" on each individual relationship, and it's reasonably straight-forward to identify every RepoGroup query throughout the code, and add explicit sorting. But we don't really need that sorting most of the time, so a better way forward may be to identify all the places that actually needs the sorting, make it explicit there, and then kill the __mapper_args__. (Anyway, future work.)
db: always do case-insensitive sorting of repository names
We retain the implicit order_by on the follows_repository relationship. This is probably a bad idea, since it causes sorting even when it's not needed; but for now, at least, we consistently do case-insensitive sort.
This makes database query code more explicit and increases readability.
E.g. the function name get_pullrequest_cnt_for_user was bad, because the concept of "pullrequest for user" is incredibly vague, and could refer to any kind of association between PRs and users. (Quiz time! Does it mean that the user is the PR owner, that the user is reviewing, or that the user has commented on the PR and thus is receiving notifications?)
A descriptive name could be "get_open_pull_request_count_for_reviewer", because the function is indeed only concerned with reviewers and only with open pull requests. But at this point, we might as well say PullRequest.query(reviewer_id=user, include_closed=False).count() which is only slightly longer, and doesn't require us to write dozens of little wrapper functions (including, any moment now, a separate function for listing the PRs instead of counting them).
Note that we're not actually going down an abstraction level by doing this. We're still operating on the concepts of "pull request", "open" and "reviewer", and are not leaking database implementation details.
The query() shortcuts are designed so they default to not altering the query. Any processing requires explicit opt-in by the caller.
BaseRepoController already provides the repository object in c.db_repo (may be None) and BaseController already provides the repository name in c.repo_name (as given in the URL).
(Arguably, that's a bad design, and we should revisit that decision in the future. For now, the code just performs slightly better.)
This method saves basically no typing, compared to "query().all()". Additionally, "all()" returns a list, forcing all records to be loaded into a memory at the same time, but some callers just need to iterate over the objects one at a time, in which case "query()" alone is more efficient. In one case, the caller can even use "count()" and avoid loading any objects from the database at all.
Turbogears2 migration: replace pylons.url by kallithea.config.routing.url
In preparation for the migration to Turbogears2, introduce a kallithea.config.routing.url to replace pylons.url. The implementation is basically the same: wrap around routes.url().
This change involves: - a number of import statement changes - fixing some tests in test_libs.py; to avoid duplication, the different implementations of fake_url were grouped in one place.
This change was first proposed by Alessandro Molina in his initial port. Following changes were made afterwards: - move UrlGenerator from kallithea.lib.utils to kallithea.config.routing - add documentation to UrlGenerator - kallithea/lib/auth.py used url_for instead of url, for no apparent reason so this was changed. - fix libs tests - rebase onto Pylons-based Kallithea first
tests: fix assertion rewriting in some tests with pytest-3.0.0+
Since pytest 3.0.0, asserts present in modules that are not directly seen by pytest as 'test modules', are no longer rewritten to have improved reporting, unless they are explicitly marked as up-for-rewriting. Rationale from pytest upstream:
However since we do not want to test different bytecode then you will run in production this hook only re-writes test modules themselves as well as any modules which are part of plugins. Any other imported module will not be re-written and normal assertion behaviour will happen.
This is e.g. the case for asserts in files like kallithea/tests/api/api_base.py and kallithea/tests/models/common.py.
This commit registers all modules below kallithea.tests for assert rewriting, but note that asserts in kallithea/tests/__init__.py itself are not rewritten as kallithea.tests is already imported when the register statement is encountered. Moving the register statement to kallithea/__init__.py would fix that, but even then asserts in kallithea/tests/__init__.py seem not to be rewritten (let alone the issue that we do not want a pytest dependency enforced in kallithea/__init__.py which is also used in production). Moving the code from kallithea/tests/__init__.py to a proper module, as suggested by Mads Kiilerich, solves that problem.
tests: add as little code as possible in __init__.py
kallithea/tests/__init__.py contained quite a lot of code, including the test base class TestController. This in itself may be considered bad practice.
Specifically, this poses a problem when using pytest 3.0+, in which asserts in some files are not automatically rewritten to give improved assert output. That problem can be fixed by explicitly registering such files for assertion rewriting, but that register call should be executed _before_ said files are imported. I.e. if the register call is in kallithea/tests/__init__.py, assert calls in __init__.py itself can not be rewritten.
Since the TestController base class does effectively contain asserts, and we do not want to execute the register call from somewhere outside the kallithea/tests directory, we need to move the TestController class to another file (kallithea/tests/base.py) so we can have a register call in __init__.py before loading base.py.
While not strictly necessary to fix the mentioned pytest problem, we take the opportunity to fully clean __init__.py and move everything to the new kallithea/tests/base.py. While doing so, unnecessary imports are removed, and imports are ordered alphabetically. Explicit imports of symbols from modules that were already imported as a whole, are removed in favor of fully qualifying the references (e.g. tempfile._RandomNameSequence).
Turbogears2 migration: remove some references to Pylons in comments
In order to minimize the diff of the actual Turbogears2 migration, this commit already removes certain unnecessary references to Pylons from the Kallithea source base. Places where the reference to Pylons is important are still kept for now, as well as references in kallithea/config where many changes are made for Turbogears2 anyway.
Turbogears2 migration: use sqlalchemy.url iso sqlalchemy.db1.url
In Turbogears2, much of the application initialization is handled by the framework, whereas in Pylons the application was responsible for it. Initializing SQLAlchemy is one such part of initialization which is handled by Turbogears2.
Turbogears2 expects the configuration file to refer to the database using 'sqlalchemy.url' rather than the current 'sqlalchemy.db1.url'. While the exact name is not really important, not following this approach means we'll need to override the sqlalchemy initialization method.
Therefore, as a preparation to the Turbogears2 migration, already change the database reference string under Pylons. When upgrading to a version of Kallithea containing this commit, the .ini file will manually need to be adapted to remove the .db1 strings.
lib: simplify version; drop get_current_revision - it was run every time on import time - we don't want that
This functionality was already not in use with the version numbers we use. I also doubt it is possible to have version numbers where it worked - it seems like there might have been some regression that nobody noticed. Dropping it will thus not really lose any actual functionality.
If we want to include the revision number in the version (for browser cache invalidation) then we should do it in a different way. Perhaps writing it to some file from setup.py .
utils: when building a Mercurial ui object with configuration, don't use 'None' for NULL values
If Ui had an entry with ui_section='extensions', ui_key='largefiles', ui_value=NULL it would be passed to Mercurial as if the .ini file had [extensions] largefiles = None and it would fail to load the largefiles extension because it couldn't find './None/'.
Note: get_current_revision might currently mask this problem. It will not get the Ui from the database and will thus read a normal .ini file from the system and (if configured) read the largefiles extension from the default location. That will make the largefiles extension available for later largefiles imports even if they specify the bogus path.
As f8a714c2c5a1 noted in a FIXME: ui_value should perhaps not be nullable.
For now, just handle NULL in extension configuration.
helpers: introduce ascii preserving visual markup of *bold*
No matter what kind of text (code and markup) is parsed, it will at most have slightly odd styling. Nothing will be lost and it can be copy-pasted correctly.
Other kinds of markup (like _underline_ and /italic/) has also been considered ... but they will too often interfere with valid code and command snippets.
We inline it so we eventually can match all patterns in the same regexp and thus avoid problems with parsing of formatted html. Inlining it will also make repo_name and other parameters easily available.
helpers: in urlify_text, use <br> for newlines in pre-formatted text so it can be cut'n'pasted correctly
Before, when copying the preformatted nice looking ASCII-artsy text in a PR or changeset description from a browser to a text editor, it would be pasted as text without newline. Simply put, copy/paste ignores that it is inside a <pre> / white-space:pre-wrap section.
Instead, translate newlines to <br> which always translates to a newline when pasted.
celery: use Celery 3 config settings instead of deprecated
As warned by: The 'CELERYD_LOG_LEVEL' setting is scheduled for deprecation in version 2.4 and removal in version v4.0. Use the --loglevel argument instead remove celeryd.log.file and celeryd.log.level from the ini file. Instead, use: paster celeryd my.ini --loglevel=DEBUG --logfile=my.log or, in the future: gearbox celeryd -c my.ini -- --loglevel=DEBUG --logfile=my.log
As warned by: The 'BROKER_VHOST' setting is scheduled for deprecation in version 2.5 and removal in version v4.0. Use the BROKER_URL setting instead The 'BROKER_HOST' setting is scheduled for deprecation in version 2.5 and removal in version v4.0. Use the BROKER_URL setting instead The 'BROKER_USER' setting is scheduled for deprecation in version 2.5 and removal in version v4.0. Use the BROKER_URL setting instead The 'BROKER_PASSWORD' setting is scheduled for deprecation in version 2.5 and removal in version v4.0. Use the BROKER_URL setting instead The 'BROKER_PORT' setting is scheduled for deprecation in version 2.5 and removal in version v4.0. Use the BROKER_URL setting instead change the .ini template to use: broker.url = amqp://rabbitmq:qewqew@localhost:5672/rabbitmqhost
As warned by: Starting from version 3.2 Celery will refuse to accept pickle by default.
The pickle serializer is a security concern as it may give attackers the ability to execute any command. It's important to secure your broker from unauthorized access when using pickle, so we think that enabling pickle should require a deliberate action and not be the default choice.
If you depend on pickle then you should set a setting to disable this warning and to be sure that everything will continue working when you upgrade to Celery 3.2::
Celery 3 is more than 2 and is the future and should be more stable than Celery 2.
There might be problems like PRECONDITION_FAILED - cannot redeclare exchange 'celeryresults' in vhost 'kallithea' with different type, durable, internal or autodelete value when celery is upgraded and it tries to upgrade existing tasks.
One way to avoid that problem: - make sure celery has run to completion and there are no important pending tasks, - delete the old vhost, - upgrade celery, - create new vhost - set permissions for vhost - profit