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")
             if ff:
                 open(ff, "wb").close()
 
@@ -251,6 +258,7 @@ class ImportCommand(SubCommand):
             try:
                 import_one(f)
             except:
+                raise
                 print("Error in importing:", f)
                 traceback.print_exc()
                 r = 1
diff --git a/tests/test_import.py b/tests/test_import.py
index 5693d7e..250f391 100755
--- a/tests/test_import.py
+++ b/tests/test_import.py
@@ -107,21 +107,21 @@ class UnprivilegedImportTest(ImportTest):
 
     test_import_belong_to_multiple_projects = None
 
-    def check_import_should_fail(self):
-        self.cli_import("0001-simple-patch.mbox.gz", 1)
+    def check_import_status(self, exit_status):
+        self.cli_import("0001-simple-patch.mbox.gz", exit_status)
         a, b = self.check_cli(["search"])
         self.assertNotIn('[Qemu-devel] [PATCH] quorum: Only compile when supported',
                          a.splitlines())
 
     def test_anonymous_import(self):
         self.cli_logout()
-        self.check_import_should_fail()
+        self.check_import_status(exit_status=1)
 
     def test_normal_user_import(self):
         self.cli_logout()
         self.create_user("someuser", "somepass")
         self.cli_login("someuser", "somepass")
-        self.check_import_should_fail()
+        self.check_import_status(exit_status=0)
 
     def test_project_update(self):
         p = Project.objects.all()[0]
-- 
2.15.1 (Apple Git-101)

_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel

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.

>              if ff:
>                  open(ff, "wb").close()
>  
> @@ -251,6 +258,7 @@ class ImportCommand(SubCommand):
>              try:
>                  import_one(f)
>              except:
> +                raise

Move the print line before raise and drop the following lines?

>                  print("Error in importing:", f)
>                  traceback.print_exc()
>                  r = 1
> diff --git a/tests/test_import.py b/tests/test_import.py
> index 5693d7e..250f391 100755
> --- a/tests/test_import.py
> +++ b/tests/test_import.py
> @@ -107,21 +107,21 @@ class UnprivilegedImportTest(ImportTest):
>  
>      test_import_belong_to_multiple_projects = None
>  
> -    def check_import_should_fail(self):
> -        self.cli_import("0001-simple-patch.mbox.gz", 1)
> +    def check_import_status(self, exit_status):
> +        self.cli_import("0001-simple-patch.mbox.gz", exit_status)
>          a, b = self.check_cli(["search"])
>          self.assertNotIn('[Qemu-devel] [PATCH] quorum: Only compile when supported',
>                           a.splitlines())
>  
>      def test_anonymous_import(self):
>          self.cli_logout()
> -        self.check_import_should_fail()
> +        self.check_import_status(exit_status=1)
>  
>      def test_normal_user_import(self):
>          self.cli_logout()
>          self.create_user("someuser", "somepass")
>          self.cli_login("someuser", "somepass")
> -        self.check_import_should_fail()
> +        self.check_import_status(exit_status=0)
>  
>      def test_project_update(self):
>          p = Project.objects.all()[0]
> -- 
> 2.15.1 (Apple Git-101)
> 
> _______________________________________________
> Patchew-devel mailing list
> Patchew-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/patchew-devel

_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel

On Mon, Jul 2, 2018 at 12:32 PM Fam Zheng <famz@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?

>
> >              if ff:
> >                  open(ff, "wb").close()
> >
> > @@ -251,6 +258,7 @@ class ImportCommand(SubCommand):
> >              try:
> >                  import_one(f)
> >              except:
> > +                raise
>
> Move the print line before raise and drop the following lines?
>
I think we can remove the raise from here. Since we are already returning 1
here.

