[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