Bug 1434064 - Refactor Project.pm to use Moo for better type checking

3 files changed, 154 insertions, 159 deletions

diff --git a/extensions/PhabBugz/bin/update_project_members.pl b/extensions/PhabBugz/bin/update_project_members.pl
index 06cc55626..a0449a915 100755
--- a/extensions/PhabBugz/bin/update_project_members.pl
+++ b/extensions/PhabBugz/bin/update_project_members.pl
@@ -55,10 +55,10 @@ my $sync_groups = Bugzilla::Group->match({ name => [ split('[,\s]+', $phab_sync_
 foreach my $group (@$sync_groups) {
     # Create group project if one does not yet exist
     my $phab_project_name = 'bmo-' . $group->name;
-    my $project = Bugzilla::Extension::PhabBugz::Project->new({
+    my $project = Bugzilla::Extension::PhabBugz::Project->new_from_query({
         name => $phab_project_name
     });
-    if (!$project->id) {
+    if (!$project) {
         $project = Bugzilla::Extension::PhabBugz::Project->create({
             name        => $phab_project_name,
             description => 'BMO Security Group for ' . $group->name
diff --git a/extensions/PhabBugz/lib/Policy.pm b/extensions/PhabBugz/lib/Policy.pm
index 3205562c3..23f04b354 100644
--- a/extensions/PhabBugz/lib/Policy.pm
+++ b/extensions/PhabBugz/lib/Policy.pm
@@ -83,9 +83,8 @@ sub new_from_query {
     my ($class, $params) = @_;
     my $result = request('policy.query', $params);
     if (exists $result->{result}{data} && @{ $result->{result}{data} }) {
-        return $result->{result}->{data}->[0];
+        return $class->new($result->{result}->{data}->[0]);
     }
-    return $class->new($result);
 }
 
 sub create {
@@ -105,7 +104,7 @@ sub create {
     if (@$project_names) {
         my $project_phids = [];
         foreach my $project_name (@$project_names) {
-            my $project = Bugzilla::Extension::PhabBugz::Project->new({ name => $project_name });
+            my $project = Bugzilla::Extension::PhabBugz::Project->new_from_query({ name => $project_name });
             push @$project_phids, $project->phid if $project;
         }
 
@@ -134,7 +133,7 @@ sub _build_rule_projects {
     return [
         map  { $_->name }
         grep { $_ }
-        map  { Bugzilla::Extension::PhabBugz::Project->new( { phids => [$_] } ) }
+        map  { Bugzilla::Extension::PhabBugz::Project->new_from_query( { phids => [$_] } ) }
         @{ $rule->{value} }
     ];
 }
diff --git a/extensions/PhabBugz/lib/Project.pm b/extensions/PhabBugz/lib/Project.pm
index ec00b9532..91dc2133d 100644
--- a/extensions/PhabBugz/lib/Project.pm
+++ b/extensions/PhabBugz/lib/Project.pm
@@ -8,46 +8,66 @@
 package Bugzilla::Extension::PhabBugz::Project;
 
 use 5.10.1;
-use strict;
-use warnings;
+use Moo;
+use Types::Standard -all;
+use Type::Utils;
 
 use Bugzilla::Error;
 use Bugzilla::Util qw(trim);
 use Bugzilla::Extension::PhabBugz::Util qw(
-    request
-    get_phab_bmo_ids
+  request
+  get_phab_bmo_ids
 );
 
-use Types::Standard -all;
-use Type::Utils;
+#########################
+#    Initialization     #
+#########################
+
+has id              => ( is => 'ro', isa => Int );
+has phid            => ( is => 'ro', isa => Str );
+has type            => ( is => 'ro', isa => Str );
+has name            => ( is => 'ro', isa => Str );
+has description     => ( is => 'ro', isa => Str );
+has creation_ts     => ( is => 'ro', isa => Str );
+has modification_ts => ( is => 'ro', isa => Str );
+has view_policy     => ( is => 'ro', isa => Str );
+has edit_policy     => ( is => 'ro', isa => Str );
+has join_policy     => ( is => 'ro', isa => Str );
+has members_raw     => ( is => 'ro', isa => ArrayRef [ Dict [ phid => Str ] ] );
+has members => ( is => 'lazy', isa => ArrayRef [Object] );
+
+sub new_from_query {
+    my ( $class, $params ) = @_;
+
+    my $data = {
+        queryKey    => 'all',
+        attachments => { members => 1 },
+        constraints => $params
+    };
+
+    my $result = request( 'project.search', $data );
+    if ( exists $result->{result}{data} && @{ $result->{result}{data} } ) {
+        return $class->new( $result->{result}{data}[0] );
+    }
+}
+
+sub BUILDARGS {
+    my ( $class, $params ) = @_;
+
+    $params->{name}            = $params->{fields}->{name};
+    $params->{description}     = $params->{fields}->{description};
+    $params->{creation_ts}     = $params->{fields}->{dateCreated};
+    $params->{modification_ts} = $params->{fields}->{dateModified};
+    $params->{view_policy}     = $params->{fields}->{policy}->{view};
+    $params->{edit_policy}     = $params->{fields}->{policy}->{edit};
+    $params->{join_policy}     = $params->{fields}->{policy}->{join};
+    $params->{members_raw}     = $params->{attachments}->{members}->{members};
 
-my $SearchResult = Dict[
-    id     => Int,
-    type   => Str,
-    phid   => Str,
-    fields => Dict[
-        name         => Str,
-        slug         => Str,
-        depth        => Int,
-        milestone    => Maybe[Str],
-        parent       => Maybe[Str],
-        icon         => Dict[ key => Str, name => Str, icon => Str ],
-        color        => Dict[ key => Str, name => Str ],
-        dateCreated  => Int,
-        dateModified => Int,
-        policy       => Dict[ view => Str, edit => Str, join => Str ],
-        description  => Maybe[Str]
-    ],
-    attachments => Dict[
-        members => Dict[
-            members => ArrayRef[
-                Dict[
-                    phid => Str
-                ],
-            ],
-        ],
-    ],
-];
+    delete $params->{fields};
+    delete $params->{attachments};
+
+    return $params;
+}
 
 # {
 #   "data": [
@@ -108,44 +128,14 @@ my $SearchResult = Dict[
 # }
 
 #########################
-#    Initialization     #
-#########################
-
-sub new {
-    my ($class, $params) = @_;
-    my $self = $params ? _load($params) : {};
-    $SearchResult->assert_valid($self);
-    return bless($self, $class);
-}
-
-sub _load {
-    my ($params) = @_;
-
-    my $data = {
-        queryKey    => 'all',
-        attachments => {
-            members => 1
-        },
-        constraints => $params
-    };
-
-    my $result = request('project.search', $data);
-    if (exists $result->{result}{data} && @{ $result->{result}{data} }) {
-        return $result->{result}->{data}->[0];
-    }
-
-    return $result;
-}
-
-#########################
 #     Modification      #
 #########################
 
 sub create {
-    my ($class, $params) = @_;
+    my ( $class, $params ) = @_;
 
-    my $name = trim($params->{name});
-    $name || ThrowCodeError('param_required', { param => 'name' });
+    my $name = trim( $params->{name} );
+    $name || ThrowCodeError( 'param_required', { param => 'name' } );
 
     my $description = $params->{description} || 'Need description';
     my $view_policy = $params->{view_policy} || 'admin';
@@ -154,19 +144,20 @@ sub create {
 
     my $data = {
         transactions => [
-            { type => 'name',        value => $name        },
+            { type => 'name',        value => $name },
             { type => 'description', value => $description },
             { type => 'edit',        value => $edit_policy },
             { type => 'join',        value => $join_policy },
             { type => 'view',        value => $view_policy },
-            { type => 'icon',        value => 'group'      },
-            { type => 'color',       value => 'red'        }
+            { type => 'icon',        value => 'group' },
+            { type => 'color',       value => 'red' }
         ]
     };
 
-    my $result = request('project.edit', $data);
+    my $result = request( 'project.edit', $data );
 
-    return $class->new({ phids => $result->{result}{object}{phid} });
+    return $class->new_from_query(
+        { phids => $result->{result}{object}{phid} } );
 }
 
 sub update {
@@ -177,137 +168,142 @@ sub update {
         transactions     => []
     };
 
-    if ($self->{set_name})  {
-        push(@{ $data->{transactions} }, {
-            type  => 'name',
-            value => $self->{set_name}
-        });
+    if ( $self->{set_name} ) {
+        push(
+            @{ $data->{transactions} },
+            {
+                type  => 'name',
+                value => $self->{set_name}
+            }
+        );
     }
 
-    if ($self->{set_description})  {
-        push(@{ $data->{transactions} }, {
-            type  => 'description',
-            value => $self->{set_description}
-        });
+    if ( $self->{set_description} ) {
+        push(
+            @{ $data->{transactions} },
+            {
+                type  => 'description',
+                value => $self->{set_description}
+            }
+        );
     }
 
-    if ($self->{set_members}) {
-        push(@{ $data->{transactions} }, {
-            type  => 'members.set',
-            value => $self->{set_members}
-        });
+    if ( $self->{set_members} ) {
+        push(
+            @{ $data->{transactions} },
+            {
+                type  => 'members.set',
+                value => $self->{set_members}
+            }
+        );
     }
     else {
-        if ($self->{add_members}) {
-            push(@{ $data->{transactions} }, {
-                type  => 'members.add',
-                value => $self->{add_members}
-            });
+        if ( $self->{add_members} ) {
+            push(
+                @{ $data->{transactions} },
+                {
+                    type  => 'members.add',
+                    value => $self->{add_members}
+                }
+            );
         }
 
-        if ($self->{remove_members}) {
-            push(@{ $data->{transactions} }, {
-                type  => 'members.remove',
-                value => $self->{remove_members}
-            });
+        if ( $self->{remove_members} ) {
+            push(
+                @{ $data->{transactions} },
+                {
+                    type  => 'members.remove',
+                    value => $self->{remove_members}
+                }
+            );
         }
     }
 
-    if ($self->{set_policy}) {
-        foreach my $name ("view", "edit") {
+    if ( $self->{set_policy} ) {
+        foreach my $name ( "view", "edit" ) {
             next unless $self->{set_policy}->{$name};
-            push(@{ $data->{transactions} }, {
-                type  => $name,
-                value => $self->{set_policy}->{$name}
-            });
+            push(
+                @{ $data->{transactions} },
+                {
+                    type  => $name,
+                    value => $self->{set_policy}->{$name}
+                }
+            );
         }
     }
 
-    my $result = request('project.edit', $data);
+    my $result = request( 'project.edit', $data );
 
     return $result;
 }
 
 #########################
-#      Accessors        #
-#########################
-
-sub id              { return $_[0]->{id};                          }
-sub phid            { return $_[0]->{phid};                        }
-sub type            { return $_[0]->{type};                        }
-sub name            { return $_[0]->{fields}->{name};              }
-sub description     { return $_[0]->{fields}->{description};       }
-sub creation_ts     { return $_[0]->{fields}->{dateCreated};       }
-sub modification_ts { return $_[0]->{fields}->{dateModified};      }
-
-sub view_policy { return $_[0]->{fields}->{policy}->{view}; }
-sub edit_policy { return $_[0]->{fields}->{policy}->{edit}; }
-sub join_policy { return $_[0]->{fields}->{policy}->{join}; }
-
-sub members_raw { return $_[0]->{attachments}->{members}->{members}; }
-
-sub members {
-    my ($self) = @_;
-    return $self->{members} if $self->{members};
-
-    my @phids;
-    foreach my $member (@{ $self->members_raw }) {
-        push(@phids, $member->{phid});
-    }
-
-    return [] if !@phids;
-
-    my $users = get_phab_bmo_ids({ phids => \@phids });
-
-    my @members;
-    foreach my $user (@$users) {
-        my $member = Bugzilla::User->new({ id => $user->{id}, cache => 1});
-        $member->{phab_phid} = $user->{phid};
-        push(@members, $member);
-    }
-
-    return \@members;
-}
-
-#########################
 #       Mutators        #
 #########################
 
 sub set_name {
-    my ($self, $name) = @_;
+    my ( $self, $name ) = @_;
     $name = trim($name);
     $self->{set_name} = $name;
 }
 
 sub set_description {
-    my ($self, $description) = @_;
+    my ( $self, $description ) = @_;
     $description = trim($description);
     $self->{set_description} = $description;
 }
 
 sub add_member {
-    my ($self, $member) = @_;
+    my ( $self, $member ) = @_;
     $self->{add_members} ||= [];
     my $member_phid = blessed $member ? $member->phab_phid : $member;
-    push(@{ $self->{add_members} }, $member_phid);
+    push( @{ $self->{add_members} }, $member_phid );
 }
 
 sub remove_member {
-    my ($self, $member) = @_;
+    my ( $self, $member ) = @_;
     $self->{remove_members} ||= [];
     my $member_phid = blessed $member ? $member->phab_phid : $member;
-    push(@{ $self->{remove_members} }, $member_phid);
+    push( @{ $self->{remove_members} }, $member_phid );
 }
 
 sub set_members {
-    my ($self, $members) = @_;
+    my ( $self, $members ) = @_;
     $self->{set_members} = [ map { $_->phab_phid } @$members ];
 }
 
 sub set_policy {
-    my ($self, $name, $policy) = @_;
+    my ( $self, $name, $policy ) = @_;
     $self->{set_policy} ||= {};
     $self->{set_policy}->{$name} = $policy;
 }
 
-1;
\ No newline at end of file
+############
+# Builders #
+############
+
+sub _build_members {
+    my ($self) = @_;
+    return [] unless $self->members_raw;
+
+    my @phids;
+    foreach my $member ( @{ $self->members_raw } ) {
+        push( @phids, $member->{phid} );
+    }
+
+    return [] if !@phids;
+
+    my $users = get_phab_bmo_ids( { phids => \@phids } );
+
+    my @members;
+    foreach my $user (@$users) {
+        my $member = Bugzilla::User->new( { id => $user->{id}, cache => 1 } );
+        $member->{phab_phid} = $user->{phid};
+        push( @members, $member );
+    }
+
+    return \@members;
+}
+
+1;
+