>
> >                  print("Error in importing:", f)
> >                  traceback.print_exc()
> >                  r = 1
> > diff --git a/tests/test_import.py b/tests/test_import.py
> > index 5693d7e..250f391 100755
> > --- a/tests/test_import.py
> > +++ b/tests/test_import.py
> > @@ -107,21 +107,21 @@ class UnprivilegedImportTest(ImportTest):
> >
> >      test_import_belong_to_multiple_projects = None
> >
> > -    def check_import_should_fail(self):
> > -        self.cli_import("0001-simple-patch.mbox.gz", 1)
> > +    def check_import_status(self, exit_status):
> > +        self.cli_import("0001-simple-patch.mbox.gz", exit_status)
> >          a, b = self.check_cli(["search"])
> >          self.assertNotIn('[Qemu-devel] [PATCH] quorum: Only compile
> when supported',
> >                           a.splitlines())
> >
> >      def test_anonymous_import(self):
> >          self.cli_logout()
> > -        self.check_import_should_fail()
> > +        self.check_import_status(exit_status=1)
> >
> >      def test_normal_user_import(self):
> >          self.cli_logout()
> >          self.create_user("someuser", "somepass")
> >          self.cli_login("someuser", "somepass")
> > -        self.check_import_should_fail()
> > +        self.check_import_status(exit_status=0)
> >
> >      def test_project_update(self):
> >          p = Project.objects.all()[0]
> > --
> > 2.15.1 (Apple Git-101)
> >
> > _______________________________________________
> > Patchew-devel mailing list
> > Patchew-devel@redhat.com
> > https://www.redhat.com/mailman/listinfo/patchew-devel
>
_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel

On Mon, 07/02 13:11, Shubham Jain wrote:
> On Mon, Jul 2, 2018 at 12:32 PM Fam Zheng <famz@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

_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel

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.

On Mon, Jul 2, 2018 at 1:34 PM Fam Zheng <famz@redhat.com> wrote:

> On Mon, 07/02 13:11, Shubham Jain wrote:
> > On Mon, Jul 2, 2018 at 12:32 PM Fam Zheng <famz@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
>
_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel

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@redhat.com> wrote:
> 
> > On Mon, 07/02 13:11, Shubham Jain wrote:
> > > On Mon, Jul 2, 2018 at 12:32 PM Fam Zheng <famz@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
> >

_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel

On 02/07/2018 10:27, Fam Zheng wrote:
> 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.

An unauthenticated user will always return 1, because POST will fail
with 403.  However, for an authenticated _but unauthorized_ user that
sends /api/v1/messages/, the response will be successful.

In patchew-cli, returning success is probably okay for a generic user,
but indeed Fam is right, it is not okay for an importer that runs in the
background.

Shubham, please modify the patch to return an exit status of 1 when the
project_list is empty, so that the background importer signals the error
promptly.

The API doesn't tell you which projects have not had permission, since
it only returns (in the 201 Created response) the resources that were
created.  However, the common case of "I forgot to put the user in the
importer group" will be caught just fine.  Special users should not be
assigned as maintainers!

Thanks,

Paolo

_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel

On Mon, 07/02 18:39, Paolo Bonzini wrote:
> On 02/07/2018 10:27, Fam Zheng wrote:
> > 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.
> 
> An unauthenticated user will always return 1, because POST will fail
> with 403.  However, for an authenticated _but unauthorized_ user that
> sends /api/v1/messages/, the response will be successful.
> 
> In patchew-cli, returning success is probably okay for a generic user,
> but indeed Fam is right, it is not okay for an importer that runs in the
> background.
> 
> Shubham, please modify the patch to return an exit status of 1 when the
> project_list is empty, so that the background importer signals the error
> promptly.

Do we return empty project_list if the message is already imported before? If so
there should be no error. Similarly if the message format is valid but it
doesn't belong to any known project, it should be nop and no error.

Fam

> 
> The API doesn't tell you which projects have not had permission, since
> it only returns (in the 201 Created response) the resources that were
> created.  However, the common case of "I forgot to put the user in the
> importer group" will be caught just fine.  Special users should not be
> assigned as maintainers!


_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel

On importing a message which is already imported before, the REST would
give error UNIQUE constraint failed: api_message.project_id,
api_message.message_id. The project_list would be empty only in two cases.
First, when there is no known projects and second when the authorised user
don't have the permission.

