[PATCH 1/3] Start moving code from the controller to the logic
Bohuslav Kabrda
bkabrda at redhat.com
Wed Jan 23 11:58:18 UTC 2013
----- Original Message -----
> There are several advantages in doing that :
> - we can reuse it in the API
> - less coupling to the framework
> - easier unit-testing for the logic
> ---
>
> This is just moving some code around, following the design of
> Bohuslav (generic
> code goes into the logic package).
>
> coprs_frontend/coprs/logic/builds_logic.py | 16 +++++++++++
> coprs_frontend/coprs/logic/coprs_logic.py | 15 ++++++++++
> .../coprs/views/coprs_ns/coprs_general.py | 32
> ++++++++++------------
> 3 files changed, 46 insertions(+), 17 deletions(-)
>
> diff --git a/coprs_frontend/coprs/logic/builds_logic.py
> b/coprs_frontend/coprs/logic/builds_logic.py
> index 41575ee..519e5bc 100644
> --- a/coprs_frontend/coprs/logic/builds_logic.py
> +++ b/coprs_frontend/coprs/logic/builds_logic.py
> @@ -53,6 +53,22 @@ class BuildsLogic(object):
> return models.Build.query.filter(models.Build.id.in_(ids))
>
> @classmethod
> + def add_build(cls, pkgs, copr, owner):
> + build = models.Build(
> + pkgs = pkgs,
> + copr = copr,
> + repos = copr.repos,
> + chroots = ' '.join(map(
> + lambda x: x.chroot_name, copr.mock_chroots)
> + ),
> + user = owner,
> + submitted_on = int(time.time()))
> + # no need to check for authorization here
> + builds_logic.BuildsLogic.new(owner, build, copr,
> + check_authorized = False)
> + db.session.commit()
> +
> + @classmethod
> def new(cls, user, build, copr, check_authorized = True):
> if check_authorized:
> if not user.can_build_in(copr):
> diff --git a/coprs_frontend/coprs/logic/coprs_logic.py
> b/coprs_frontend/coprs/logic/coprs_logic.py
> index 45ea492..c39d566 100644
> --- a/coprs_frontend/coprs/logic/coprs_logic.py
> +++ b/coprs_frontend/coprs/logic/coprs_logic.py
> @@ -1,3 +1,5 @@
> +import time
> +
> from coprs import db
> from coprs import exceptions
> from coprs import helpers
> @@ -23,6 +25,19 @@ class CoprsLogic(object):
> return query
>
> @classmethod
> + def add_coprs(cls, name, repos, owner, selected_chroots):
> + copr = models.Copr(name = name,
> + repos = repos,
> + owner = owner,
> + created_on = int(time.time()))
> + CoprsLogic.new(owner, copr,
> + check_for_duplicates = False) # form validation checks
> for duplicates
> + CoprChrootLogic.new_from_names(owner, copr,
> + selected_chroots)
> + db.session.commit()
> + return copr
> +
> + @classmethod
> def get_multiple(cls, user, **kwargs):
> user_relation = kwargs.get('user_relation', None)
> username = kwargs.get('username', None)
> diff --git a/coprs_frontend/coprs/views/coprs_ns/coprs_general.py
> b/coprs_frontend/coprs/views/coprs_ns/coprs_general.py
> index 027845b..6fc53dc 100644
> --- a/coprs_frontend/coprs/views/coprs_ns/coprs_general.py
> +++ b/coprs_frontend/coprs/views/coprs_ns/coprs_general.py
> @@ -59,33 +59,31 @@ def copr_add():
> @coprs_ns.route('/new/', methods=['POST'])
> @login_required
> def copr_new():
> + """ Receive information from the user on how to create its new
> copr
> + and create it accordingly.
> + """
> form = forms.CoprFormFactory.create_form_cls()()
> if form.validate_on_submit():
> - copr = models.Copr(name = form.name.data,
> - repos = form.repos.data.replace('\n', '
> '),
> - owner = flask.g.user,
> - created_on = int(time.time()))
> - coprs_logic.CoprsLogic.new(flask.g.user, copr,
> check_for_duplicates = False) # form validation checks for
> duplicates
> - coprs_logic.CoprChrootLogic.new_from_names(flask.g.user,
> copr, form.selected_chroots)
> - db.session.commit()
> + copr = CoprsLogic.add_coprs(cls,
> + name=form.name.data,
> + repos=form.repos.data.replace('\n', ' '),
> + owner=flask.g.user,
> + selected_chroots=form.selected_chroots)
> flask.flash('New copr was successfully created.')
>
> if form.initial_pkgs.data:
> - build = models.Build(pkgs =
> form.initial_pkgs.data.replace('\n', ' '),
> - copr = copr,
> - repos = copr.repos,
> - chroots = ' '.join(map(lambda x:
> x.chroot_name, copr.mock_chroots)),
> - user = flask.g.user,
> - submitted_on = int(time.time()))
> - # no need to check for authorization here
> - builds_logic.BuildsLogic.new(flask.g.user, build, copr,
> check_authorized = False)
> - db.session.commit()
> - flask.flash('Initial packages were successfully
> submitted for building.')
> + BuildsLogic.add_build(
> + pkgs=form.initial_pkgs.data.replace('\n', ' '),
> + copr=copr,
> + owner=flask.g.user)
> + flask.flash('Initial packages were successfully
> submitted '
> + 'for building.')
>
> return flask.redirect(flask.url_for('coprs_ns.coprs_show'))
> else:
> return flask.render_template('coprs/add.html', form = form)
>
> +
> @coprs_ns.route('/detail/<username>/<coprname>/')
> def copr_detail(username, coprname):
> query = coprs_logic.CoprsLogic.get(flask.g.user, username,
> coprname)
> --
> 1.8.1
>
Few comments:
- I tend to add "user" (who is doing the operation) as argument to all *Logic methods, since this may be needed for some authorization checking etc. Although your methods don't need that, would you please add the argument for consistency?
- I would prefer seeing db.session.commit() in views, not *Logic methods. The reason is that if you're doing three logic functions and each one of them commits, it's three separate DB queries - I don't think we want that. That might turn out to be slow (and also with single commit, you either do the _whole_ transaction or nothing of it).
- The add_build method should use flask.g.user as a build submitted to be more general and usable from other views, too.
Maybe we should put together some guidelines about methods of *Logic classes and check them strictly, otherwise we will end up very badly soon ;)
--
Regards,
Bohuslav "Slavek" Kabrda.
More information about the copr-devel
mailing list