[Patchew-devel] [PATCH v2 3/3] Make Import Command use Rest

[Fam Zheng]

On Mon, 07/02 13:40, Shubham Jain wrote:
> Right. So in REST we are giving permissions to import any authenticated
> user but the projects would be only imported to recognised projects. We can
> give warning in our client when there are no messages are imported.

Or maybe it's even better to give warnings when projects recognize the patch but
permission is lacking.

But for the importer running in the background, making sure the exit status is
not 0 when any error happens is the highest priority, this includes any
authentication and permission issues.

Fam

> 
> On Mon, Jul 2, 2018 at 1:34 PM Fam Zheng <famz at redhat.com> wrote:
> 
> > On Mon, 07/02 13:11, Shubham Jain wrote:
> > > On Mon, Jul 2, 2018 at 12:32 PM Fam Zheng <famz at redhat.com> wrote:
> > >
> > > > On Fri, 06/29 19:35, Shubham Jain wrote:
> > > > > Make patchew-cli's Import user REST apis. Changed the exit status of
> > > > authenticated user from cli test while importing since in REST every
> > > > authenticated user can access the import api but messages would be
> > imported
> > > > only to the projects recognised ie whether it imported by maintainer or
> > > > importer. Also added warning if message is not imported to any project.
> > > > > ---
> > > > >  api/rest.py          |  3 ++-
> > > > >  patchew-cli          | 18 +++++++++++++-----
> > > > >  tests/test_import.py |  8 ++++----
> > > > >  3 files changed, 19 insertions(+), 10 deletions(-)
> > > > >
> > > > > diff --git a/api/rest.py b/api/rest.py
> > > > > index 9b47f37..876be75 100644
> > > > > --- a/api/rest.py
> > > > > +++ b/api/rest.py
> > > > > @@ -423,7 +423,8 @@ class MessagesViewSet(BaseMessageViewSet):
> > > > >      serializer_class = MessageSerializer
> > > > >      permission_classes = (permissions.IsAuthenticatedOrReadOnly,)
> > > > >      parser_classes = (JSONParser, MessagePlainTextParser, )
> > > > > -
> > > > > +    authentication_classes = (CsrfExemptSessionAuthentication, )
> > > > > +
> > > > >      def create(self, request, *args, **kwargs):
> > > > >          m = MboxMessage(request.data['mbox'])
> > > > >          projects = [p for p in Project.objects.all() if
> > p.recognizes(m)]
> > > > > diff --git a/patchew-cli b/patchew-cli
> > > > > index f90f3c5..3796caf 100755
> > > > > --- a/patchew-cli
> > > > > +++ b/patchew-cli
> > > > > @@ -220,11 +220,18 @@ class ImportCommand(SubCommand):
> > > > >                      print("[OLD] " + mo["Subject"])
> > > > >                      return
> > > > >              print("[NEW] " + mo["Subject"])
> > > > > -            r = self.api_do("import", mboxes=[mo.as_string()])
> > > > > -            for p in r:
> > > > > -                if p not in projects:
> > > > > -                    projects.add(p)
> > > > > -                    print(p)
> > > > > +            for mbox in [mo.as_string()]:
> > > > > +                r = self.rest_api_do(url_cmd="messages",
> > > > > +                                     request_method='post',
> > > > > +                                     content_type='message/rfc822',
> > > > > +                                     data=mbox)
> > > > > +                projects_list =
> > [x['resource_uri'].split("messages")[0]
> > > > for x in r['results']]
> > > > > +                for p in projects_list:
> > > > > +                    if p not in projects:
> > > > > +                        projects.add(p)
> > > > > +                        print(p)
> > > > > +                if len(projects_list)==0:
> > > > > +                    print("The message was not imported to any
> > project.
> > > > Perhaps you're not logged in as an importer or maintainer")
> > > >
> > > > Let's report error or raise exception instead of only showing a
> > message if
> > > > not
> > > > logged in as importer/maintainer/suerpuser.
> > > >
> > > Raising an exception will return exit status 1, also the condition of
> > > checking is only just checking the length of projects list. What if an
> > > importer is logged in and messages not imported to any project, ie there
> > > are no recognised project. In that case it should return 0 as exit status
> > > right?
> >
> > Yes. It should basically work like this:
> >
> > have permission  |   message format  |     number of     |    result
> >   to import?     |      is valid?    | imported projects |
> > ---------------------------------------------------------------------
> >        N                    X                    X             error
> >        Y                    N                    X             error
> >        Y                    Y                    X             ok
> >
> > (X = don't care)
> >
> > Fam
> >