On Tue, Jul 3, 2018 at 8:20 AM Fam Zheng <famz@redhat.com> wrote:

> On Mon, 07/02 18:39, Paolo Bonzini wrote:
> > On 02/07/2018 10:27, Fam Zheng wrote:
> > > 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.
> >
> > An unauthenticated user will always return 1, because POST will fail
> > with 403.  However, for an authenticated _but unauthorized_ user that
> > sends /api/v1/messages/, the response will be successful.
> >
> > In patchew-cli, returning success is probably okay for a generic user,
> > but indeed Fam is right, it is not okay for an importer that runs in the
> > background.
> >
> > Shubham, please modify the patch to return an exit status of 1 when the
> > project_list is empty, so that the background importer signals the error
> > promptly.
>
> Do we return empty project_list if the message is already imported before?
> If so
> there should be no error. Similarly if the message format is valid but it
> doesn't belong to any known project, it should be nop and no error.
>
> Fam
>
> >
> > The API doesn't tell you which projects have not had permission, since
> > it only returns (in the 201 Created response) the resources that were
> > created.  However, the common case of "I forgot to put the user in the
> > importer group" will be caught just fine.  Special users should not be
> > assigned as maintainers!
>
>
>
_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel

On Tue, 07/03 13:41, Shubham Jain wrote:
> On importing a message which is already imported before, the REST would
> give error UNIQUE constraint failed: api_message.project_id,
> api_message.message_id.

This breaks scripts/patchew-impoter. Upon network error, the importer instance
is restarted and will retry the import. The server may already received the
message but just failed to return to patchew-cli, in this case, an empty
project_list makes more sense than error.

The UNIQUE contraint error should only apply to explicit project list when
import. From a user perspective, if I submit a message and rely on the server to
figure out which projects to add it, I don't think such an error is reasonable,
even if it has been imported before.

Fam

> 
> On Tue, Jul 3, 2018 at 8:20 AM Fam Zheng <famz@redhat.com> wrote:
> 
> > On Mon, 07/02 18:39, Paolo Bonzini wrote:
> > > On 02/07/2018 10:27, Fam Zheng wrote:
> > > > 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.
> > >
> > > An unauthenticated user will always return 1, because POST will fail
> > > with 403.  However, for an authenticated _but unauthorized_ user that
> > > sends /api/v1/messages/, the response will be successful.
> > >
> > > In patchew-cli, returning success is probably okay for a generic user,
> > > but indeed Fam is right, it is not okay for an importer that runs in the
> > > background.
> > >
> > > Shubham, please modify the patch to return an exit status of 1 when the
> > > project_list is empty, so that the background importer signals the error
> > > promptly.
> >
> > Do we return empty project_list if the message is already imported before?
> > If so
> > there should be no error. Similarly if the message format is valid but it
> > doesn't belong to any known project, it should be nop and no error.
> >
> > Fam
> >
> > >
> > > The API doesn't tell you which projects have not had permission, since
> > > it only returns (in the 201 Created response) the resources that were
> > > created.  However, the common case of "I forgot to put the user in the
> > > importer group" will be caught just fine.  Special users should not be
> > > assigned as maintainers!
> >
> >
> >

_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel

On 03/07/2018 10:36, Fam Zheng wrote:
> On Tue, 07/03 13:41, Shubham Jain wrote:
>> On importing a message which is already imported before, the REST would
>> give error UNIQUE constraint failed: api_message.project_id,
>> api_message.message_id.
> 
> This breaks scripts/patchew-impoter. Upon network error, the importer instance
> is restarted and will retry the import. The server may already received the
> message but just failed to return to patchew-cli, in this case, an empty
> project_list makes more sense than error.
> 
> The UNIQUE contraint error should only apply to explicit project list when
> import. From a user perspective, if I submit a message and rely on the server to
> figure out which projects to add it, I don't think such an error is reasonable,
> even if it has been imported before.

Shubham, can you create a github issue for this?

Thanks,

Paolo

_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel