From mathias.de_mare at nokia.com Wed Jan 18 06:45:20 2023 From: mathias.de_mare at nokia.com (=?utf-8?q?Mathias_De_Mare?=) Date: Wed, 18 Jan 2023 07:45:20 +0100 Subject: [PATCH] pullrequests: introduce limit to stop displaying additional changes Message-ID: # HG changeset patch # User Mathias De Mare # Date 1672834977 -3600 # Wed Jan 04 13:22:57 2023 +0100 # Branch stable # Node ID ac278c9c011136b72de43d8dbd742f9cf3dbf020 # Parent b7efb8fdc45fb95ce8ece04e1aed7d965c300bae pullrequests: introduce limit to stop displaying additional changes The previous pull request threw away some of the changesets to keep the total amount more manageable, but this results in an (for the user) unpredictable subset of changesets being shown. To resolve this issue, we instead do not display any additional changes if the amount of additional changes is larger than a user-defined limit. This could be extended further with "too long to be shown - click here to show", but that was quite a bit of additional work and did not cover our use case of not allowing the display at all in case of too many additional changes. diff --git a/kallithea/controllers/pullrequests.py b/kallithea/controllers/pullrequests.py --- a/kallithea/controllers/pullrequests.py +++ b/kallithea/controllers/pullrequests.py @@ -35,6 +35,8 @@ from tg import tmpl_context as c from tg.i18n import ugettext as _ from webob.exc import HTTPBadRequest, HTTPForbidden, HTTPFound, HTTPNotFound +import kallithea + import kallithea.lib.helpers as h from kallithea.controllers import base from kallithea.controllers.changeset import create_cs_pr_comment, delete_cs_pr_comment @@ -494,6 +496,8 @@ class PullrequestsController(base.BaseRe except IndexError: # probably because c.cs_ranges is empty, probably because revisions are missing pass + rev_limit = safe_int(kallithea.CONFIG.get('next_iteration_rev_limit'), 0) + avail_revs = set() avail_show = [] c.cs_branch_name = c.cs_ref_name @@ -563,9 +567,16 @@ class PullrequestsController(base.BaseRe except ChangesetDoesNotExistError: c.update_msg = _('Error: some changesets not found when displaying pull request from %s.') % c.cs_rev - c.avail_revs = avail_revs - c.avail_cs = [org_scm_instance.get_changeset(r) for r in avail_show] - c.avail_jsdata = graph_data(org_scm_instance, avail_show) + if rev_limit and len(avail_revs) > rev_limit: + c.update_msg = _('Additional changesets (%d) are not shown because they exceed the limit (%d).') % (len(avail_revs), rev_limit) + c.avail_revs = [] + c.avail_cs = [] + c.avail_jsdata = None + else: + c.rev_limit_reached = False + c.avail_revs = avail_revs + c.avail_cs = [org_scm_instance.get_changeset(r) for r in avail_show] + c.avail_jsdata = graph_data(org_scm_instance, avail_show) raw_ids = [x.raw_id for x in c.cs_ranges] c.cs_comments = c.cs_repo.get_comments(raw_ids) From mads at kiilerich.com Wed Jan 18 10:17:11 2023 From: mads at kiilerich.com (Mads Kiilerich) Date: Wed, 18 Jan 2023 11:17:11 +0100 Subject: [PATCH] pullrequests: introduce limit to stop displaying additional changes In-Reply-To: References: Message-ID: <8e38bf3d-d5c7-8096-7390-95062fb42708@kiilerich.com> Thank you. Pushed to stable with some minor tweaks and additions. /Mads On 18/01/2023 07:45, Mathias De Mare wrote: > # HG changeset patch > # User Mathias De Mare > # Date 1672834977 -3600 > # Wed Jan 04 13:22:57 2023 +0100 > # Branch stable > # Node ID ac278c9c011136b72de43d8dbd742f9cf3dbf020 > # Parent b7efb8fdc45fb95ce8ece04e1aed7d965c300bae > pullrequests: introduce limit to stop displaying additional changes > > The previous pull request threw away some of the changesets > to keep the total amount more manageable, but this results > in an (for the user) unpredictable subset of changesets being shown. > > To resolve this issue, we instead do not display any additional changes > if the amount of additional changes is larger than a user-defined limit. > > This could be extended further with "too long to be shown - click here > to show", but that was quite a bit of additional work > and did not cover our use case of not allowing the display at all > in case of too many additional changes. > > diff --git a/kallithea/controllers/pullrequests.py b/kallithea/controllers/pullrequests.py > --- a/kallithea/controllers/pullrequests.py > +++ b/kallithea/controllers/pullrequests.py > @@ -35,6 +35,8 @@ from tg import tmpl_context as c > from tg.i18n import ugettext as _ > from webob.exc import HTTPBadRequest, HTTPForbidden, HTTPFound, HTTPNotFound > > +import kallithea > + > import kallithea.lib.helpers as h > from kallithea.controllers import base > from kallithea.controllers.changeset import create_cs_pr_comment, delete_cs_pr_comment > @@ -494,6 +496,8 @@ class PullrequestsController(base.BaseRe > except IndexError: # probably because c.cs_ranges is empty, probably because revisions are missing > pass > > + rev_limit = safe_int(kallithea.CONFIG.get('next_iteration_rev_limit'), 0) > + > avail_revs = set() > avail_show = [] > c.cs_branch_name = c.cs_ref_name > @@ -563,9 +567,16 @@ class PullrequestsController(base.BaseRe > except ChangesetDoesNotExistError: > c.update_msg = _('Error: some changesets not found when displaying pull request from %s.') % c.cs_rev > > - c.avail_revs = avail_revs > - c.avail_cs = [org_scm_instance.get_changeset(r) for r in avail_show] > - c.avail_jsdata = graph_data(org_scm_instance, avail_show) > + if rev_limit and len(avail_revs) > rev_limit: > + c.update_msg = _('Additional changesets (%d) are not shown because they exceed the limit (%d).') % (len(avail_revs), rev_limit) > + c.avail_revs = [] > + c.avail_cs = [] > + c.avail_jsdata = None > + else: > + c.rev_limit_reached = False > + c.avail_revs = avail_revs > + c.avail_cs = [org_scm_instance.get_changeset(r) for r in avail_show] > + c.avail_jsdata = graph_data(org_scm_instance, avail_show) > > raw_ids = [x.raw_id for x in c.cs_ranges] > c.cs_comments = c.cs_repo.get_comments(raw_ids) > > _______________________________________________ > kallithea-general mailing list > kallithea-general at sfconservancy.org > https://lists.sfconservancy.org/mailman/listinfo/kallithea-general From svhb at telenet.be Tue Feb 21 11:03:35 2023 From: svhb at telenet.be (svhb at telenet.be) Date: Tue, 21 Feb 2023 12:03:35 +0100 (CET) Subject: username allowed characters Message-ID: <1100788871.14329366.1676977415664.JavaMail.zimbra@telenet.be> Hello, I'm using LDAP to authenticate users to our system. Out IT-dept hammers on the fact that we need to use the email-address of users to login. The login works ok, but when I want to change the settings for a certain user, it complains about a '@' in the user name.A simple patch during docker build solved this issue. Since email addresses are used regularly for logging in, maybe this can be also in the next version of Kallithea. Simply said : just adding @ to the regexp for username does the job. --- validators.py 2023-02-21 10:25:27.657212999 +0000 +++ validators_new.py 2023-02-21 10:26:40.560218089 +0000 @@ -92,7 +92,7 @@ msg = self.message('username_exists', state, username=value) raise formencode.Invalid(msg, value, state) - if re.match(r'^[a-zA-Z0-9\_]{1}[a-zA-Z0-9\-\_\.]*$', value) is None: + if re.match(r'^[a-zA-Z0-9\_]{1}[a-zA-Z0-9\-\_\.@]*$', value) is None: msg = self.message('invalid_username', state) raise formencode.Invalid(msg, value, state) return _validator Cheers, Stefaan BTW : this is the best package I encountered since bitbucket stopped with Hg. -------------- next part -------------- An HTML attachment was scrubbed... URL: From mads at kiilerich.com Tue Feb 21 14:25:10 2023 From: mads at kiilerich.com (Mads Kiilerich) Date: Tue, 21 Feb 2023 15:25:10 +0100 Subject: username allowed characters In-Reply-To: <1100788871.14329366.1676977415664.JavaMail.zimbra@telenet.be> References: <1100788871.14329366.1676977415664.JavaMail.zimbra@telenet.be> Message-ID: Hi Thank you. But ... Kallithea generally tries to keep logins and emails unambiguous and in different namespaces. It is arguably a bug if Kallithea allows LDAP to use email addresses as usernames. The assumption is probably not entirely enforced, but it and the consequences show up for example in get_by_username_or_email . If allowing @ in usernames, it should perhaps be enforced that it only is allowed if it matches the email ... but that seems like a hack that would be hard to enforce and not really feasible. But it is already generally possible to login with email instead of username. Perhaps that doesn't work with LDAP? Can you set attr_login to point at the email attribute? Or does that have other bad consequences? Something that could be fixed instead? /Mads On 21/02/2023 12:03, svhb at telenet.be wrote: > Hello, > > I'm using LDAP to authenticate users to our system. Out IT-dept > hammers on the fact that we need to use the email-address of users to > login. > > The login works ok, but when I want to change the settings for a > certain user, it complains about a '@' in the user name.A simple patch > during docker build solved this issue. > > Since email addresses are used regularly for logging in, maybe this > can be also in the next version of Kallithea. > > > Simply said : just adding @ to the regexp for username does the job. > > --- validators.py    2023-02-21 10:25:27.657212999 +0000 > +++ validators_new.py    2023-02-21 10:26:40.560218089 +0000 > @@ -92,7 +92,7 @@ >                      msg = self.message('username_exists', state, > username=value) >                      raise formencode.Invalid(msg, value, state) > > -            if re.match(r'^[a-zA-Z0-9\_]{1}[a-zA-Z0-9\-\_\.]*$', > value) is None: > +            if re.match(r'^[a-zA-Z0-9\_]{1}[a-zA-Z0-9\-\_\.@]*$', > value) is None: >                  msg = self.message('invalid_username', state) >                  raise formencode.Invalid(msg, value, state) >      return _validator > > Cheers, > Stefaan > > > BTW : this is the best package I encountered since bitbucket stopped > with Hg. > > > _______________________________________________ > kallithea-general mailing list > kallithea-general at sfconservancy.org > https://lists.sfconservancy.org/mailman/listinfo/kallithea-general -------------- next part -------------- An HTML attachment was scrubbed... URL: From svhb at telenet.be Wed Feb 22 09:34:08 2023 From: svhb at telenet.be (svhb at telenet.be) Date: Wed, 22 Feb 2023 10:34:08 +0100 (CET) Subject: username allowed characters In-Reply-To: References: <1100788871.14329366.1676977415664.JavaMail.zimbra@telenet.be> Message-ID: <1383179661.17628328.1677058448482.JavaMail.zimbra@telenet.be> Hi Mads, I don't have issues with emails and usernames in different namespaces. It is not my intention to use the "username" as email address, lets keep them indeed separate. It is just problematic that the username in my case also looks like an email address (with '@') As it happens, our IT-dept has a hard request to use the email to login in systems, so I need to use the email as "Login Attribute" for LDAPS (the AD attribute is called userPrincipleName but I don't know if that is a standard), no problem here. The "Email Attribute" just gets the same data, so for Kallithea, the mail address is still a different variable, but probably with the same content. To complicate things further, this 'userPrincipleName' can be an email-address on a different domain, than the standard 'Email Attribute' for a specific user, in this case the Email is different than the login. The issue arises when I want to change a setting on a user (for example make someone admin), then the earlier 'valid' username is not valid anymore. Hence, the little hack, just allow a slightly more flexible rule to validate the username. Cheers, Stefaan Van: "Mads Kiilerich" Aan: "vanheesbeke stefaan" , "kallithea-general" Verzonden: Dinsdag 21 februari 2023 15:25:10 Onderwerp: Re: username allowed characters Hi Thank you. But ... Kallithea generally tries to keep logins and emails unambiguous and in different namespaces. It is arguably a bug if Kallithea allows LDAP to use email addresses as usernames. The assumption is probably not entirely enforced, but it and the consequences show up for example in get_by_username_or_email . If allowing @ in usernames, it should perhaps be enforced that it only is allowed if it matches the email ... but that seems like a hack that would be hard to enforce and not really feasible. But it is already generally possible to login with email instead of username. Perhaps that doesn't work with LDAP? Can you set attr_login to point at the email attribute? Or does that have other bad consequences? Something that could be fixed instead? /Mads On 21/02/2023 12:03, [ mailto:svhb at telenet.be | svhb at telenet.be ] wrote: Hello, I'm using LDAP to authenticate users to our system. Out IT-dept hammers on the fact that we need to use the email-address of users to login. The login works ok, but when I want to change the settings for a certain user, it complains about a '@' in the user name.A simple patch during docker build solved this issue. Since email addresses are used regularly for logging in, maybe this can be also in the next version of Kallithea. Simply said : just adding @ to the regexp for username does the job. --- validators.py 2023-02-21 10:25:27.657212999 +0000 +++ validators_new.py 2023-02-21 10:26:40.560218089 +0000 @@ -92,7 +92,7 @@ msg = self.message('username_exists', state, username=value) raise formencode.Invalid(msg, value, state) - if re.match(r'^[a-zA-Z0-9\_]{1}[a-zA-Z0-9\-\_\.]*$', value) is None: + if re.match(r'^[a-zA-Z0-9\_]{1}[a-zA-Z0-9\-\_\.@]*$', value) is None: msg = self.message('invalid_username', state) raise formencode.Invalid(msg, value, state) return _validator Cheers, Stefaan BTW : this is the best package I encountered since bitbucket stopped with Hg. _______________________________________________ kallithea-general mailing list [ mailto:kallithea-general at sfconservancy.org | kallithea-general at sfconservancy.org ] [ https://lists.sfconservancy.org/mailman/listinfo/kallithea-general | https://lists.sfconservancy.org/mailman/listinfo/kallithea-general ] -------------- next part -------------- An HTML attachment was scrubbed... URL: From mads at kiilerich.com Wed Feb 22 11:04:58 2023 From: mads at kiilerich.com (Mads Kiilerich) Date: Wed, 22 Feb 2023 12:04:58 +0100 Subject: username allowed characters In-Reply-To: <1383179661.17628328.1677058448482.JavaMail.zimbra@telenet.be> References: <1100788871.14329366.1676977415664.JavaMail.zimbra@telenet.be> <1383179661.17628328.1677058448482.JavaMail.zimbra@telenet.be> Message-ID: Hi Well ... since you use @ in usernames, I think you *do* have an issue with the way Kallithea assumes emails and usernames are different "namespaces" (or rather "value spaces"). I'm reluctant to allow @ in usernames in general without making sure that it works correctly in all cases, without opening up for new problems. As mentioned, one case that must be considered is when the username of one user is the email of another user. Some invariant would have to be enforced. We need a convincing story of how get_by_username_or_email and all the invocations of it should handle emails as username. And there might be others. It seems like in your setup, both kallithea/controllers/base.py and kallithea/lib/auth_modules/__init__.py will find the corresponding user object with get_by_username_or_email ... which will look up the user by matching on the email field. You say that "the mail address is still a different variable, but probably with the same content" ... but apparently it *must*  have the same content for your setup to work correctly. But then I don't understand what you say about the case where Email is different than the login. Another potential problem that comes to mind is auto completion of usernames, for example in @mention. There might not be a problem there, but it would have to be verified. So while it is great that it works for you with this minor change, I don't think it is a setup that can be recommended or that we would consider "supported". Fixing it properly would imply investing in bigger changes to the fragile and critical authentication system while making sure nothing breaks for other use cases. For upstream Kallithea, I think it would be more feasible to expand ldap auth to allow login with email, but still use a "real" username without @ inside Kallithea ... assuming your ldap has another field that can be used for that purpose. But I don't know if that could work for you and with your IT department requirements. /Mads On 22/02/2023 10:34, svhb at telenet.be wrote: > Hi Mads, > > I don't have issues with emails and usernames in different namespaces. > It is not my intention to use the "username" as email address, lets > keep them indeed separate. It is just problematic that the username in > my case also looks like an email address (with '@') > > As it happens, our IT-dept has a hard request to use the email to > login in systems, so I need to use the email as "Login Attribute" for > LDAPS (the AD attribute is called userPrincipleName but I don't know > if that is a standard), no problem here. The "Email Attribute" just > gets the same data, so for Kallithea, the mail address is still a > different variable, but probably with the same content. > To complicate things further, this 'userPrincipleName' can be an > email-address on a different domain, than the standard 'Email > Attribute' for a specific user, in this case the Email is different > than the login. > > The issue arises when I want to change a setting on a user (for > example make someone admin), then the earlier 'valid' username is not > valid anymore. Hence, the little hack, just allow a slightly more > flexible rule to validate the username. > > Cheers, > > Stefaan > > > > ------------------------------------------------------------------------ > *Van: *"Mads Kiilerich" > *Aan: *"vanheesbeke stefaan" , "kallithea-general" > > *Verzonden: *Dinsdag 21 februari 2023 15:25:10 > *Onderwerp: *Re: username allowed characters > > Hi > > Thank you. > > But ... > > Kallithea generally tries to keep logins and emails unambiguous and in > different namespaces. It is arguably a bug if Kallithea allows LDAP to > use email addresses as usernames. > > The assumption is probably not entirely enforced, but it and the > consequences show up for example in get_by_username_or_email . > > If allowing @ in usernames, it should perhaps be enforced that it only > is allowed if it matches the email ... but that seems like a hack that > would be hard to enforce and not really feasible. > > But it is already generally possible to login with email instead of > username. Perhaps that doesn't work with LDAP? Can you set attr_login > to point at the email attribute? Or does that have other bad > consequences? Something that could be fixed instead? > > /Mads > > > On 21/02/2023 12:03, svhb at telenet.be wrote: > > Hello, > > I'm using LDAP to authenticate users to our system. Out IT-dept > hammers on the fact that we need to use the email-address of users > to login. > > The login works ok, but when I want to change the settings for a > certain user, it complains about a '@' in the user name.A simple > patch during docker build solved this issue. > > Since email addresses are used regularly for logging in, maybe > this can be also in the next version of Kallithea. > > > Simply said : just adding @ to the regexp for username does the job. > > --- validators.py    2023-02-21 10:25:27.657212999 +0000 > +++ validators_new.py    2023-02-21 10:26:40.560218089 +0000 > @@ -92,7 +92,7 @@ >                      msg = self.message('username_exists', state, > username=value) >                      raise formencode.Invalid(msg, value, state) > > -            if re.match(r'^[a-zA-Z0-9\_]{1}[a-zA-Z0-9\-\_\.]*$', > value) is None: > +            if re.match(r'^[a-zA-Z0-9\_]{1}[a-zA-Z0-9\-\_\.@]*$', > value) is None: >                  msg = self.message('invalid_username', state) >                  raise formencode.Invalid(msg, value, state) >      return _validator > > Cheers, > Stefaan > > > BTW : this is the best package I encountered since bitbucket > stopped with Hg. > > > _______________________________________________ > kallithea-general mailing list > kallithea-general at sfconservancy.org > https://lists.sfconservancy.org/mailman/listinfo/kallithea-general > > > -------------- next part -------------- An HTML attachment was scrubbed... URL: From me at manueljacob.de Sat Mar 25 21:10:26 2023 From: me at manueljacob.de (Manuel Jacob) Date: Sat, 25 Mar 2023 22:10:26 +0100 Subject: Transferring changeset approval status to rebased successors Message-ID: <3190ffc5-7825-2342-03d7-41747622ecfe@manueljacob.de> Hi, In one project I’m working on, we do code review of single changesets in a feature branch (usually the changesets are quite small and on average more than 10 are submitted for review at the same time). We also use Mercurial’s changeset evolution quite heavily. Feature branches are rebased regularly and single changesets are amended between two reviews (causing the descendants of these changesets to be rebased by the evolve extension). Currently, we track the review status of each of these changesets manually. After the branch is rebased, each of the rebased changesets is shown as unreviewed in Kallihea. It would be a significant improvement if Kallithea showed for each changeset whether an “unchanged” predecessor was already approved. Thanks to the obsoleteness markers provided by Mercurial, this is easy to determine. The algorithm would walk through the predecessors if there is only one and only the parent changed in between them, until it hits a changeset whose status is not “unreviewed”. One question is how to show this information to the user. What would work for me is to show "approved predecessor" in all places where "approved" can be shown. Instead of a green circle, it could show the outline of a green circle. (The same could be applied to “under review” and “not approved”). Another question is when to run the logic. Running it each time the review status is shown somewhere would work good enough for us. Caching this is not easy. It would need to be invalidated each time a predecessor is added or its review status is changed. Recomputing it each time shouldn’t be a problem in practice because the obsoleteness markers are stored in-memory, the number of considered predecessors is limited (until the algorithm hits a “changed” or already reviewed predecessor) and in most places where the review status is shown, the changeset description is also shown, which has to be read from disk, so walking the predecessors should not contribute much to the total time. What do you think? From me at manueljacob.de Wed Mar 29 10:06:07 2023 From: me at manueljacob.de (=?iso-8859-1?q?Manuel_Jacob?=) Date: Wed, 29 Mar 2023 12:06:07 +0200 Subject: [PATCH 1 of 2] api: fix get_changeset() when incomplete raw_id is passed with with_reviews Message-ID: <2704d3ca4210e14e559b.1680084367@tmp> # HG changeset patch # User Manuel Jacob # Date 1680080700 -7200 # Wed Mar 29 11:05:00 2023 +0200 # Node ID 2704d3ca4210e14e559b2a1f23dacdd988989533 # Parent 7037365a7bc351b81f05c790c6d3417d81d7bd5d # EXP-Topic api-comments api: fix get_changeset() when incomplete raw_id is passed with with_reviews Previously, ChangesetStatusModel was queried with the raw_id passed as an argument to the API function. When the raw_id was incomplete (i.e. shortened), no reviews were found. Using the full raw_id from the changeset instance fixes that. Someone might argue that the caller is supposed to pass a full raw_id to the API function. However, in any case, the return value should not be incomplete without notice. diff --git a/kallithea/controllers/api/api.py b/kallithea/controllers/api/api.py --- a/kallithea/controllers/api/api.py +++ b/kallithea/controllers/api/api.py @@ -2335,7 +2335,7 @@ if with_reviews: reviews = ChangesetStatusModel().get_statuses( - repo.repo_name, raw_id) + repo.repo_name, changeset.raw_id) info["reviews"] = reviews return info From me at manueljacob.de Wed Mar 29 10:06:08 2023 From: me at manueljacob.de (=?iso-8859-1?q?Manuel_Jacob?=) Date: Wed, 29 Mar 2023 12:06:08 +0200 Subject: [PATCH 2 of 2] api: add possibility to optionally return comments from get_changeset() In-Reply-To: <2704d3ca4210e14e559b.1680084367@tmp> References: <2704d3ca4210e14e559b.1680084367@tmp> Message-ID: # HG changeset patch # User Manuel Jacob # Date 1680083947 -7200 # Wed Mar 29 11:59:07 2023 +0200 # Node ID d2166e7ea65797fdccb548cd9cf416c8ed02fc6b # Parent 2704d3ca4210e14e559b2a1f23dacdd988989533 # EXP-Topic api-comments api: add possibility to optionally return comments from get_changeset() diff --git a/kallithea/controllers/api/api.py b/kallithea/controllers/api/api.py --- a/kallithea/controllers/api/api.py +++ b/kallithea/controllers/api/api.py @@ -2323,7 +2323,7 @@ raise JSONRPCError('Repository is empty') # permission check inside - def get_changeset(self, repoid, raw_id, with_reviews=False): + def get_changeset(self, repoid, raw_id, with_reviews=False, with_comments=False, with_inline_comments=False): repo = get_repo_or_error(repoid) if not HasRepoPermissionLevel('read')(repo.repo_name): raise JSONRPCError('Access denied to repo %s' % repo.repo_name) @@ -2338,6 +2338,16 @@ repo.repo_name, changeset.raw_id) info["reviews"] = reviews + if with_comments: + comments = ChangesetCommentsModel().get_comments( + repo.repo_id, changeset.raw_id) + info["comments"] = comments + + if with_inline_comments: + inline_comments = ChangesetCommentsModel().get_inline_comments( + repo.repo_id, changeset.raw_id) + info["inline_comments"] = inline_comments + return info # permission check inside diff --git a/kallithea/tests/api/api_base.py b/kallithea/tests/api/api_base.py --- a/kallithea/tests/api/api_base.py +++ b/kallithea/tests/api/api_base.py @@ -2374,6 +2374,8 @@ result = ext_json.loads(response.body)["result"] assert result["raw_id"] == self.TEST_REVISION assert "reviews" not in result + assert "comments" not in result + assert "inline_comments" not in result def test_api_get_changeset_with_reviews(self): reviewobjs = fixture.review_changeset(self.REPO, self.TEST_REVISION, "approved") @@ -2384,6 +2386,8 @@ result = ext_json.loads(response.body)["result"] assert result["raw_id"] == self.TEST_REVISION assert "reviews" in result + assert "comments" not in result + assert "inline_comments" not in result assert len(result["reviews"]) == 1 review = result["reviews"][0] expected = { @@ -2393,6 +2397,47 @@ } assert review == expected + def test_api_get_changeset_with_comments(self): + commentobj = fixture.add_changeset_comment(self.REPO, self.TEST_REVISION, "example changeset comment") + id_, params = _build_data(self.apikey, 'get_changeset', + repoid=self.REPO, raw_id=self.TEST_REVISION, + with_comments=True) + response = api_call(self, params) + result = ext_json.loads(response.body)["result"] + assert result["raw_id"] == self.TEST_REVISION + assert "reviews" not in result + assert "comments" in result + assert "inline_comments" not in result + comment = result["comments"][-1] + expected = { + 'comment_id': commentobj.comment_id, + 'text': 'example changeset comment', + 'username': 'test_admin', + } + assert comment == expected + + def test_api_get_changeset_with_inline_comments(self): + commentobj = fixture.add_changeset_comment(self.REPO, self.TEST_REVISION, "example inline comment", f_path='vcs/__init__.py', line_no="n3") + id_, params = _build_data(self.apikey, 'get_changeset', + repoid=self.REPO, raw_id=self.TEST_REVISION, + with_inline_comments=True) + response = api_call(self, params) + result = ext_json.loads(response.body)["result"] + assert result["raw_id"] == self.TEST_REVISION + assert "reviews" not in result + assert "comments" not in result + assert "inline_comments" in result + expected = [ + ['vcs/__init__.py', { + 'n3': [{ + 'comment_id': commentobj.comment_id, + 'text': 'example inline comment', + 'username': 'test_admin', + }] + }] + ] + assert result["inline_comments"] == expected + def test_api_get_changeset_that_does_not_exist(self): """ Fetch changeset status for non-existant changeset. revision id is the above git hash used in the test above with the diff --git a/kallithea/tests/fixture.py b/kallithea/tests/fixture.py --- a/kallithea/tests/fixture.py +++ b/kallithea/tests/fixture.py @@ -327,6 +327,11 @@ meta.Session().commit() return csm + def add_changeset_comment(self, repo, revision, text, author=TEST_USER_ADMIN_LOGIN, f_path=None, line_no=None): + comment = ChangesetCommentsModel().create(text, repo, author, revision=revision, f_path=f_path, line_no=line_no, send_email=False) + meta.Session().commit() + return comment + def create_pullrequest(self, testcontroller, repo_name, pr_src_rev, pr_dst_rev, title='title'): org_ref = 'branch:stable:%s' % pr_src_rev other_ref = 'branch:default:%s' % pr_dst_rev From me at manueljacob.de Wed Mar 29 11:01:48 2023 From: me at manueljacob.de (=?iso-8859-1?q?Manuel_Jacob?=) Date: Wed, 29 Mar 2023 13:01:48 +0200 Subject: [PATCH] api: include creation date in comments Message-ID: <44e34dcb73b1d1cf6a75.1680087708@tmp> # HG changeset patch # User Manuel Jacob # Date 1680087692 -7200 # Wed Mar 29 13:01:32 2023 +0200 # Node ID 44e34dcb73b1d1cf6a7527094521ab97968d8396 # Parent d2166e7ea65797fdccb548cd9cf416c8ed02fc6b # EXP-Topic api-comments api: include creation date in comments diff --git a/kallithea/model/db.py b/kallithea/model/db.py --- a/kallithea/model/db.py +++ b/kallithea/model/db.py @@ -1914,6 +1914,7 @@ return dict( comment_id=self.comment_id, username=self.author.username, + created_on=self.created_on.replace(microsecond=0), text=self.text, ) diff --git a/kallithea/tests/api/api_base.py b/kallithea/tests/api/api_base.py --- a/kallithea/tests/api/api_base.py +++ b/kallithea/tests/api/api_base.py @@ -2413,6 +2413,7 @@ 'comment_id': commentobj.comment_id, 'text': 'example changeset comment', 'username': 'test_admin', + 'created_on': commentobj.created_on.replace(microsecond=0).isoformat(), } assert comment == expected @@ -2433,6 +2434,7 @@ 'comment_id': commentobj.comment_id, 'text': 'example inline comment', 'username': 'test_admin', + 'created_on': commentobj.created_on.replace(microsecond=0).isoformat(), }] }] ] @@ -2480,7 +2482,8 @@ "org_ref_parts": ["branch", "stable", self.TEST_PR_SRC], "other_ref_parts": ["branch", "default", self.TEST_PR_DST], "comments": [{"username": base.TEST_USER_ADMIN_LOGIN, "text": "", - "comment_id": pullrequest.comments[0].comment_id}], + "comment_id": pullrequest.comments[0].comment_id, + "created_on": "2000-01-01T00:00:00"}], "owner": base.TEST_USER_ADMIN_LOGIN, "statuses": [{"status": "under_review", "reviewer": base.TEST_USER_ADMIN_LOGIN, "modified_at": "2000-01-01T00:00:00"} for i in range(0, len(self.TEST_PR_REVISIONS))], "title": "get test", From me at manueljacob.de Thu Mar 30 01:37:57 2023 From: me at manueljacob.de (=?iso-8859-1?q?Manuel_Jacob?=) Date: Thu, 30 Mar 2023 03:37:57 +0200 Subject: [PATCH 1 of 2] tests: set Git author and committer name and email settings explicitly Message-ID: <2304f8da62b03f6cfd96.1680140277@tmp> # HG changeset patch # User Manuel Jacob # Date 1680121377 -7200 # Wed Mar 29 22:22:57 2023 +0200 # Node ID 2304f8da62b03f6cfd966b72eec70c38c4d52e37 # Parent 7037365a7bc351b81f05c790c6d3417d81d7bd5d # EXP-Topic tests-git tests: set Git author and committer name and email settings explicitly Passing at least GIT_COMMITTER_NAME and GIT_COMMITTER_EMAIL as environment variables is necessary on my machine, which has the user.useconfigonly config set. The author could be passed via command-line options, but it seems best to pass everything uniformly. diff --git a/kallithea/tests/other/test_vcs_operations.py b/kallithea/tests/other/test_vcs_operations.py --- a/kallithea/tests/other/test_vcs_operations.py +++ b/kallithea/tests/other/test_vcs_operations.py @@ -167,6 +167,23 @@ return tempfile.mkdtemp(dir=base.TESTS_TMP_PATH, prefix=prefix, suffix=suffix) +email = 'me at example.com' +if os.name == 'nt': + name = 'User' +else: + name = 'User ǝɯɐᴎ' + +HG_USER = '%s <%s>' % (name, email) +# If the user.useconfigonly config is set, Git won't try to auto-detect the +# name and email. For this case, we need to pass them as environment variables. +GIT_NAME_AND_EMAIL_ENV_VARS = dict( + GIT_AUTHOR_NAME=name, + GIT_AUTHOR_EMAIL=email, + GIT_COMMITTER_NAME=name, + GIT_COMMITTER_EMAIL=email, +) + + def _add_files(vcs, dest_dir, files_no=3): """ Generate some files, add it to dest_dir repo and push back @@ -179,24 +196,18 @@ open(os.path.join(dest_dir, added_file), 'a').close() Command(dest_dir).execute(vcs, 'add', added_file) - email = 'me at example.com' - if os.name == 'nt': - author_str = 'User <%s>' % email - else: - author_str = 'User ǝɯɐᴎ <%s>' % email for i in range(files_no): cmd = """echo "added_line%s" >> %s""" % (i, added_file) Command(dest_dir).execute(cmd) if vcs == 'hg': cmd = """hg commit -m "committed new %s" -u "%s" "%s" """ % ( - i, author_str, added_file + i, HG_USER, added_file ) elif vcs == 'git': - cmd = """git commit -m "committed new %s" --author "%s" "%s" """ % ( - i, author_str, added_file + cmd = """git commit -m "committed new %s" "%s" """ % ( + i, added_file ) - # git commit needs EMAIL on some machines - Command(dest_dir).execute(cmd, EMAIL=email) + Command(dest_dir).execute(cmd, **GIT_NAME_AND_EMAIL_ENV_VARS) def _add_files_and_push(webserver, vt, dest_dir, clone_url, ignoreReturnCode=False, files_no=3): _add_files(vt.repo_type, dest_dir, files_no=files_no) @@ -618,7 +629,7 @@ # add submodule stdout, stderr = Command(base.TESTS_TMP_PATH).execute('git clone', fork_url, dest_dir) stdout, stderr = Command(dest_dir).execute('git submodule add', clone_url, 'testsubmodule') - stdout, stderr = Command(dest_dir).execute('git commit -am "added testsubmodule pointing to', clone_url, '"', EMAIL=base.TEST_USER_ADMIN_EMAIL) + stdout, stderr = Command(dest_dir).execute('git commit -am "added testsubmodule pointing to', clone_url, '"', **GIT_NAME_AND_EMAIL_ENV_VARS) stdout, stderr = Command(dest_dir).execute('git push', fork_url, 'master') # check for testsubmodule link in files page From me at manueljacob.de Thu Mar 30 01:37:58 2023 From: me at manueljacob.de (=?iso-8859-1?q?Manuel_Jacob?=) Date: Thu, 30 Mar 2023 03:37:58 +0200 Subject: [PATCH 2 of 2] tests: prevent Git system and global configuration from loading In-Reply-To: <2304f8da62b03f6cfd96.1680140277@tmp> References: <2304f8da62b03f6cfd96.1680140277@tmp> Message-ID: <4933600c49883dfc142f.1680140278@tmp> # HG changeset patch # User Manuel Jacob # Date 1680139355 -7200 # Thu Mar 30 03:22:35 2023 +0200 # Node ID 4933600c49883dfc142f97a32dd08f9dd82b0d31 # Parent 2304f8da62b03f6cfd966b72eec70c38c4d52e37 # EXP-Topic tests-git tests: prevent Git system and global configuration from loading This reduces differences between different testing environments. Something similar is already done for Mercurial (in the lines directly above this change). The parent changeset has originally been added to support user.useconfigonly. With this changeset, the original motivation for it becomes obsolete. However, it is still necessary to set the committer name via a environment variable, at least on my machine. diff --git a/kallithea/tests/other/test_vcs_operations.py b/kallithea/tests/other/test_vcs_operations.py --- a/kallithea/tests/other/test_vcs_operations.py +++ b/kallithea/tests/other/test_vcs_operations.py @@ -150,6 +150,8 @@ testenv['LANGUAGE'] = 'en_US:en' testenv['HGPLAIN'] = '' testenv['HGRCPATH'] = '' + testenv['GIT_CONFIG_SYSTEM'] = '' + testenv['GIT_CONFIG_GLOBAL'] = '' testenv.update(environ) p = Popen(command, shell=True, stdout=PIPE, stderr=PIPE, cwd=self.cwd, env=testenv) stdout, stderr = p.communicate() From mads at kiilerich.com Fri Mar 31 18:39:40 2023 From: mads at kiilerich.com (Mads Kiilerich) Date: Fri, 31 Mar 2023 20:39:40 +0200 Subject: [PATCH 1 of 2] tests: set Git author and committer name and email settings explicitly In-Reply-To: <2304f8da62b03f6cfd96.1680140277@tmp> References: <2304f8da62b03f6cfd96.1680140277@tmp> Message-ID: On 30/03/2023 03:37, Manuel Jacob wrote: > # HG changeset patch > # User Manuel Jacob > # Date 1680121377 -7200 > # Wed Mar 29 22:22:57 2023 +0200 > # Node ID 2304f8da62b03f6cfd966b72eec70c38c4d52e37 > # Parent 7037365a7bc351b81f05c790c6d3417d81d7bd5d > # EXP-Topic tests-git > tests: set Git author and committer name and email settings explicitly > > Passing at least GIT_COMMITTER_NAME and GIT_COMMITTER_EMAIL as environment variables is necessary on my machine, which has the user.useconfigonly config set. The author could be passed via command-line options, but it seems best to pass everything uniformly. (We usually wrap commit messages at less than 80 columns.) I agree with the intent here. But EMAIL=email is lost? It is slightly annoying to have to introduce global variables. It would be slightly better to have a _commit_files() function that apply environment variables directly to execute() and could be invoked in both places. But that is not a deal breaker. /Mads > diff --git a/kallithea/tests/other/test_vcs_operations.py b/kallithea/tests/other/test_vcs_operations.py > --- a/kallithea/tests/other/test_vcs_operations.py > +++ b/kallithea/tests/other/test_vcs_operations.py > @@ -167,6 +167,23 @@ > return tempfile.mkdtemp(dir=base.TESTS_TMP_PATH, prefix=prefix, suffix=suffix) > > > +email = 'me at example.com' > +if os.name == 'nt': > + name = 'User' > +else: > + name = 'User ǝɯɐᴎ' > + > +HG_USER = '%s <%s>' % (name, email) > +# If the user.useconfigonly config is set, Git won't try to auto-detect the > +# name and email. For this case, we need to pass them as environment variables. > +GIT_NAME_AND_EMAIL_ENV_VARS = dict( > + GIT_AUTHOR_NAME=name, > + GIT_AUTHOR_EMAIL=email, > + GIT_COMMITTER_NAME=name, > + GIT_COMMITTER_EMAIL=email, > +) > + > + > def _add_files(vcs, dest_dir, files_no=3): > """ > Generate some files, add it to dest_dir repo and push back > @@ -179,24 +196,18 @@ > open(os.path.join(dest_dir, added_file), 'a').close() > Command(dest_dir).execute(vcs, 'add', added_file) > > - email = 'me at example.com' > - if os.name == 'nt': > - author_str = 'User <%s>' % email > - else: > - author_str = 'User ǝɯɐᴎ <%s>' % email > for i in range(files_no): > cmd = """echo "added_line%s" >> %s""" % (i, added_file) > Command(dest_dir).execute(cmd) > if vcs == 'hg': > cmd = """hg commit -m "committed new %s" -u "%s" "%s" """ % ( > - i, author_str, added_file > + i, HG_USER, added_file > ) > elif vcs == 'git': > - cmd = """git commit -m "committed new %s" --author "%s" "%s" """ % ( > - i, author_str, added_file > + cmd = """git commit -m "committed new %s" "%s" """ % ( > + i, added_file > ) > - # git commit needs EMAIL on some machines > - Command(dest_dir).execute(cmd, EMAIL=email) > + Command(dest_dir).execute(cmd, **GIT_NAME_AND_EMAIL_ENV_VARS) > > def _add_files_and_push(webserver, vt, dest_dir, clone_url, ignoreReturnCode=False, files_no=3): > _add_files(vt.repo_type, dest_dir, files_no=files_no) > @@ -618,7 +629,7 @@ > # add submodule > stdout, stderr = Command(base.TESTS_TMP_PATH).execute('git clone', fork_url, dest_dir) > stdout, stderr = Command(dest_dir).execute('git submodule add', clone_url, 'testsubmodule') > - stdout, stderr = Command(dest_dir).execute('git commit -am "added testsubmodule pointing to', clone_url, '"', EMAIL=base.TEST_USER_ADMIN_EMAIL) > + stdout, stderr = Command(dest_dir).execute('git commit -am "added testsubmodule pointing to', clone_url, '"', **GIT_NAME_AND_EMAIL_ENV_VARS) > stdout, stderr = Command(dest_dir).execute('git push', fork_url, 'master') > > # check for testsubmodule link in files page > _______________________________________________ > kallithea-general mailing list > kallithea-general at sfconservancy.org > https://lists.sfconservancy.org/mailman/listinfo/kallithea-general From mads at kiilerich.com Fri Mar 31 18:45:41 2023 From: mads at kiilerich.com (Mads Kiilerich) Date: Fri, 31 Mar 2023 20:45:41 +0200 Subject: [PATCH 2 of 2] tests: prevent Git system and global configuration from loading In-Reply-To: <4933600c49883dfc142f.1680140278@tmp> References: <2304f8da62b03f6cfd96.1680140277@tmp> <4933600c49883dfc142f.1680140278@tmp> Message-ID: <3def5f3f-b50f-1d5e-4385-4abaf7778bc3@kiilerich.com> On 30/03/2023 03:37, Manuel Jacob wrote: > # HG changeset patch > # User Manuel Jacob > # Date 1680139355 -7200 > # Thu Mar 30 03:22:35 2023 +0200 > # Node ID 4933600c49883dfc142f97a32dd08f9dd82b0d31 > # Parent 2304f8da62b03f6cfd966b72eec70c38c4d52e37 > # EXP-Topic tests-git > tests: prevent Git system and global configuration from loading > > This reduces differences between different testing environments. Something similar is already done for Mercurial (in the lines directly above this change). > > The parent changeset has originally been added to support user.useconfigonly. With this changeset, the original motivation for it becomes obsolete. However, it is still necessary to set the committer name via a environment variable, at least on my machine. > > diff --git a/kallithea/tests/other/test_vcs_operations.py b/kallithea/tests/other/test_vcs_operations.py > --- a/kallithea/tests/other/test_vcs_operations.py > +++ b/kallithea/tests/other/test_vcs_operations.py > @@ -150,6 +150,8 @@ > testenv['LANGUAGE'] = 'en_US:en' > testenv['HGPLAIN'] = '' > testenv['HGRCPATH'] = '' > + testenv['GIT_CONFIG_SYSTEM'] = '' > + testenv['GIT_CONFIG_GLOBAL'] = '' Looking at the git man page, I wonder why not using /dev/null? (I don't like how Mercurial put different semantics to undefined and empty environment variables. That used to cause problems on some almost-extinct unices. But it seems to work...) /Mads > testenv.update(environ) > p = Popen(command, shell=True, stdout=PIPE, stderr=PIPE, cwd=self.cwd, env=testenv) > stdout, stderr = p.communicate() > > _______________________________________________ > kallithea-general mailing list > kallithea-general at sfconservancy.org > https://lists.sfconservancy.org/mailman/listinfo/kallithea-general From mads at kiilerich.com Fri Mar 31 19:31:05 2023 From: mads at kiilerich.com (Mads Kiilerich) Date: Fri, 31 Mar 2023 21:31:05 +0200 Subject: [PATCH 2 of 2] api: add possibility to optionally return comments from get_changeset() In-Reply-To: References: <2704d3ca4210e14e559b.1680084367@tmp> Message-ID: Thanks - pushed to stable. /Mads From mads at kiilerich.com Fri Mar 31 19:31:22 2023 From: mads at kiilerich.com (Mads Kiilerich) Date: Fri, 31 Mar 2023 21:31:22 +0200 Subject: [PATCH] api: include creation date in comments In-Reply-To: <44e34dcb73b1d1cf6a75.1680087708@tmp> References: <44e34dcb73b1d1cf6a75.1680087708@tmp> Message-ID: Thanks - pushed to stable. /Mads From mads at kiilerich.com Fri Mar 31 20:53:10 2023 From: mads at kiilerich.com (Mads Kiilerich) Date: Fri, 31 Mar 2023 22:53:10 +0200 Subject: Transferring changeset approval status to rebased successors In-Reply-To: <3190ffc5-7825-2342-03d7-41747622ecfe@manueljacob.de> References: <3190ffc5-7825-2342-03d7-41747622ecfe@manueljacob.de> Message-ID: <5792feaa-12fe-14e6-610c-556e8d3a9fc5@kiilerich.com> Hi Some brief comments to the big questions: c.cs_repo.statuses() is already used for finding status for changeset hashes in bulk. It will perhaps also be able to handle that you pass all ancestor hashes for all pending PR changes. But you will of course have to process the result and pick the most recent approval to be the one that applies. If you don't want to compute that when rendering web pages, it can also be computed in a push hook. (You can probably ignore the possibility of obsoleted changesets changing review status. Only the latest changeset will get new reviews from the web UI, and that will overrule any old result anyway.) I guess you ideally also should verify that the changeset didn't change significantly since the previous approval. Perhaps by looking at the textual diff (without line numbers) and see if it is the same. This seems to only be about one reviewer on each changeset. Great if that works for you. Doing the same for PRs with multiple reviewers with independent review status will be more tricky. /Mads On 25/03/2023 22:10, Manuel Jacob wrote: > Hi, > > In one project I’m working on, we do code review of single changesets > in a feature branch (usually the changesets are quite small and on > average more than 10 are submitted for review at the same time). We > also use Mercurial’s changeset evolution quite heavily. Feature > branches are rebased regularly and single changesets are amended > between two reviews (causing the descendants of these changesets to be > rebased by the evolve extension). > > Currently, we track the review status of each of these changesets > manually. After the branch is rebased, each of the rebased changesets > is shown as unreviewed in Kallihea. It would be a significant > improvement if Kallithea showed for each changeset whether an > “unchanged” predecessor was already approved. > > Thanks to the obsoleteness markers provided by Mercurial, this is easy > to determine. The algorithm would walk through the predecessors if > there is only one and only the parent changed in between them, until > it hits a changeset whose status is not “unreviewed”. > > One question is how to show this information to the user. What would > work for me is to show "approved predecessor" in all places where > "approved" can be shown. Instead of a green circle, it could show the > outline of a green circle. (The same could be applied to “under > review” and “not approved”). > > Another question is when to run the logic. Running it each time the > review status is shown somewhere would work good enough for us. > Caching this is not easy. It would need to be invalidated each time a > predecessor is added or its review status is changed. Recomputing it > each time shouldn’t be a problem in practice because the obsoleteness > markers are stored in-memory, the number of considered predecessors is > limited (until the algorithm hits a “changed” or already reviewed > predecessor) and in most places where the review status is shown, the > changeset description is also shown, which has to be read from disk, > so walking the predecessors should not contribute much to the total time. > > What do you think? > _______________________________________________ > kallithea-general mailing list > kallithea-general at sfconservancy.org > https://lists.sfconservancy.org/mailman/listinfo/kallithea-general