[PATCH] Fetch Transifex image from https://www.transifex.com

[Lukas Fleischer]

Fixes GitLab issue #3.

Signed-off-by: Lukas Fleischer 
---
 README.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/README.md b/README.md
index f7285a5..77f3b13 100644
--- a/README.md
+++ b/README.md
@@ -45,4 +45,4 @@ Translations
 Translations are welcome via our Transifex project at
 https://www.transifex.com/lfleischer/aurweb; see `doc/i18n.txt` for details.
 
-![Transifex](http://www.transifex.net/projects/p/aurweb/chart/image_png)
+![Transifex](https://www.transifex.com/projects/p/aurweb/chart/image_png)
-- 
2.29.2

[PATCH] .gitignore: add test/trash directory*

[Lukas Fleischer]

Signed-off-by: Lukas Fleischer 
---
 .gitignore | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.gitignore b/.gitignore
index 7c9fa60..4d961d1 100644
--- a/.gitignore
+++ b/.gitignore
@@ -7,4 +7,5 @@ aur.git/
 __pycache__/
 *.py[cod]
 test/test-results/
+test/trash directory*
 schema/aur-schema-sqlite.sql
-- 
2.29.2

Re: Highlight orphaned packages in table (Marcus Andersson)

[Lukas Fleischer]

On Tue, 22 Sep 2020 at 09:35:34,  wrote:
> Yes, exactly, changing the font color to red of the word "orphan". I 
> think that the background color can stay the same,  it will be easy 
> enough to differentiate an orphaned package from a maintained one if 
> the text color is different.

Yes, sounds good to me! You can send patches to this mailing list.

Re: Highlight orphaned packages in table

[Lukas Fleischer]

Hi Marcus,

Sorry, this somehow fell through the cracks.

I like the idea in general. Do you have something more specific in mind?
What would the highlighted entry look like? Red text color for the word
"orphan"?

Best,
Lukas

[PATCH v2] pkg_search_page: Limit number of results on package search

[Lukas Fleischer]

From: Morten Linderud 

The current package search query is quite poorly optimized and becomes a
resource hog when the offsets gets large enough. This DoSes the service.

A quick fix is to just ensure we have some limit to the number of hits
we return. The current hardcoding of 2500 is based on the following:

* 250 hits per page max
* 10 pages

We can maybe consider having it lower, but it seems easier to just have
this a multiple of 250 in the first iteration.

Signed-off-by: Morten Linderud 
Signed-off-by: Lukas Fleischer 
---
 web/lib/pkgfuncs.inc.php | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/web/lib/pkgfuncs.inc.php b/web/lib/pkgfuncs.inc.php
index 8c91571..8075800 100644
--- a/web/lib/pkgfuncs.inc.php
+++ b/web/lib/pkgfuncs.inc.php
@@ -619,7 +619,7 @@ function pkg_search_page($params, $show_headers=true, 
$SID="") {
 
/* Sanitize paging variables. */
if (isset($params['O'])) {
-   $params['O'] = max(intval($params['O']), 0);
+   $params['O'] = bound(intval($params['O']), 0, 2500);
} else {
$params['O'] = 0;
}
@@ -771,9 +771,8 @@ function pkg_search_page($params, $show_headers=true, 
$SID="") {
$result_t = $dbh->query($q_total);
if ($result_t) {
$row = $result_t->fetch(PDO::FETCH_NUM);
-   $total = $row[0];
-   }
-   else {
+   $total = min($row[0], 2500);
+   } else {
$total = 0;
}
 
-- 
2.28.0

[PATCH] Deliver emails to Cc in smtplib code path

[Lukas Fleischer]

When using the sendmail() function with smtplib.SMTP or
smtplib.SMTP_SSL, the list of actual recipients for the email (to be
translated to RCPT commands) has to be provided as a parameter.

Update the notification script and add all Cc recipients to that
parameter.

Signed-off-by: Lukas Fleischer 
---
 aurweb/scripts/notify.py | 18 --
 test/t2500-notify.t  | 16 +---
 2 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/aurweb/scripts/notify.py b/aurweb/scripts/notify.py
index 79cf633..b698bd7 100755
--- a/aurweb/scripts/notify.py
+++ b/aurweb/scripts/notify.py
@@ -14,10 +14,6 @@ import aurweb.l10n
 aur_location = aurweb.config.get('options', 'aur_location')
 
 
-def headers_cc(cclist):
-return {'Cc': str.join(', ', cclist)}
-
-
 def headers_msgid(thread_id):
 return {'Message-ID': thread_id}
 
@@ -53,6 +49,9 @@ class Notification:
 def get_headers(self):
 return {}
 
+def get_cc(self):
+return []
+
 def get_body_fmt(self, lang):
 body = ''
 for line in self.get_body(lang).splitlines():
@@ -80,6 +79,8 @@ class Notification:
 msg['From'] = sender
 msg['Reply-to'] = reply_to
 msg['To'] = to
+if self.get_cc():
+msg['Cc'] = str.join(', ', self.get_cc())
 msg['X-AUR-Reason'] = reason
 msg['Date'] = email.utils.formatdate(localtime=True)
 
@@ -116,6 +117,7 @@ class Notification:
 server.login(user, passwd)
 
 server.set_debuglevel(0)
+deliver_to = [to] + self.get_cc()
 server.sendmail(sender, to, msg.as_bytes())
 server.quit()
 
@@ -450,6 +452,9 @@ class RequestOpenNotification(Notification):
 def get_recipients(self):
 return [(self._to, 'en')]
 
+def get_cc(self):
+return self._cc
+
 def get_subject(self, lang):
 return '[PRQ#%d] %s Request for %s' % \
(self._reqid, self._reqtype.title(), self._pkgbase)
@@ -478,7 +483,6 @@ class RequestOpenNotification(Notification):
 # Use a deterministic Message-ID for the first email referencing a
 # request.
 headers = headers_msgid(thread_id)
-headers.update(headers_cc(self._cc))
 return headers
 
 
@@ -509,6 +513,9 @@ class RequestCloseNotification(Notification):
 def get_recipients(self):
 return [(self._to, 'en')]
 
+def get_cc(self):
+return self._cc
+
 def get_subject(self, lang):
 return '[PRQ#%d] %s Request for %s %s' % (self._reqid,
   self._reqtype.title(),
@@ -538,7 +545,6 @@ class RequestCloseNotification(Notification):
 def get_headers(self):
 thread_id = ''
 headers = headers_reply(thread_id)
-headers.update(headers_cc(self._cc))
 return headers
 
 
diff --git a/test/t2500-notify.t b/test/t2500-notify.t
index 5ef64c1..713b31e 100755
--- a/test/t2500-notify.t
+++ b/test/t2500-notify.t
@@ -277,13 +277,18 @@ test_expect_success 'Test subject and body of merge 
notifications.' '
test_cmp actual expected
 '
 
-test_expect_success 'Test subject and body of request open notifications.' '
+test_expect_success 'Test Cc, subject and body of request open notifications.' 
'
cat <<-EOD | sqlite3 aur.db &&
/* Use package request IDs which can be distinguished from other IDs. */
-   INSERT INTO PackageRequests (ID, PackageBaseID, PackageBaseName, 
UsersID, ReqTypeID, Comments, ClosureComment) VALUES (3001, 1001, "foobar", 1, 
1, "This is a request test comment.", "");
+   INSERT INTO PackageRequests (ID, PackageBaseID, PackageBaseName, 
UsersID, ReqTypeID, Comments, ClosureComment) VALUES (3001, 1001, "foobar", 2, 
1, "This is a request test comment.", "");
EOD
>sendmail.out &&
"$NOTIFY" request-open 1 3001 orphan 1001 &&
+   grep ^Cc: sendmail.out >actual &&
+   cat <<-EOD >expected &&
+   Cc: user@localhost, tu@localhost
+   EOD
+   test_cmp actual expected &&
grep ^Subject: sendmail.out >actual &&
cat <<-EOD >expected &&
Subject: [PRQ#3001] Orphan Request for foobar
@@ -324,9 +329,14 @@ test_expect_success 'Test subject and body of request open 
notifications for mer
test_cmp actual expected
 '
 
-test_expect_success 'Test subject and body of request close notifications.' '
+test_expect_success 'Test Cc, subject and body of request close 
notifications.' '
>sendmail.out &&
"$NOTIFY" request-close 1 3001 accepted &&
+   grep ^Cc: sendmail.out >actual &&
+   cat <<-EOD >expected &&
+   Cc: user@localhost, tu@localhost
+   EOD
+   test_cmp actual expected &&
grep ^Subject: sendmail.out >actual &&
cat <<-EOD >expected &&
Subject: [PRQ#3001] Deletion Request for foobar Accepted
-- 
2.28.0

Re: [PATCH 1/4] spawn: expand AUR_CONFIG to the full path

[Lukas Fleischer]

On Thu, 13 Aug 2020 at 10:45:58, Filipe Laíns wrote:
> This allows using a relative path for the config. PHP didn't play well
> with it.
> 
> Signed-off-by: Filipe Laíns 
> ---
>  aurweb/spawn.py | 4 
>  1 file changed, 4 insertions(+)

Merged into pu (with a typo fix for the commit message of patch 3/4).
Thanks!

Re: [PATCH] Redirect to referer after SSO login

[Lukas Fleischer]

On Wed, 29 Jul 2020 at 11:25:44, Frédéric Mangano-Tarumi wrote:
> Introduce a `redirect` query argument to SSO login endpoints so that
> users are redirected to the page they were originally on when they
> clicked the Login link.
> ---
>  aurweb/routers/sso.py | 23 +--
>  web/html/login.php| 18 --
>  2 files changed, 29 insertions(+), 12 deletions(-)

Thanks! I merged the patch into pu but we need to be careful here.

It probably makes sense to have all the code go through a security
review before we enable this feature in production in any case, so I am
not too worried now.

Re: [PATCH] Remove the per-user session limit

[Lukas Fleischer]

On Wed, 29 Jul 2020 at 07:46:10, Frédéric Mangano-Tarumi wrote:
> This feature was originally introduced by
> f961ffd9c7f2d3d51d3e3b060990a4fef9e56c1b as a fix for FS#12898
> .
> 
> As of today, it is broken because of the `q.SessionID IS NULL` condition
> in the WHERE clause, which can\u2019t be true because SessionID is not
> nullable. As a consequence, the session limit was not applied.
> 
> The fact the absence of the session limit hasn\u2019t caused any issue so
> far, and hadn\u2019t even been noticed, suggests the feature is unneeded.
> ---
>  aurweb/routers/sso.py |  2 +-
>  conf/config.defaults  |  1 -
>  web/lib/acctfuncs.inc.php | 17 -
>  3 files changed, 1 insertion(+), 19 deletions(-)

Merged into pu, thanks!

Re: [PATCH 1/3] Update last login information on SSO login

[Lukas Fleischer]

On Tue, 28 Jul 2020 at 10:33:12, Frédéric Mangano-Tarumi wrote:
> ---
>  aurweb/routers/sso.py | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

Merged all three patches in this patch set, thanks!

Re: [PATCH 2/2] Stop redirecting stderr with proc_open

[Lukas Fleischer]

On Mon, 27 Jul 2020 at 08:44:03, Frédéric Mangano-Tarumi wrote:
> Error outputs were piped to a temporary buffer that wasn\u2019t read by
> anyone, making debugging hard because errors were completely silenced.
> By not explicitly redirecting stderr on proc_open, the subprocess
> inherits its parent stderr.
> ---
>  web/lib/acctfuncs.inc.php| 2 --
>  web/lib/pkgbasefuncs.inc.php | 2 --
>  2 files changed, 4 deletions(-)

Good point. Merged, thanks!

Re: [PATCH 1/2] aurweb.spawn: Support stdout redirections to non-tty

[Lukas Fleischer]

On Mon, 27 Jul 2020 at 08:43:48, Frédéric Mangano-Tarumi wrote:
> Only ttys have a terminal size. If we can\u2019t obtain it, we\u2019ll just 
> use 80
> as a sane default.
> ---
>  aurweb/spawn.py | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)

Merged, thanks!

Re: [PATCH v2] Exclude suspended Users from being notified

[Lukas Fleischer]

On Wed, 15 Jul 2020 at 14:45:54, Kevin Morris wrote:
> The existing notify.py script was grabbing entries regardless
> of user suspension. This has been modified to only send notifications
> to unsuspended users.
> 
> This change was written as a solution to
> https://bugs.archlinux.org/task/65554.
> 
> Signed-off-by: Kevin Morris 
> ---
> 
> The issue was in OwnershipEventNotification: `Users.Suspended = 0` was
> included in the bottom-most SQL fetch, which is not a fetch of the
> recipients. That part of the stanza was removed, and `Users.Suspended = 0`
> is only ever used when fetching recipients for notifications.
> 
>  aurweb/scripts/notify.py | 28 ++--
>  1 file changed, 18 insertions(+), 10 deletions(-)

Thanks for identifying the problem and sending an updated patch!
Replaced the previous one in pu with this version.

Re: [PATCH 1/3] SSO: Port IP ban checking

[Lukas Fleischer]

On Mon, 20 Jul 2020 at 10:25:11, Frédéric Mangano-Tarumi wrote:
> ---
>  aurweb/routers/sso.py | 19 +--
>  1 file changed, 17 insertions(+), 2 deletions(-)

Thanks Frédéric! Merged all patches in this series to pu!

Re: [PATCH 1/4] SSO: Explain the rationale behind prompt=login

[Lukas Fleischer]

On Tue, 14 Jul 2020 at 09:34:06, Frédéric Mangano-Tarumi wrote:
> We might reconsider it in the future.
> ---
>  aurweb/routers/sso.py | 7 +++
>  1 file changed, 7 insertions(+)

Queued all patches in this patch series in pu, thanks Frédéric!

Re: [PATCH] Exclude suspended Users from being notified

[Lukas Fleischer]

On Tue, 14 Jul 2020 at 18:25:04, Lukas Fleischer wrote:
> On Mon, 13 Jul 2020 at 10:47:03, Frédéric Mangano-Tarumi wrote:
> > In that last line, `+,` is not a valid Python syntax. The test suite is
> > screaming.
> 
> Thanks for the pointer Frédéric, I amended the patch.

Seems like some tests are still failing, even after fixing the typo.
Kevin, could you please have another look?

Re: [PATCH] Fix typos in CONTRIBUTING.md

[Lukas Fleischer]

On Mon, 13 Jul 2020 at 11:05:37, Frédéric Mangano-Tarumi wrote:
> ---
>  CONTRIBUTING.md | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Merged, thanks!

Re: [PATCH] Exclude suspended Users from being notified

[Lukas Fleischer]

On Mon, 13 Jul 2020 at 10:47:03, Frédéric Mangano-Tarumi wrote:
> In that last line, `+,` is not a valid Python syntax. The test suite is
> screaming.

Thanks for the pointer Frédéric, I amended the patch.

Re: [PATCH v3] Support conjunctive keyword search in RPC interface

[Lukas Fleischer]

On Mon, 06 Jul 2020 at 16:39:59, Kevin Morris wrote:
> Sweet! Thanks for all your time/help with this, Lukas. I'm growing more
> familiar with the codebase, I'll work on reducing the amount of patchsets
> I send through for stuff. Sorry about all of them.

No worries, as long as revisions are clearly marked, such that it's easy
to keep track of what the latest revision is, that's not a problem at
all.

One more tip: You can use `git send-email --annotate` and add a comment
between the "---" separator and the diff stat which eliminates the need
to send a separate email with comments/updates (that message only
appears in the email, not in the applied patch):

> [...]
> > > Signed-off-by: Kevin Morris 
> > > ---

Add text here.

> > >  doc/rpc.txt   |  4 
> > >  web/lib/aurjson.class.php | 33 +++--
> > >  web/lib/pkgfuncs.inc.php  | 36 +++-
> > >  3 files changed, 50 insertions(+), 23 deletions(-)

Re: [PATCH v3] Support conjunctive keyword search in RPC interface

[Lukas Fleischer]

On Sun, 05 Jul 2020 at 21:19:06, Kevin Morris wrote:
> Newly supported API Version 6 modifies `type=search` for _by_ type
> `name-desc`: it now behaves the same as `name-desc` search through the
> https://aur.archlinux.org/packages/ search page.
> 
> Search for packages containing the literal keyword `blah blah` AND `haha`:
> https://aur.archlinux.org/rpc/?v=6=search="blah blah"%20haha
> 
> Search for packages containing the literal keyword `abc 123`:
> https://aur.archlinux.org/rpc/?v=6=search="abc 123"
> 
> The following example searches for packages that contain `blah` AND `abc`:
> https://aur.archlinux.org/rpc/?v=6=search=blah%20abc
> 
> The legacy method still searches for packages that contain `blah abc`:
> https://aur.archlinux.org/rpc/?v=5=search=blah%20abc
> https://aur.archlinux.org/rpc/?v=5=search=blah%20abc
> 
> API Version 6 is currently only considered during a `search` of `name-desc`.
> 
> Note: This change was written as a solution to
> https://bugs.archlinux.org/task/49133.
> 
> PS: + Some spacing issues fixed in comments.
> 
> Signed-off-by: Kevin Morris 
> ---
>  doc/rpc.txt   |  4 
>  web/lib/aurjson.class.php | 33 +++--
>  web/lib/pkgfuncs.inc.php  | 36 +++-
>  3 files changed, 50 insertions(+), 23 deletions(-)

Awesome! Pushed to pu with some minor space changes and code
deduplication. Thanks!

Re: [PATCH] Support conjunctive keyword search in RPC interface

[Lukas Fleischer]

On Sun, 05 Jul 2020 at 01:13:25, Kevin Morris wrote:
> Alright, patch sent; I used `-v1` this time with `git send-email`... I
> can't find documentation for that switch yet. I've tested basic search
> through both paths; the only refactoring that needed to be done was to
> remove the extra "AND (" and ")" from the generic function, and add it
> where we need it in `pkg_search_page`. Then, we can reuse the same
> `construct_keyword_search` in rpc.

Thanks for the revision! I added some comments inline.

It's common to not use any versioning for the first revision of a patch
(e.g. send without -v1) but then start using (-v2, -v3, ...) for
followup revisions. The -v argument is documented in the
git-format-patch(1) man page, if you are using git-format-patch and
git-send-email separately, you need to specify the argument when
formatting the patch (however, in most cases, you can also run `git
format patch` directly).

Re: [PATCH] Support conjunctive keyword search in RPC interface

[Lukas Fleischer]

On Sun, 05 Jul 2020 at 01:10:07, Kevin Morris wrote:
> Newly supported API Version 6 modifies `type=search` functionality; it
> now behaves the same as `name` or `name-desc` search through the
> https://aur.archlinux.org/packages/ search page.
> 
> Search for packages containing the literal keyword `blah blah` AND `haha`:
> https://aur.archlinux.org/rpc/?v=6=search="blah blah"%20haha
> 
> Search for packages containing the literal keyword `abc 123`:
> https://aur.archlinux.org/rpc/?v=6=search="abc 123"
> 
> The following example searches for packages that contain `blah` AND `abc`:
> https://aur.archlinux.org/rpc/?v=6=search=blah%20abc
> 
> The legacy method still searches for packages that contain `blah abc`:
> https://aur.archlinux.org/rpc/?v=5=search=blah%20abc
> https://aur.archlinux.org/rpc/?v=5=search=blah%20abc
> 
> API Version 6 is currently only considered during a `search` of `name` or
> `name-desc`.
> 
> Note: This change was written as a solution to
> https://bugs.archlinux.org/task/49133.
> 
> PS: + Some spacing issues fixed in comments.
> 
> Signed-off-by: Kevin Morris 
> ---
>  doc/rpc.txt   |  4 
>  web/lib/aurjson.class.php | 29 +
>  web/lib/pkgfuncs.inc.php  | 10 +-
>  3 files changed, 30 insertions(+), 13 deletions(-)
> [...]
> @@ -492,13 +492,26 @@ class AurJSON {
> if (strlen($keyword_string) < 2) {
> return $this->json_error('Query arg too 
> small.');
> }
> -   $keyword_string = $this->dbh->quote("%" . 
> addcslashes($keyword_string, '%_') . "%");
>  
> if ($search_by === 'name') {
> -   $where_condition = "(Packages.Name LIKE 
> $keyword_string)";
> +   if ($this->version >= 6) {
> +   $where_condition = 
> construct_keyword_search($this->dbh,
> +   $keyword_string, false);

Do name and name-desc now behave exactly the same (see below)? If so,
this change in behavior should be documented at least. I would have
expected some more refactoring in construct_keyword_search() and one
additional parameter being passed here though.

Should we include pkgfuncs.inc.php from aurjson.class.php now? How does
this currently get imported?

Also, since the code for both name and name-desc are now very similar,
can we refactor and share most of the code instead of copy pasting? To
this end, it might be easier to switch the blocks (i.e. check for API
version first, then check for request type). That would allow us to
reuse the same assignment to $keyword_string as before and possibly the
same construct_keyword_search() invocation too.

> +   } else {
> +   $keyword_string = $this->dbh->quote(
> +   "%" . 
> addcslashes($keyword_string, '%_') . "%");
> +   $where_condition = "(Packages.Name 
> LIKE $keyword_string)";
> +   }
> } else if ($search_by === 'name-desc') {
> -   $where_condition = "(Packages.Name LIKE 
> $keyword_string OR ";
> -   $where_condition .= "Description LIKE 
> $keyword_string)";
> +   if ($this->version >= 6) {
> +   $where_condition = 
> construct_keyword_search($this->dbh,
> +   $keyword_string, true);

See above.

> +   } else {
> +   $keyword_string = $this->dbh->quote(
> +   "%" . 
> addcslashes($keyword_string, '%_') . "%");
> +   $where_condition = "(Packages.Name 
> LIKE $keyword_string ";
> +   $where_condition .= "OR Description 
> LIKE $keyword_string)";
> +   }
> }
> } else if ($search_by === 'maintainer') {
> if (empty($keyword_string)) {
> diff --git a/web/lib/pkgfuncs.inc.php b/web/lib/pkgfuncs.inc.php
> index 8c915711..f6108e5a 100644
> --- a/web/lib/pkgfuncs.inc.php
> +++ b/web/lib/pkgfuncs.inc.php
> @@ -697,7 +697,9 @@ function pkg_search_page($params, $show_headers=true, 
> $SID="") {
> }
> elseif (isset($params["SeB"]) && $params["SeB"] == "k") {
> /* Search by keywords. */
> +   $q_where .= " AND ( ";

Do we need the space at the end?

> $q_where .= construct_keyword_search($dbh, 
> $params['K'], false);
> +   $q_where .= " )";

Same here, do we need the space before the closing parentheses? More
importantly 

Re: [PATCH] Bump RPC API Version 6

[Lukas Fleischer]

On Sat, 04 Jul 2020 at 20:52:09, Kevin Morris wrote:
> Awesome! I do like the sound better of using the same exact search sql for
> both RPC and Web, and so your suggestion seems like a great idea to me.
> I'll start looking at this today!

Sounds great, I am looking forward to a new patch. And sorry for not
thinking this through more carefully earlier!

Re: [PATCH] Bump RPC API Version 6

[Lukas Fleischer]

On Sat, 04 Jul 2020 at 17:14:26, Kevin Morris wrote:
> API Version 6 modifies `type=search` functionality: Split `arg` into
> keywords separated by spaces (quoted keywords are preserved).
> [...]
> Signed-off-by: Kevin Morris 
> ---
>  doc/rpc.txt   |  4 +++
>  web/lib/aurjson.class.php | 60 +--
>  2 files changed, 56 insertions(+), 8 deletions(-)

Thanks for the quick revision. Looks great now, I merged this as

Support conjunctive keyword search in RPC interface

to pu (I think it is better to refer to the new feature in the subject
line and put aside "Bump RPC API Version" for patches that just bump the
API version in case it was forgotten in a previous patch).

Now that this is implemented, I just wanted to suggest implementing the
same functionality for the web interface until I noticed that we already
do and it's even more powerful: we additionally support "OR" and "NOT"
there; see construct_keyword_search() in pkgfuncs.inc.php. Would you be
open to working on another revision of this patch that refactors
construct_keyword_search() a little bit (to make it usable for the RPC
interface) and then uses that for API version 6 instead?

Re: [PATCH] Bump RPC API Version 6

[Lukas Fleischer]

On Thu, 02 Jul 2020 at 23:10:40, Kevin Morris wrote:
> [...]
> Signed-off-by: Kevin Morris 
> ---
>  doc/rpc.txt   |  4 +++
>  web/lib/aurjson.class.php | 66 ++-
>  2 files changed, 62 insertions(+), 8 deletions(-)

Thanks Kevin! A few comments below.

> diff --git a/doc/rpc.txt b/doc/rpc.txt
> index 3148ebea..b0f5c4e1 100644
> @@ -472,6 +472,33 @@ class AurJSON {
> return array('ids' => $id_args, 'names' => $name_args);
> }
>  
> +   /*
> +* Prepare a WHERE statement for each keyword: append $func($keyword)
> +* separated by $delim. Each keyword is sanitized as wildcards before
> +* it's passed to $func.

What does that last part of the comment mean? Doesn't sanitization
happen in $func itself?

> +*
> +* @param $delim Delimiter to use in the middle of two keywords.
> +* @param $keywords Array of keywords to prepare.
> +* @param $func A function that returns a string. This value is 
> concatenated.
> +*
> +* @return A WHERE condition statement of keywords separated by 
> $delim.
> +*/
> +   private function join_where($delim, $keywords, $func) {
> +   // Applied to each item to concatenate our entire statement.
> +   $reduce_func = function($carry, $item) use(&$func) {
> +   array_push($carry, $func($item));
> +   return $carry;
> +   };
> +
> +   // Manual array_reduce with a local lambda.
> +   $acc = array(); // Initial
> +   foreach ($keywords as &$keyword) {
> +   $acc += $reduce_func($acc, $keyword);

I am slightly puzzled by the implementation here; why is there a need to
have array_push() above and also use the += operator? Can't we simply
call $func() directly and use array_push() here?

> +   }
> +
> +   return join(" $delim ", $acc);

It might make sense to just use $delim here and use " AND " in the
caller (I actually skipped this function on my first read and was asking
myself why the caller doesn't put spaces around "AND", that's the
behavior people expect from a join function). Either that or rename the
function slightly; in the latter case it could even make sense to drop
the parameter altogether and hardcode the AND, unless you think the
function could be used for something else in the future.

Re: [PATCH] Exclude suspended Users from being notified

[Lukas Fleischer]

On Fri, 03 Jul 2020 at 21:29:16, Kevin Morris wrote:
> The existing notify.py script was grabbing entries regardless
> of user suspension. This has been modified to only send notifications
> to unsuspended users.
> 
> This change was written as a solution to
> https://bugs.archlinux.org/task/65554.
> 
> Signed-off-by: Kevin Morris 
> ---
>  aurweb/scripts/notify.py | 31 ---
>  1 file changed, 20 insertions(+), 11 deletions(-)

Merged into pu, thanks Kevin! From your emails, I assume that all other
patches in this thread are obsolete? I recommend using the -v option of
git-send-email (e.g. -v2, -v3, ...) to mark revisions of the same patch.

Re: [PATCH] Add search_type url parameter

[Lukas Fleischer]

Hi Kevin,

Thanks for your quick reply and your willingness to improve on your
first submission!

On Thu, 02 Jul 2020 at 20:04:10, Kevin Morris wrote:
> The refraining from #2 is due to me being a bit worried due to it changing
> how the API behaves, didn't think about the complete picture ("arch linux"
> second_keyword). I'll start working on that now.

Yes, we need to bump the API version and keep the old behavior for the
current API version. But I don't see any issues with that.

> 1. Rename search_type values to something more sensible (waiting on this
> for feedback from ML)
> 2. Implement support for quoted strings in the search arg: "arch linux".
> Quoted keywords will be taken literally -- that is, "arch linux" would
> trigger a search for the literal 'arch linux' string.

Would (1) still be relevant if (2) is implemented?

Best,
Lukas

Re: aurweb tests and ci

[Lukas Fleischer]

Hi Kevin,

On Tue, 30 Jun 2020 at 17:57:57, Kevin Morris wrote:
> Working on some things in aurweb at the moment, and I noticed that on
> master tests are failing on my system, and .travis.yml doesn't seem to
> properly setup the environment needed to run tests (sqlalchemy is not
> installed, db tables missing) on travis.
> 
> Just a few questions
> 1. What's the state of testing aurweb currently?
> 2. What are any test barriers to committing code and submitting patches?
> 2a. What are the testing goals?
> 3. Is there a thread on this mailing list about this topic?
> 
> Thank you very much for your time.

Thank you for your interest!

Please use the pu branch from GitLab for development [1]. We're
currently in a migration phase and the repository at git.archlinux.org
is no longer updated.

All patches should pass `make -C test`.

The test suite has evolved over time; there are several threads about
individual aspects of the test suite but no single thread that covers
everything.

Best regards,
Lukas

[1] https://gitlab.archlinux.org/archlinux/aurweb

Re: [PATCH] Add search_type url parameter

[Lukas Fleischer]

Hi Kevin,

Thanks for the patch!

On Tue, 30 Jun 2020 at 19:36:01, Kevin Morris wrote:
> This commit introduces new functionality: When `search_type` is `web`,
> search behavior matches aurweb's HTML search page.
> 
> New parameters: `search_type`
> Valid search_type values: `rpc` (default), `web`
> 
> The `rpc` search type uses the existing legacy search method.
> 
> The `web` search type clontes the web-based aurweb search page's behavior
> as closely as possible (see note at the bottom of this message).
> 
> The following example searches for packages that contain `blah` AND `abc`:
> https://aur.archlinux.org/rpc/?v=5=search_type=web=blah%20abc
> 
> The legacy method still searches for packages that contain 'blah abc':
> https://aur.archlinux.org/rpc/?v=5=search=blah%20abc
> https://aur.archlinux.org/rpc/?v=5=search_type=rpc=blah%20abc

Before having a look at the implementation, is there any reason the two
search types are called "rpc" and "web" (other than "rpc" is how the RPC
interface used to behave and "web" is how the RPC interface used to
behave)? If not, I think we should choose a different naming that is
more intuitive and self-explanatory.

Going a step further, would it be much more work to instead make the
interface more powerful, use conjunctive search by default and perform
exact matches for phrases put in quotes, such as

"arch linux" package

matching everything that contains the strings "arch linux" and
"package"? With that, we wouldn't even have to distinguish different
search types (but we may need to bump the API version).

Best regards,
Lukas

Re: [PATCH 3/3] Open AUR sessions from SSO

[Lukas Fleischer]

On Mon, 08 Jun 2020 at 14:16:49, Frédéric Mangano-Tarumi wrote:
> Only the core functionality is implemented here. See the TODOs.
> ---
>  aurweb/routers/sso.py | 51 +--
>  1 file changed, 49 insertions(+), 2 deletions(-)

Merged all three patches of this patch set into pu, thanks!

Re: [PATCH] aurweb.l10n: Translate without side effects

[Lukas Fleischer]

On Tue, 09 Jun 2020 at 14:25:22, Frédéric Mangano-Tarumi wrote:
> The install method in Python\u2019s gettext API aliases the translator\u2019s
> gettext method to an application-global _(). We don\u2019t use that anywhere,
> and it\u2019s clear from aurweb\u2019s Translator interface that we want to
> translate a piece of text without affecting any global namespace.
> ---
>  aurweb/l10n.py | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Merged into pu. Will review your other patches later this week. Thanks!

Re: [PATCH 4/4] Guide to setting up Keycloak for the SSO

[Lukas Fleischer]

On Thu, 04 Jun 2020 at 16:00:34, Frédéric Mangano-Tarumi wrote:
> ---
>  conf/config.dev |  2 +-
>  doc/SSO | 38 ++

Nit: Can we name this file sso.txt, following the convention in the doc/
directory?

>  2 files changed, 39 insertions(+), 1 deletion(-)
>  create mode 100644 doc/SSO

Looks good otherwise, thanks!

Re: [PATCH 3/4] Crude OpenID Connect client using Authlib

[Lukas Fleischer]

On Thu, 04 Jun 2020 at 16:00:20, Frédéric Mangano-Tarumi wrote:
> Developers can go to /sso/login to get redirected to the SSO. On
> successful login, the ID token is displayed.
> ---
>  .gitlab-ci.yml |  3 ++-
>  TESTING|  3 ++-
>  aurweb/asgi.py | 13 +
>  aurweb/routers/__init__.py |  5 +
>  aurweb/routers/sso.py  | 30 ++
>  aurweb/spawn.py|  3 +++
>  conf/config.defaults   |  8 
>  conf/config.dev|  9 +
>  8 files changed, 72 insertions(+), 2 deletions(-)
>  create mode 100644 aurweb/routers/__init__.py
>  create mode 100644 aurweb/routers/sso.py
> [...]
> diff --git a/conf/config.dev b/conf/config.dev
> index d752f61f..27e981f8 100644
> --- a/conf/config.dev
> +++ b/conf/config.dev
> @@ -20,6 +20,12 @@ aur_location = http://127.0.0.1:8080
>  disable_http_login = 0
>  enable-maintenance = 0
>  
> +; Single sign-on
> +[sso]
> +openid_configuration = 
> http://127.0.0.1:8083/auth/realms/aurweb/.well-known/openid-configuration
> +client_id = aurweb
> +client_secret =
> +
>  [php]
>  ; Address PHP should bind when spawned in development mode by aurweb.spawn.
>  bind_address = 127.0.0.1:8081
> @@ -30,3 +36,6 @@ htmldir = YOUR_AUR_ROOT/web/html
>  [fastapi]
>  ; Address uvicorn should bind when spawned in development mode by 
> aurweb.spawn.
>  bind_address = 127.0.0.1:8082
> +
> +; Passphrase FastAPI uses to sign client-side sessions.
> +session_secret = 
> \u6975\u79d8\u3001\u8a33\u3059\u306a\uff01\u3042\u3001\u9045\u904e\u304e\u305f\u3002

Nit: Since this is not really a secret, can we just use a plain text
value (e.g. "secret") here? I know we had a similar discussion before,
but I don't like the idea of having binary data in text configuration
files since it might cause all sorts of issues with different tools (and
even if that means the tools are bad, it's better to avoid it
altogether).

Re: Tech stack for Python aurweb

[Lukas Fleischer]

On Wed, 03 Jun 2020 at 17:37:00, Baptiste Jonglez wrote:
> Right, I had not understood this would be such a strong design constraint.
> In that case, yes, Django is clearly out of the loop.

Yes, should definitely have been more clear about this in the initial
email, especially since I was asking for comments from non-contributors.

> I understand the advantages of a gradual rollout.  However there's a risk
> that the new code can end up being inconsistent, with possible security
> impacts.  You will have to really double-check any code related to
> authentication, autorization, privilege, permissions, etc.

Good point, we are aware of that and we will be trying to minimize the
amount of duplicate code. For example, logins will always only be
handled in either the PHP code or the Python code, but we need to
validate session cookies in both code bases of course.

> Indeed, I didn't know FastAPI.  It looks like a more fancy/modern Flask,
> which is a good sign.

Thanks again for your comments!

Re: [PATCH v3 2/2] Introduce conf/config.dev for development

[Lukas Fleischer]

On Tue, 02 Jun 2020 at 20:04:02, Frédéric Mangano-Tarumi wrote:
> conf/config.dev\u2019s purpose is to provide a lighter configuration template
> for developers, and split development-specific options off the default
> configuration file.
> ---
>  TESTING  | 11 ++-
>  conf/config.defaults | 10 --
>  conf/config.dev  | 32 
>  3 files changed, 38 insertions(+), 15 deletions(-)
>  create mode 100644 conf/config.dev

Looks good to me. Merged (together with patch 1/2), thanks!

Re: [PATCH v2 1/5] refactor code to comply with flake8 and isort

[Lukas Fleischer]

Nit: We usually capitalize the first word in commit messages.

On Mon, 01 Jun 2020 at 18:35:25, Filipe Laíns wrote:
> Signed-off-by: Filipe Laíns 
> ---
>  aurweb/git/auth.py  |   3 +-
>  aurweb/git/serve.py |  14 +-
>  aurweb/git/update.py|   6 +-
>  aurweb/initdb.py|   7 +-
>  aurweb/l10n.py  |   2 +-
>  aurweb/schema.py|   4 +-
>  aurweb/scripts/aurblup.py   |   3 +-
>  aurweb/scripts/rendercomment.py |   6 +-
>  migrations/env.py   |   9 +-
>  schema/gendummydata.py  | 354 
>  setup.py|   3 +-
>  11 files changed, 210 insertions(+), 201 deletions(-)

Merged into pu, thanks!

Re: [PATCH v2 2/2] Introduce conf/config.dev for development

[Lukas Fleischer]

On Tue, 02 Jun 2020 at 08:41:28, Frédéric Mangano-Tarumi wrote:
> > Saying ${AUR_CONFIG}.defaults here is elegant, short, precise and clear
> > to everybody who is familiar with the code base. However, given that
> > those comments are mainly relevant to new contributors setting up a dev
> > environment, I am not sure whether mentioning $AUR_CONFIG is a good idea
> > here (people might not be aware of what exactly it is used for at this
> > point). How about just saying config.defaults, given that the file is in
> > the same directory? While being less precise, most people should still
> > understand which file is referred to.
> 
> Developers need to set AUR_CONFIG to spawn aurweb. If we introduced it
> earlier like \u201cDefine the path to you configuration:
> export AUR_CONFIG="\u2026"\u201d, I think it should fit.

That's better, even though I still don't believe it's more useful than
just saying, config.defaults here. New users might set things up, then
go back to the config the next day without knowing about $AUR_CONFIG
anymore.

> As you implied though, using anything other than conf/config is madness.
> How about making aurweb.spawn set AUR_CONFIG to, by order of preference:
> 
> 1. $AUR_CONFIG, if already defined,
> 2. $PWD/conf/config, if it exists,
> 3. $parent/conf/config where $parent is the nearest parent directory
>such that the file exists.
> 
> aurweb.spawn is only meant for development, and it spawns test servers,
> so I think it makes sense to use a different default for AUR_CONFIG than
> production.

Sounds reasonable. I would prefer to infer the path from the location of
the script itself (__file__) instead of (2) and (3) though.

> I can replace $AUR_ROOT by YOUR_AUR_ROOT, which is more explicit, and
> remove the comment. In TESTING, we can suggest using the following
> command:
> sed -e "s;YOUR_AUR_ROOT;$PWD;g" conf/config.dev > conf/config

Sounds good.

> With more motivation, we could add support for environment variable
> interpolation in INI\u202ffiles. aurweb.config would know AUR_ROOT as a
> by-product of finding AUR_CONFIG, if we implement that.

Yeah, probably not worthwhile for just this one use case but something
we might want to look into in the future.

I am not sure what "aurweb.config would know AUR_ROOT as a by-product of
finding AUR_CONFIG" means exactly but note that AUR_CONFIG is usually
not contained in the document root in production setups.

> I meant it to make the SQLite block stand out next to MySQL, but it does
> look special compared to other sections.
> 
> In config.defaults, we use newlines only between sections, but that
> doesn\u2019t leave much room for documentation. Interleaving comments with
> variables like I did below is okay only because the section is tiny. If
> the file grew bigger, I wish the norm was \u201cblock of documentation + set
> of correlated variable definitions + new line\u201d. Sections would need a
> special comment to make them visible, like a ruler or a frame.

I would hope that each section would correspond to exactly one set of
correlated variable definitions, so we would not need this. I am aware
that that's currently not the case though.

> > I think we usually don't add modelines or any other editor-specific
> > configuration.
> 
> Ok!

Thanks!

Re: Tech stack for Python aurweb

[Lukas Fleischer]

Thank you for your comments Ricardo and Baptiste.

On Tue, 02 Jun 2020 at 09:07:21, Baptiste Jonglez wrote:
> I generally don't like the kind of comments that go "it would be nicer if
> you do X and Y" from people that won't actually participate, but I still
> feel it's relevant:

One of the reasons for starting this thread was to give non-core
developers the opportunity to step in and make alternative proposals or
raise concerns, so no need to worry about that.

> On 24-05-20, Ricardo Band wrote:
> > I'm a python programmer for a while now and I really like the small
> > size and simplicity of Flask. But let me give you one advice here.
> > If you're dealing with a more complex application Flask tends to become
> > more complex and very hard to manage. All of a sudden you integrate
> > about 10 Flask addons. Some of them are not well maintained. Some
> > droppen support for others etc.
> > You have to manage all those dependencies.
> > 
> > Personally I prefer Django in more complex projects as it comes with
> > everything included. You rarely have to add something to your
> > dependencies.
> 
> I completely agree with this.
> 
> Flask is fantastic for simple projects.  But once you start adding users,
> authentication, an admin panel, command-line scripts, and so on, it
> quickly becomes a mess.  Django is much more structured, which is a big
> advantage for complex projects.

One thing I should have clarified much more in my first email in this
thread is that this is part of a dual stack solution, with most of the
code still being written in PHP and only specific pages being handled by
the new framework. Everything we do (including database access, user and
session management, ...) must be fully compatible with our PHP
implementation, hence we would almost certainly not be able to use any
of the more sophisticated features that these more comprehensive
frameworks provide.

> Regarding databases and migrations, Django is much nicer to work with than
> SQLAlchemy / Flask-SQLAlchemy.  For instance, with Flask-SQLAlchemy I've
> had serious consistency issues between PostgreSQL and Sqlite, something
> that I had never seen with Django.

Same here, everything needs to be compatible with our existing PHP code
base; and we're already using SQLAlchemy for migrations there (even
though this would be not too hard to change at this point if there is a
strong reason for doing so).

> Also, the Flask ecosystem is not exactly bustling with activity.  On the
> positive side, this is because things are remarkably stable and working as
> intended.  On the negative side, it means some amount of bit-rot and
> unmaintained projects, although it's clearly not to the point of complete
> disrepair.

As you may have noticed from some of the patches submitted to the ML, we
decided to use FastAPI over Flask.

Best regards,
Lukas

Re: [PATCH v2 2/2] Introduce conf/config.dev for development

[Lukas Fleischer]

On Mon, 01 Jun 2020 at 12:50:23, Frédéric Mangano-Tarumi wrote:
> conf/config.dev\u2019s purpose is to provide a lighter configuration template
> for developers, and split development-specific options off the default
> configuration file.
> ---
>  TESTING  |  8 +++-
>  conf/config.defaults | 10 --
>  conf/config.dev  | 36 
>  3 files changed, 39 insertions(+), 15 deletions(-)
>  create mode 100644 conf/config.dev

Great, thanks! A few remarks below.

> diff --git a/conf/config.dev b/conf/config.dev
> new file mode 100644
> index ..e9c2112e
> --- /dev/null
> +++ b/conf/config.dev
> @@ -0,0 +1,36 @@
> +; Configuration file for aurweb development.
> +;
> +; Options are implicitly inherited from ${AUR_CONFIG}.defaults, which lists 
> all

Saying ${AUR_CONFIG}.defaults here is elegant, short, precise and clear
to everybody who is familiar with the code base. However, given that
those comments are mainly relevant to new contributors setting up a dev
environment, I am not sure whether mentioning $AUR_CONFIG is a good idea
here (people might not be aware of what exactly it is used for at this
point). How about just saying config.defaults, given that the file is in
the same directory? While being less precise, most people should still
understand which file is referred to.

> +; available options for productions, and their default values. This current 
> file
> +; overrides only options useful for development, and introduces
> +; development-specific options too.
> +;
> +; Paths need to be absolute, so please replace $AUR_ROOT with your project 
> root.

Even though you added a comment is here, I am not sure that using
$AUR_ROOT below is a great idea since it is too easy to skip part of the
comment and then think that $AUR_ROOT is an actual placeholder parsed by
aurweb. I would either go back to putting a sensible default path
everywhere or replicate this comment directly above all lines containing
$AUR_ROOT.

> +
> +[database]
> +

Stray newline?

> +backend = sqlite
> +name = $AUR_ROOT/aurweb.sqlite3
> +
> +; Alternative MySQL configuration
> +;backend = mysql
> +;name = aurweb
> +;user = aur
> +;password = aur
> +
> +[options]
> +aur_location = http://127.0.0.1:8080
> +disable_http_login = 0
> +enable-maintenance = 0
> +
> +[php]
> +; Address PHP should bind when spawned in development mode by aurweb.spawn.
> +bind_address = 127.0.0.1:8081
> +; Directory containing aurweb's PHP code, required by aurweb.spawn.
> +htmldir = $AUR_ROOT/web/html
> +
> +[fastapi]
> +; Address uvicorn should bind when spawned in development mode by 
> aurweb.spawn.
> +bind_address = 127.0.0.1:8082
> +
> +; vim: ft=dosini

I think we usually don't add modelines or any other editor-specific
configuration.

Re: [PATCH 1/3] flake8: add initial config

[Lukas Fleischer]

On Mon, 01 Jun 2020 at 16:51:05, Filipe Laíns wrote:
> Signed-off-by: Filipe Laíns 
> ---
>  aurweb/git/auth.py|   1 -
>  aurweb/git/serve.py   |  14 +-
>  aurweb/git/update.py  |   4 +-
>  aurweb/l10n.py|   2 +-
>  aurweb/schema.py  |   2 +-
>  aurweb/scripts/rendercomment.py   |   3 +-
>  .../versions/f47cad5d6d03_initial_revision.py |   4 +-
>  schema/gendummydata.py| 349 +-
>  setup.cfg |   4 +
>  9 files changed, 195 insertions(+), 188 deletions(-)
>  create mode 100644 setup.cfg

Thanks, much appreciated. Would it be possible to split the
formatting/style changes from the flake8 setup (fix style first, set up
flake8 next)?

> diff --git a/migrations/versions/f47cad5d6d03_initial_revision.py 
> b/migrations/versions/f47cad5d6d03_initial_revision.py
> index 9e99490f..ec5ecb67 100644
> --- a/migrations/versions/f47cad5d6d03_initial_revision.py
> +++ b/migrations/versions/f47cad5d6d03_initial_revision.py
> @@ -1,12 +1,10 @@
>  """initial revision
>  
>  Revision ID: f47cad5d6d03
> -Revises: 
> +Revises:
>  Create Date: 2020-02-23 13:23:32.331396
>  
>  """
> -from alembic import op
> -import sqlalchemy as sa

Do we want to enforce this on generated code?

Re: [PATCH] aurweb.spawn: Integrate FastAPI and nginx

[Lukas Fleischer]

On Sun, 31 May 2020 at 17:20:45, Frédéric Mangano-Tarumi wrote:
> > Even if the dev-only options are indicated by comments, I am not sure
> > whether it's a good idea to mix production and development configuration
> > like this. Can we put them in a separate file or at least a separate
> > section without too much effort?
> 
> aurweb.config is quite trivial so we could easily add mechanisms for
> handling environments or things like that. However, since these values
> are just ignored by production, I don\u2019t think it\u2019s a big deal.

> How about creating conf/config.dev with dev-specific options, and
> options most developers need to change (disable_http_login, etc.). In
> TESTING, we would change 3) to \u201cCopy conf/config.dev to 
> conf/config\u201d.
> The downside is that when we add new required options to config.dev,
> developers need to update their conf/config manually.

With the current design, you already need to copy the config file to
/etc/aurweb/ on changes or use the AUR_CONFIG environment variable to
point to an alternative path (TESTING uses the latter). So maybe create
a development version of the config file as you suggested, add the
dev-only options only there and update the TESTING instructions to use
that file instead of the production config defaults?

Re: [PATCH] aurweb.spawn: Integrate FastAPI and nginx

[Lukas Fleischer]

Thanks a lot for the patch; two minor comments inline in addition to
what Filipe mentioned previously (I am fine with ^C instead of CTRL+C by
the way but don't care much either way).

On Sat, 30 May 2020 at 13:41:07, Frédéric Mangano-Tarumi wrote:
> aurweb.spawn used to launch only PHP\u2019s built-in server. Now it spawns a
> dummy FastAPI application too. Since both stacks spawn their own HTTP
> server, aurweb.spawn also spawns nginx as a reverse proxy to mount them
> under the same base URL, defined by aur_location in the configuration.
> ---
>  TESTING  |  3 +-
>  aurweb/asgi.py   |  8 +
>  aurweb/spawn.py  | 80 +---
>  conf/config.defaults |  7 
>  4 files changed, 85 insertions(+), 13 deletions(-)
>  create mode 100644 aurweb/asgi.py
> [...]
> diff --git a/aurweb/asgi.py b/aurweb/asgi.py
> new file mode 100644
> index ..8d3deedc
> --- /dev/null
> +++ b/aurweb/asgi.py
> @@ -0,0 +1,8 @@
> +from fastapi import FastAPI
> +
> +app = FastAPI()
> +
> +
> +@app.get("/sso/")
> +async def hello():
> +return {"message": "Hello from FastAPI!"}

I know that this is in preparation of the next step, SSO, but can we use
a separate path here, such as /hello/ or /test/? That would cause less
confusion to future readers of this patch (who may not have any context)
and would allow us to decouple the actual SSO patch series from this
patch more easily. For example, that series could start with a patch to
remove this test, and all followup patches would not mention this test
at all.

> diff --git a/conf/config.defaults b/conf/config.defaults
> index 86fe765c..ed495168 100644
> --- a/conf/config.defaults
> +++ b/conf/config.defaults
> @@ -41,9 +41,16 @@ cache = none
>  cache_pkginfo_ttl = 86400
>  memcache_servers = 127.0.0.1:11211
>  
> +[php]
> +; Address PHP should bind when spawned in development mode by aurweb.spawn.
> +bind_address = 127.0.0.1:8081
>  ; Directory containing aurweb's PHP code, required by aurweb.spawn.
>  ;htmldir = /path/to/web/html
>  
> +[fastapi]
> +; Address uvicorn should bind when spawned in development mode by 
> aurweb.spawn.
> +bind_address = 127.0.0.1:8082
> +

Even if the dev-only options are indicated by comments, I am not sure
whether it's a good idea to mix production and development configuration
like this. Can we put them in a separate file or at least a separate
section without too much effort?

Best regards,
Lukas

Re: Tech stack for Python aurweb

[Lukas Fleischer]

On Mon, 18 May 2020 at 18:10:14, Filipe Laíns wrote:
> On Mon, 2020-05-18 at 17:49 -0400, Lukas Fleischer wrote:
> > Everybody involved in these efforts seemed to prefer Flask [1] so far.
> > It's a mature micro framework and has a relatively low barrier of entry.
> 
> I wouldn't be opposed to other framework, as long as it is mature and
> simple (rules out Django), but the obvious choice here seems to be
> Flask.

Just for the record, since the other email was detached from this
thread: Sven made a case for FastAPI here [1]. I think it is an
interesting alternative that we should consider before finalizing our
decision.

[1] https://lists.archlinux.org/pipermail/aur-dev/2020-May/004857.html

aurweb Git repository moved to GitLab

[Lukas Fleischer]

The official aurweb Git repository has been moved to our new GitLab
instance. The new upstream URL is

https://gitlab.archlinux.org/archlinux/aurweb.git

For the time being, we will accept aurweb bug reports and requests via
the mailing list, Flyspray and the GitLab issue tracker. Patches can be
submitted to the mailing list or via GitLab. Features that require a
GitLab account (such as creating tickets on GitLab or filing merge
requests) are currently only available to members of the Arch Linux
staff. I will send another email when this changes.

In the meantime, please feel free to update any references to the old
Git repository.

Best regards,
Lukas

Re: [PATCH] First HTTP functional test of the RPC interface

[Lukas Fleischer]

On Sat, 23 May 2020 at 12:58:32, Frédéric Mangano-Tarumi wrote:
> I submitted this patch when we were considering porting the whole
> PHP\u202fcode base to Python. However, if we instead port the code as we make
> new features, then the new framework would provide testing tools without
> requiring us to prepare and run a dual-stack instance. Functional HTTP
> testing won\u2019t be as useful.
> 
> I suggest we put this patch on the back burner until we gather the will
> manpower to intensively port the code with the intention of keeping its
> behavior intact.

Thanks for the context. I agree with your analysis and dropped the patch
from pu. We can merge it again in the future if we think it makes more
sense.

Re: [PATCH] ci: remove Travis CI

[Lukas Fleischer]

On Sat, 23 May 2020 at 12:54:07, Filipe Laíns wrote:
> We are are moving to Gitlab CI.
> 
> Signed-off-by: Filipe Laíns 
> ---
>  .travis.yml | 23 ---
>  1 file changed, 23 deletions(-)
>  delete mode 100644 .travis.yml

Merged, thanks

Re: [PATCH] First HTTP functional test of the RPC interface

[Lukas Fleischer]

On Sun, 12 Apr 2020 at 12:55:46, Frédéric Mangano-Tarumi wrote:
> Though barely useful in its current state, it shows how to integrate
> pytest with SQLAlchemy and Werkzeug, providing a test framework for the
> potential future Flask port, while actually testing the current PHP
> implementation.
> ---
>  aurweb/test/__init__.py |  0
>  aurweb/test/conftest.py | 51 +
>  aurweb/test/test_rpc.py | 21 +
>  aurweb/test/wsgihttp.py | 38 ++
>  test/README.md  |  3 +++
>  test/rpc.t  |  2 ++
>  6 files changed, 115 insertions(+)
>  create mode 100644 aurweb/test/__init__.py
>  create mode 100644 aurweb/test/conftest.py
>  create mode 100644 aurweb/test/test_rpc.py
>  create mode 100644 aurweb/test/wsgihttp.py
>  create mode 100755 test/rpc.t

This currently seems to break our tests:

configparser.NoSectionError: No section: 'database'

Did not yet investigate further but that's something that likely needs
to be addressed in some way.

Tech stack for Python aurweb

[Lukas Fleischer]

Context: In light of growing demand/requests for modernizing the aurweb
code base and switching the code base to a more popular programming
language, we did some preparatory research and work to allow for writing
new subsystems in Python. Frédéric proposed a lightweight dual stack
implementation that allows us to implement different parts of the
website with different tech stacks.

A full rewrite, as proposed before, can be carried out gradually and
ported subsystems will be usable early. Independent of these efforts, we
suggest that new subsystems will be written in Python and any components
that undergo major changes should be reimplemented in Python if the
extra effort is manageable.

With a dual stack PoC and a first use case (SSO), I would like to
briefly discuss some design decisions before we start implementing.

Everybody involved in these efforts seemed to prefer Flask [1] so far.
It's a mature micro framework and has a relatively low barrier of entry.

Being the default template engine for Flask, Jinja [2] is a natural
choice. PHP and Python versions of base templates will co-exist. Despite
the fact that these base templates are rarely modified and maintaining
two copies will not involve significant extra work, it is beneficial to
deduplicate as much as possible. With this in mind, a language agnostic
engine such as mustache [3] might be an option worth considering. This
is open for discussion.

As briefly discussed before, SQLAlchemy [4] will be used as database
toolkit.

We appreciate any comments and we would be happy to discuss alternative
frameworks/toolkits.

If I forgot any important aspect here, please feel free to chime in too!

[1] https://flask.palletsprojects.com/
[2] https://palletsprojects.com/p/jinja/
[3] https://mustache.github.io/
[4] https://www.sqlalchemy.org/

Re: aurweb testing environment using Docker

[Lukas Fleischer]

On Tue, 24 Mar 2020 at 22:06:53, Lukas Fleischer wrote:
> On Tue, 24 Mar 2020 at 18:47:30, Filipe Laíns wrote:
> > I think we should build on our existent ansible infrastructure and use
> > ansible-bender for this. We already have all the required roles and the
> > aur-dev playbook defined. Using ansible-bender we can generate from a
> > playbook.
> 
> I don't have any experience with that but if it's possible and the end
> product is convenient for the end user, I'd actually prefer that. Less
> maintenance work and we will mirror the production environment pretty
> well for free. Please keep us updated.

Any new insights on using ansible-bender?

Re: [PATCH] Create aurweb.spawn for spawing the test server

[Lukas Fleischer]

On Sun, 19 Apr 2020 at 14:11:02, Frédéric Mangano-Tarumi wrote:
> This program makes it easier for developers to spawn the PHP server
> since it fetches automatically what it needs from the configuration
> file, rather than having the user explicitly pass arguments to the php
> executable.
> 
> When the setup gets more complicated as we introduce Python,
> aurweb.spawn will keep providing the same interface, while under the
> hood it is planned to support running multiple sub-processes.
> 
> Its Python interface provides an way for the test suite to spawn the
> test server when it needs to perform HTTP requests to the test server.
> 
> The current implementation is somewhat weak as it doesn\u2019t detect when a
> child process dies, but this is not supposed to happen often, and it is
> only meant for aurweb developers.
> 
> In the long term, aurweb.spawn will eventually become obsolete, and
> replaced by Docker or Flask\u2019s tools.
> ---
>  TESTING  |   7 +--
>  aurweb/spawn.py  | 107 +++
>  conf/config.defaults |   3 ++
>  3 files changed, 114 insertions(+), 3 deletions(-)
>  create mode 100644 aurweb/spawn.py

Merged into pu, thanks!

Re: Append latest commit link to package update notification

[Lukas Fleischer]

On Thu, 30 Apr 2020 at 15:26:08, Daniel M. Capella wrote:
> > So appending latest commit link "helps" and "encourages" people to
> > check latest update and I don't think in any possible way has a
> > downside except for a little more text to email which I assume it's
> > OK.
> [...]
> IIRC when I had suggested this feature previously, it had gotten a
> thumbs up, just requires someone take the time to write the code.

Agreed. Note that adding a single commit may not be enough since
multiple commits can be pushed at once. We should either include links
to all new commits or add a link to a diff comprising all commits.

Re: [PATCH] First HTTP functional test of the RPC interface

[Lukas Fleischer]

On Wed, 15 Apr 2020 at 17:54:45, Frédéric Mangano-Tarumi wrote:
> When I run test/t2600-rendercomment.t and measure the time, it spends
> almost 8 seconds on setup.sh, then 3 seconds to actually run the tests.
> That\u2019s quite significant.
> 
> $ ./t2600-rendercomment.t | ts -s %.s
> 0.10 Starting setup.sh
> 7.887877 Compeleted setup.sh
> 8.540758 ok 1 - Test comment rendering.
> 9.050442 ok 2 - Test Markdown conversion.
> 9.574958 ok 3 - Test HTML sanitizing.
> 10.098891 ok 4 - Test link conversion.
> 10.617952 ok 5 - Test Git commit linkification.
> 11.156466 ok 6 - Test Flyspray issue linkification.
> 11.657045 ok 7 - Test headings lowering.
> 11.673559 # passed all 7 test(s)
> 11.673628 1..7

Interesting, I get very different results here. Are you using a
traditional HDD? Since setup.sh does a lot of disk I/O, I could imagine
a significant slowdown with physical disks. Patches are always welcome!

Re: [PATCH] First HTTP functional test of the RPC interface

[Lukas Fleischer]

On Wed, 15 Apr 2020 at 08:44:44, Frédéric Mangano-Tarumi wrote:
> Lukas Fleischer [2020-04-15 08:08:08 -0400]
> > Our test suite already uses a separate configuration which is
> > auto-generated in test/setup.sh, so it is already self-contained in that
> > sense; we only need to update aur_location there.
> 
> I\u2019m hesitant about using setup.sh because it does more than we need. It
> aims at being universal but that means it prepares an environment
> suitable for all tests, but run for every test. That\u2019s quite a waste of
> resources. If it\u2019s just for the configuration file, we could make a
> config.test.defaults and make setup.sh use it except for the few
> variables it needs to override.

The new tests are going to require mock data in the database, too,
right? Do you intend to populate the database separately for each test?
Note that many tests require a common basic set of data, such as user
accounts and packages. If we want different initialization procedures
for each test, we should at least have some way of including this common
data set.

The setup script only takes a couple of milliseconds, so having to
execute it multiple times doesn't bother me too much. If you have any
suggestions on how to reduce some of the duplicate steps, feel free to
post them here, though.

> Still, I wonder if a skip is that bad an option. It\u2019s true it\u2019s not 
> as
> explicit as a fail, but a skipped test is a common convention for the
> test suite to indicate it\u2019s missing something, isn\u2019t it?

Yeah, I guess you are right, especially since there probably is an
option to make skipped tests fail the test suite.

Lukas

Re: [PATCH] First HTTP functional test of the RPC interface

[Lukas Fleischer]

On Tue, 14 Apr 2020 at 09:49:06, Frédéric Mangano-Tarumi wrote:
> My bad, you\u2019re right. I had changed it following the TESTING
> instructions. aur_location is clearly the variable to use, but in the
> current state some people will miss it and the test suite would send
> requests to production. One more argument in favor of the self-contained
> test suite.

Our test suite already uses a separate configuration which is
auto-generated in test/setup.sh, so it is already self-contained in that
sense; we only need to update aur_location there.

> > We should still try to keep the tests that require these preparatory
> > steps separate from tests that don't, just in case anybody wants to run
> > the tests without having to install all the additional components.
> 
> Easy enough. We can spawn the server on the first instantiation of the
> test client fixture. We could add some more detection steps to mark the
> tests as skipped if spawning fails because of missing dependencies.

Great! I would probably prefer having a simple way of telling the test
suite to run with or without these tests, though. We should not let the
test suite pass if we intended to run all tests but forgot to install
some dependency. Maybe we can use an environment variable for that?

Lukas

Re: [PATCH] First HTTP functional test of the RPC interface

[Lukas Fleischer]

On Tue, 14 Apr 2020 at 09:26:44, Frédéric Mangano-Tarumi wrote:
> Hi Lukas,
> 
> Lukas Fleischer [2020-04-14 08:54:03 -0400]
> > On Sun, 12 Apr 2020 at 12:55:46, Frédéric Mangano-Tarumi wrote:
> > > +base_uri = aurweb.config.get("options", "aur_location")
> > > +proxy = WsgiHttpProxy(base_uri)
> > > +return werkzeug.test.Client(proxy, Response)
> > 
> > If I understand correctly, this runs tests against our production setup
> > at aur.archlinux.org. Is that what you ultimately plan to do in the
> > transition phase?
> 
> No way! aur_location refers to the development setup address. Its
> default value in our sample configuration is http://localhost:8080,
> which is fine. Note that in my first test I also accessed the database
> directly too, so the environment is not entirely blackboxed.

Could you quickly point me to where that default value is configured?
Both the testing configuration generated by test/setup.sh and the sample
configuration at conf/config.defaults seem to use our production setup.

> > On a related note, the current test suite has been designed to run in an
> > isolated environment without any prior configuration or network
> > connectivity.
> 
> We could spawn PHP and prepare the SQLite database when the test suite
> starts, then clean up everything on exit. If we use Unix sockets
> everywhere, we may be able to avoid using a TCP port too.
> 
> That is only useful during the transition phase, and as long as we don\u2019t
> have Docker. If you believe we should invest time in auto-spawning, I
> can submit a follow-up patch. Beside self-containment, the test suite
> will have full control over its data set, so it actually sounds like a
> good thing to do even in that temporary phase. It shouldn\u2019t be too hard
> anyway. What do you think?

If it's easy to do that without a huge overhead, I am all for it.

We should still try to keep the tests that require these preparatory
steps separate from tests that don't, just in case anybody wants to run
the tests without having to install all the additional components.

Lukas

Re: [PATCH] First HTTP functional test of the RPC interface

[Lukas Fleischer]

Hi Frédéric,

Thanks for the work! My first thoughts below.

On Sun, 12 Apr 2020 at 12:55:46, Frédéric Mangano-Tarumi wrote:
> Though barely useful in its current state, it shows how to integrate
> pytest with SQLAlchemy and Werkzeug, providing a test framework for the
> potential future Flask port, while actually testing the current PHP
> implementation.
> ---
>  aurweb/test/__init__.py |  0
>  aurweb/test/conftest.py | 51 +
>  aurweb/test/test_rpc.py | 21 +
>  aurweb/test/wsgihttp.py | 38 ++
>  test/README.md  |  3 +++
>  test/rpc.t  |  2 ++
>  6 files changed, 115 insertions(+)
>  create mode 100644 aurweb/test/__init__.py
>  create mode 100644 aurweb/test/conftest.py
>  create mode 100644 aurweb/test/test_rpc.py
>  create mode 100644 aurweb/test/wsgihttp.py
>  create mode 100755 test/rpc.t
> [...]
> +@pytest.fixture
> +def client():
> +"""
> +Build a Werkzeug test client for making HTTP requests to AUR. It requires
> +that the AUR test website is already running at `[options] aur_location`,
> +specified in the configuration file.
> +
> +When aurweb becomes a pure Flask application, this should return 
> Flask\u2019s
> +test_client(), which is a Werkzeug test client too.
> +https://flask.palletsprojects.com/en/1.1.x/testing/#the-testing-skeleton
> +"""
> +base_uri = aurweb.config.get("options", "aur_location")
> +proxy = WsgiHttpProxy(base_uri)
> +return werkzeug.test.Client(proxy, Response)

If I understand correctly, this runs tests against our production setup
at aur.archlinux.org. Is that what you ultimately plan to do in the
transition phase?

On a related note, the current test suite has been designed to run in an
isolated environment without any prior configuration or network
connectivity. Do we want to break either of these assumptions? Breaking
the latter might be okay, but I would like to keep the test suite
self-contained. If that turns out to be too hard, I'd be fine with
splitting the tests into two parts (a self-contained part and another
part that requires a local aurweb setup). We also need to think about
how to integrate all that with CI.

Lukas

[PATCH 2/2] Fix PHP notices in the account form

[Lukas Fleischer]

Signed-off-by: Lukas Fleischer 
---
 web/html/account.php | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/web/html/account.php b/web/html/account.php
index c05d136..d70f4ce 100644
--- a/web/html/account.php
+++ b/web/html/account.php
@@ -25,7 +25,7 @@ if ($action == "UpdateAccount") {
$update_account_message = '';
/* Details for account being updated */
/* Verify user permissions and that the request is a valid POST */
-   if (can_edit_account($row) && check_token()) {
+   if ($row && can_edit_account($row) && check_token()) {
/* Update the details for the existing account */
list($success, $update_account_message) = process_account_form(
"edit", "UpdateAccount",
@@ -55,7 +55,7 @@ if ($action == "UpdateAccount") {
}
 }
 
-if ($action == "AccountInfo") {
+if ($row && $action == "AccountInfo") {
html_header(__('Account') . ' ' . $row['Username']);
 } else {
html_header(__('Accounts'));
@@ -122,7 +122,7 @@ if (isset($_COOKIE["AURSID"])) {
 
} elseif ($action == "DeleteAccount") {
/* Details for account being deleted. */
-   if (can_edit_account($row)) {
+   if ($row && can_edit_account($row)) {
$uid_removal = $row['ID'];
$uid_session = uid_from_sid($_COOKIE['AURSID']);
$username = $row['Username'];
@@ -155,7 +155,7 @@ if (isset($_COOKIE["AURSID"])) {
} elseif ($action == "UpdateAccount") {
print $update_account_message;
 
-   if (!$success) {
+   if ($row && !$success) {
display_account_form("UpdateAccount",
in_request("U"),
in_request("T"),
@@ -181,7 +181,7 @@ if (isset($_COOKIE["AURSID"])) {
}
 
} elseif ($action == "ListComments") {
-   if (has_credential(CRED_ACCOUNT_LIST_COMMENTS, 
array($row["ID"]))) {
+   if ($row && has_credential(CRED_ACCOUNT_LIST_COMMENTS, 
array($row["ID"]))) {
# display the comment list if they're a TU/dev
 
$total_comment_count = 
account_comments_count($row["ID"]);
-- 
2.26.0

[PATCH 1/2] Fix invalid session ID check

[Lukas Fleischer]

Signed-off-by: Lukas Fleischer 
---
 web/lib/aur.inc.php | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/web/lib/aur.inc.php b/web/lib/aur.inc.php
index dbcc23a..f4ad6b4 100644
--- a/web/lib/aur.inc.php
+++ b/web/lib/aur.inc.php
@@ -50,7 +50,7 @@ function check_sid() {
$result = $dbh->query($q);
$row = $result->fetch(PDO::FETCH_NUM);
 
-   if (!$row[0]) {
+   if (!$row) {
# Invalid SessionID - hacker alert!
#
$failed = 1;
-- 
2.26.0

aurweb 5.0.0 released

[Lukas Fleischer]

Dear aurweb contributors and users,

aurweb 5.0.0 has just been released!

You can now add a secondary email address that can be used to recover
your account in case access to the primary email address is lost. Reset
keys for an account are always sent to both the primary and the backup
email address. Most other changes were internal.

Shout-out to Frédéric Mangano-Tarumi for doing some amazing work on the
backend!

For a comprehensive list of changes, please consult the Git log [2]. As
usual, bugs should be reported to the aurweb bug tracker [3].

[1] https://aur.archlinux.org/
[2] https://projects.archlinux.org/aurweb.git/log/?id=v5.0.0
[3] https://bugs.archlinux.org/index.php?project=2

[PATCH v2] Add new upgrade instructions

[Lukas Fleischer]

Signed-off-by: Lukas Fleischer 
---
Add missing raw SQL upgrade instructions.

 upgrading/{4.9.0.txt => 5.x.x.txt} | 8 
 1 file changed, 8 insertions(+)
 rename upgrading/{4.9.0.txt => 5.x.x.txt} (54%)

diff --git a/upgrading/4.9.0.txt b/upgrading/5.x.x.txt
similarity index 54%
rename from upgrading/4.9.0.txt
rename to upgrading/5.x.x.txt
index 241f24a..94f91c6 100644
--- a/upgrading/4.9.0.txt
+++ b/upgrading/5.x.x.txt
@@ -1,3 +1,11 @@
+Starting from release 5.0.0, Alembic is used for managing database migrations.
+
+Run `alembic upgrade head` from the aurweb root directory to upgrade your
+database after upgrading the source code to a new release.
+
+When upgrading from 4.8.0, you also need to execute the following manual SQL
+statements before doing so.
+
 1. Add new columns to store the timestamp and UID when closing requests:
 
 
-- 
2.26.0

[PATCH] Add new upgrade instructions

[Lukas Fleischer]

Signed-off-by: Lukas Fleischer 
---
 upgrading/5.x.x.txt | 4 
 1 file changed, 4 insertions(+)
 create mode 100644 upgrading/5.x.x.txt

diff --git a/upgrading/5.x.x.txt b/upgrading/5.x.x.txt
new file mode 100644
index 000..ba74e28
--- /dev/null
+++ b/upgrading/5.x.x.txt
@@ -0,0 +1,4 @@
+Starting from release 5.0.0, Alembic is used for managing database migrations.
+
+Run `alembic upgrade head` from the aurweb root directory to upgrade your
+database after upgrading the source code to a new release.
-- 
2.26.0

Re: aurweb testing environment using Docker

[Lukas Fleischer]

On Tue, 24 Mar 2020 at 18:47:30, Filipe Laíns wrote:
> I think we should build on our existent ansible infrastructure and use
> ansible-bender for this. We already have all the required roles and the
> aur-dev playbook defined. Using ansible-bender we can generate from a
> playbook.

I don't have any experience with that but if it's possible and the end
product is convenient for the end user, I'd actually prefer that. Less
maintenance work and we will mirror the production environment pretty
well for free. Please keep us updated.

Lukas

Re: aurweb testing environment using Docker

[Lukas Fleischer]

Thank you for all your comments.

On Tue, 24 Mar 2020 at 15:06:49, Yaron Shahrabani wrote:
> The big catch here is this:
> Adding more complexity to the main docker requires more layers, thus making
> the docker building process slower and each and every modification of a
> building block requires starting building all the layers from the top
> (that's how docker works).

I think "big catch" is a bit of an overstatement here. Changes to the
Dockerfile will be pretty rare.

> Separating containers assures that if there's a problem with a certain
> service you'll still be able to run the container and understand what sent
> wrong plus why would the user care about actually installing MySQL during
> the building process and maintaining it while there's a container that you
> shouldn't care about and you would probably never have to consider
> whatsoever.

This is an argument that is mainly relevant in a production environment.
If anything goes wrong with the containers in any way, most first-time
contributors will likely not want to start debugging at all.

I agree that being able to use base images for popular services such as
MariaDB is a bonus for the multiple containers approach.

> The whole concept of docker is outsourcing the bulk resources while keeping
> small units that preform a single task in a tidy manner.

Yes, using a single container is not how Docker is often used but that
doesn't automatically mean it's bad.

> I would even suggest finding a way to separate the ssh process from the web
> UI for that matter.

We should definitely do that if we follow the "multiple containers"
approach. It shouldn't be difficult.

We'd have to share the Git repositories in addition to database access,
though. Not that it's something that is complicated but it shows that
multiple containers introduce additional complexity on the development
side, too.

> Regarding debug: it's actually easier to debug a single task containers
> rather than big ones because you usually use one log feed (docker logs).
> 
> I'm willing to help no matter what your choice will be but translating a
> monolithic system into a single container is not as convenient as it sounds
> (I've worked with docker containers in both dev and production for the past
> 3 years).

It's not complicated either. I do have a draft locally. But let's agree
on a design before moving forward.

Lukas

aurweb testing environment using Docker

[Lukas Fleischer]

We are currently working on a testing environment using Docker.

The idea is to have something that is super easy to set up and still
mirrors our production environment as closely as possible. It should
ideally allow future contributors to test their patch submissions
without having to go through the hassle of manually setting up a full
development environment.

Shout-out to Yaron (Cc) for sending me an initial draft.

One of the big design decisions is whether everything should be in a
single Docker container vs. several smaller atomic containers (i.e.
separate containers for the HTTP server, the database, the SSH/Git
interface, ...), as often done in production environments.

The first approach has the benefit of simplicity from a user's
perspective and being able to easily set up the environment using just
Docker.

The second approach has the benefit of atomicity (simplicity from a
developer's perspective) and is closer to what we would do if we would
actually use Docker for our production environment. It would probably
require docker-compose (or another tool) in addition to Docker to
provide the same level of convenience we can achieve with the other
approach using just Docker, though.

Since Yaron and me have conflicting opinions, I would like to hear some
more arguments and views.

Thanks!
Lukas

Re: TAP testing in Python

[Lukas Fleischer]

On Sun, 22 Mar 2020 at 21:03:31, Frédéric Mangano-Tarumi wrote:
> I didn\u2019t realize it at first, but since pytest-tap outputs TAP already,
> a very simple shell wrapper invoking pytest with the right arguments
> should be enough to integrate it with the existing test structure.
> 

Great!

> One friction point with pytest-tap is that it\u2019s not in the official Arch
> repositories, unlike all the other libraries aurweb depends on. Most
> Python developers should be familiar with pip and virtualenv though, but
> that\u2019s inconsistent with the instructions in TESTING that suggest using
> pacman to install Python dependencies.

I will work on a package and add it to the repositories tomorrow.

Best regards,
Lukas

Re: TAP testing in Python

[Lukas Fleischer]

On Sat, 21 Mar 2020 at 12:29:41, Filipe Laíns wrote:
> Why should we use unittest over pytest? pytest is easier to read and
> write, more extensible/has more integrations and has a larger userbase.
> The only caveat is that people would have to fo a quick read on
> fixtures before they start contributing.
> 
> Choosing pycotap over tappy/pytest-tap would make sense if we already
> had a codebase written in unittest, but we don't.
> 
> Lukas, what do you think?

I have a slight personal preference towards pytest. However, the much
more important questions to ask are:

1. How easy/convenient will it be to write tests for aurweb using the
   framework? What about maintainability?

2. How easy/straightforward will it be for future contributors to write
   tests using the framework?

3. How well does the framework integrate with the current test suite?

Having a small script that can be fully included in our source tree is a
nice bonus but shouldn't be one of the main criteria.

How hard would it be to make pytest-tap compatible with the usual TAP
test invocation?

Lukas

Re: [PATCH] Map BIGINT to INTEGER for SQLite

[Lukas Fleischer]

On Sat, 21 Mar 2020 at 14:13:45, Frédéric Mangano-Tarumi wrote:
> ---
>  aurweb/schema.py | 11 +++
>  1 file changed, 11 insertions(+)

Merged, thanks!

Re: [PATCH] Proof-of-concept of a dual PHP–Python stack

[Lukas Fleischer]

On Sun, 15 Mar 2020 at 09:16:52, Frédéric Mangano-Tarumi wrote:
> Lukas Fleischer [2020-03-15 08:25:31 -0400]
> > Being overloaded is a relative term. Yes, the AUR servers are often
> > under heavy load, with millions of requests every day.
> 
> To get more concrete: are the AUR servers sometimes at 100% CPU
> capacity, or do they hardly ever reach the point of saturation? In other
> words, can we afford a 1% slowdown? What about 10%?

We are in the process of getting a new machine for the AUR and will be
scaling up whenever we are reaching resource limits. So, practically, we
will never reach full utilization unless we're running out of money.
However, it might be better to think of optimizations as tradeoffs
between total time invested and the optimization benefits (reduction in
running expenses, knowledge acquisition of the person implementing the
change, ...)

As I mentioned before, I am fine with trying your fallback approach
first and possibly optimizing later.

Your explanation of the performance hit being relatively small sounds
reasonable but a few simple experiments would be even better.

> > It might make sense to only switch over once the port has been
> > completed.
> 
> I strongly recommend against that.
> 
> First, not deploying the Python backend implies we keep developing the
> PHP stack too, which in turn means we either need to stop developing
> new features, or develop them twice.

It also depends on how long the port will take. We should certainly
prioritize the port and try to defer any feature additions for the time
being.

> Second, deploying a wholly different codebase at once is dreadful for an
> actively used website. All the bugs introduced by the rewrite would pop
> up simultaneously. This is all the more risky if we decide to adjust
> features as we rewrite them. Debugging may also become harder if we
> can\u2019t narrow down the commits based on the date the bug appeared. By the
> way, I think we should for that reason accelerate the release cycle when
> we start porting code.

That's a good point. I agree that introducing the rewrite gradually is a
good idea.

Re: Functional HTTP testing

[Lukas Fleischer]

On Sat, 14 Mar 2020 at 10:58:54, Frédéric Mangano-Tarumi wrote:
> With the current plans of porting the PHP codebase to Python, we should
> consider setting up a suite of functional tests to squash most bugs
> introduced by the rewrite.
> [...]
> HTTP testing being a kind of language-agnostic blackbox testing, the
> tests would also resist any future rewrite or major refactoring without
> requiring maintenance.
> [...]
> I think it\u2019s safe to start with Requests, and consider Mechanize later
> if need be, or even Selenium. Mechanize and Selenium kind of overlap
> with each other, so we should pick only one of the two, but Requests is
> well-known and should never be unwelcome.

Thanks for all the work. I agree that functional tests are important and
your suggestions sound good to me.

We should also roughly define what we expect from a rewrite. We probably
don't want to require all HTML to be exactly the same on a byte level.
Do we want the DOM representation to remain exactly the same or are we
fine with some small changes? We don't need to be super strict with this
but having some guidelines that are only broken in special cases would
be helpful.

> If it were only me, I\u2019d pick Perl for its shell-like features and regex
> support that make it a great tool for writing tests in my opinion.
> However, not that many developers are familiar with Perl nowadays.

I'd prefer Python, even if only for consistency with the main code base.
I don't feel strongly about it but introducing another programming
language has its downsides.

Re: [PATCH] Proof-of-concept of a dual PHP–Python stack

[Lukas Fleischer]

On Fri, 13 Mar 2020 at 14:13:56, Frédéric Mangano-Tarumi wrote:
> Lukas Fleischer [2020-03-11 19:44:37 -0400]
> > Thanks! I like the approach. I wonder what the performance impact of
> > always querying the Python backend first would be, though, especially at
> > the beginning when most requests are expected to yield a 404.
> 
> One way or the other, I don\u2019t think it\u2019s worth worrying about since 
> most
> 404 consist of accessing a local socket and exchanging a few KB. Hardly
> any disk or database access is performed. Best way to be sure is to
> measure it under load though. Are the AUR servers often overloaded?

Being overloaded is a relative term. Yes, the AUR servers are often
under heavy load, with millions of requests every day.

> > Alternatively, would it make sense to use multiple location blocks and
> > use the right upstream based on matching the path against a predefined
> > set of patterns? It would add some additional maintenance work but since
> > the overall plan is to migrate everything to Python eventually, it would
> > exist only temporarily.
> 
> I couldn\u2019t find a smart way to do it without turning it into a
> maintenance burden. Beside, I can\u2019t see any advantage over the fallback
> approach, except a performance speedup which I believe would be
> irrelevant.

Fair enough. We can always keep it in mind as an alternative solution in
case there are any issues with the fallback approach.

I also wonder whether we should ever use version with two backends in
production. It might make sense to only switch over once the port has
been completed.

> > For an actual first patch to be merged, I suggest porting the RPC
> > interface which is rather small and largely independent from other parts
> > of the code.
> 
> Sure! But first, are there other approaches you would like to try out
> before we begin the serious work?

Your proposal makes the rewrite relatively easy, has low overhead and at
least one other person actively working on the rewrite (I briefly
discussed it with Filipe) likes it. Unless somebody else wants to
suggest an alternative approach here, I think we're good to go!

> 
> Also, I\u2019d like to make a proposal about regression testing to limit the
> best we can potential bugs introduced by the rewrite.

Great!

Re: [PATCH] Proof-of-concept of a dual PHP–Python stack

[Lukas Fleischer]

On Sat, 07 Mar 2020 at 16:01:52, Frédéric Mangano-Tarumi wrote:
> Here\u2019s a demonstration of a Python web stack running next to the current
> PHP stack, such that it\u2019s invisible to the client.
> 
> This approach aims at providing a way to migrate the PHP code base to
> Python, one endpoint after another, with as little glue as possible to
> have the two backends collaborate. Since they are both mounted on the
> web root, Python could implement, say, /packages/ and leave the rest to
> PHP. As soon as PHP yields it by returning 404 on this URL, Python will
> take over automatically.
> [...]
> ---
>  aurweb/wsgi.py  | 15 +++
>  conf/nginx.conf | 23 +++
>  2 files changed, 38 insertions(+)
>  create mode 100644 aurweb/wsgi.py
>  create mode 100644 conf/nginx.conf

Thanks! I like the approach. I wonder what the performance impact of
always querying the Python backend first would be, though, especially at
the beginning when most requests are expected to yield a 404.

Alternatively, would it make sense to use multiple location blocks and
use the right upstream based on matching the path against a predefined
set of patterns? It would add some additional maintenance work but since
the overall plan is to migrate everything to Python eventually, it would
exist only temporarily.

I guess we could use a similar approach if we ever wanted to decouple
certain endpoints completely and make them a separate app (optionally
sharing some code with the "main" backend).

For an actual first patch to be merged, I suggest porting the RPC
interface which is rather small and largely independent from other parts
of the code. This patch should also add instructions to the
documentation: both INSTALL and doc/maintenance.txt need to be updated.
Maybe also add a note to README.md.

Re: [PATCH 2/4] Support running tests from any directory

[Lukas Fleischer]

On Sat, 29 Feb 2020 at 01:01:38, Frédéric Mangano-Tarumi wrote:
> ---
>  test/setup.sh   | 5 ++---
>  test/t1100-git-auth.t   | 2 +-
>  test/t1200-git-serve.t  | 2 +-
>  test/t1300-git-update.t | 2 +-
>  test/t2100-mkpkglists.t | 2 +-
>  test/t2200-tuvotereminder.t | 2 +-
>  test/t2300-pkgmaint.t   | 2 +-
>  test/t2400-aurblup.t| 2 +-
>  test/t2500-notify.t | 2 +-
>  test/t2600-rendercomment.t  | 2 +-
>  test/t2700-usermaint.t  | 2 +-
>  11 files changed, 12 insertions(+), 13 deletions(-)

Merged, thanks!

Re: [PATCH 3/4] test/Makefile: Run tests with prove when available

[Lukas Fleischer]

On Sat, 29 Feb 2020 at 01:02:04, Frédéric Mangano-Tarumi wrote:
> ---
>  test/Makefile | 7 +++
>  1 file changed, 7 insertions(+)

Merged, thanks!

[PATCH] Properly escape passwords in the account edit form

[Lukas Fleischer]

Addresses FS#65639.

Signed-off-by: Lukas Fleischer 
---
Our live setup at aur.archlinux.org is already patched.

 web/template/account_edit_form.php | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/web/template/account_edit_form.php 
b/web/template/account_edit_form.php
index a4ea994..4ce6b87 100644
--- a/web/template/account_edit_form.php
+++ b/web/template/account_edit_form.php
@@ -157,12 +157,12 @@


:
-   
+   

 

:
-   
+   



-- 
2.25.1

[PATCH] Properly escape passwords in the account edit form

[Lukas Fleischer]

Addresses FS#65639.

Signed-off-by: Lukas Fleischer 
---
 web/template/account_edit_form.php | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/web/template/account_edit_form.php 
b/web/template/account_edit_form.php
index a4ea994..4ce6b87 100644
--- a/web/template/account_edit_form.php
+++ b/web/template/account_edit_form.php
@@ -157,12 +157,12 @@


:
-   
+   

 

:
-   
+   



-- 
2.25.1

Re: [PATCH] Set up Alembic for database migrations

[Lukas Fleischer]

On Wed, 26 Feb 2020 at 22:10:09, Frédéric Mangano-Tarumi wrote:
> Lukas Fleischer [2020-02-26 14:00:51 +0100]
> > We should also think about phasing out the text files in the upgrading/
> > subdirectory. Maybe we should simply keep them for one or two releases
> > and remove the directory entirely later? If anybody ever still needs
> > them after they have been removed, they can still be recovered from the
> > Git history...
> 
> I don\u2019t know who reads the upgrading instructions, but how about having
> a 4.x.0-and-later.txt saying that SQL migrations are now handled by
> Alembic, and referring to migrations/README?
> 
> Let\u2019s also keep in mind that some upgrades might require manual
> intervention anyway. In my opinion these breaking changes should be
> documented in commit messages and synthesized in release notes, but
> whatever the AUR team is used to should be fine too.

I like that suggestion.

In the past, we did not have any release notes, except for announcement
emails sent to the mailing lists. I am not sure how easy it would be to
collect those emails, compile a CHANGES.md file and add it to the source
tree, alongside with upgrade instructions extracted from the upgrading/
subdirectory. This would allow us to get rid of the directory entirely.

Re: [PATCH] Set up Alembic for database migrations

[Lukas Fleischer]

On Sat, 22 Feb 2020 at 22:31:26, Frédéric Mangano-Tarumi wrote:
> ---
>  INSTALL   |  4 +-
>  TESTING   |  3 +-
>  alembic.ini   | 86 +++
>  aurweb/initdb.py  |  9 
>  aurweb/schema.py  |  8 
>  migrations/README | 48 ++
>  migrations/env.py | 73 +
>  migrations/script.py.mako | 24 +++
>  8 files changed, 252 insertions(+), 3 deletions(-)
>  create mode 100644 alembic.ini
>  create mode 100644 migrations/README
>  create mode 100644 migrations/env.py
>  create mode 100644 migrations/script.py.mako

Looks great, thanks! I merged this patch and the two followup patches
into the pu branch. I will do some more testing and merge them to master
if there are no issues.

We should also think about phasing out the text files in the upgrading/
subdirectory. Maybe we should simply keep them for one or two releases
and remove the directory entirely later? If anybody ever still needs
them after they have been removed, they can still be recovered from the
Git history...

Re: [PATCH] Change the extension of TAP test suites to .t

[Lukas Fleischer]

On Sun, 23 Feb 2020 at 19:53:13, Frédéric Mangano-Tarumi wrote:
> This is the common convention for TAP, and makes harnesses like prove
> automatically detect them. Plus, test suites don\u2019t have to be shell
> scripts anymore.
> ---
>  test/Makefile| 2 +-
>  test/{t1100-git-auth.sh => t1100-git-auth.t} | 0
>  test/{t1200-git-serve.sh => t1200-git-serve.t}   | 0
>  test/{t1300-git-update.sh => t1300-git-update.t} | 0
>  test/{t2100-mkpkglists.sh => t2100-mkpkglists.t} | 0
>  test/{t2200-tuvotereminder.sh => t2200-tuvotereminder.t} | 0
>  test/{t2300-pkgmaint.sh => t2300-pkgmaint.t} | 0
>  test/{t2400-aurblup.sh => t2400-aurblup.t}   | 0
>  test/{t2500-notify.sh => t2500-notify.t} | 0
>  test/{t2600-rendercomment.sh => t2600-rendercomment.t}   | 0
>  test/{t2700-usermaint.sh => t2700-usermaint.t}   | 0
>  11 files changed, 1 insertion(+), 1 deletion(-)
>  rename test/{t1100-git-auth.sh => t1100-git-auth.t} (100%)
>  rename test/{t1200-git-serve.sh => t1200-git-serve.t} (100%)
>  rename test/{t1300-git-update.sh => t1300-git-update.t} (100%)
>  rename test/{t2100-mkpkglists.sh => t2100-mkpkglists.t} (100%)
>  rename test/{t2200-tuvotereminder.sh => t2200-tuvotereminder.t} (100%)
>  rename test/{t2300-pkgmaint.sh => t2300-pkgmaint.t} (100%)
>  rename test/{t2400-aurblup.sh => t2400-aurblup.t} (100%)
>  rename test/{t2500-notify.sh => t2500-notify.t} (100%)
>  rename test/{t2600-rendercomment.sh => t2600-rendercomment.t} (100%)
>  rename test/{t2700-usermaint.sh => t2700-usermaint.t} (100%)

Merged into pu, thanks!

Re: [PATCH] migrate the database schema to SQLAlchemy

[Lukas Fleischer]

On Sat, 22 Feb 2020 at 23:28:25, Frédéric Mangano-Tarumi wrote:
> Lukas Fleischer [2020-02-22 22:43:31 +0100]
> > > +5) Create a new MySQL database and a user and import the aurweb SQL 
> > > schema:
> > > +
> > > +$ python -m aurweb.initdb
> > > +
> > 
> > I noticed that this step is slightly confusing: the command must be
> > executed from the aurweb/ subdirectory, not the top-level directory of
> > the project.
> 
> Could you please try running `python -m aurweb.initdb --help` from the
> top-level directory, then in the aurweb subdirectory? In a vanilla
> setup, only the former works for me.

You are right. I guess I was confused by nested subdirectories with the
same name. Sorry for the noise.

Re: [PATCH] migrate the database schema to SQLAlchemy

[Lukas Fleischer]

On Sun, 16 Feb 2020 at 21:56:10, Frédéric Mangano-Tarumi wrote:
> The new schema was generated with sqlacodegen and then manually adjusted
> to fit schema/aur-schema.sql faithfully, both in the organisation of the
> code and in the SQL generated by SQLAlchemy.
> 
> Initializing the database now requires the new tool aurweb.initdb.
> References to aur-schema.sql have been updated and the old schema
> dropped.
> ---
>  INSTALL|  12 +-
>  TESTING|  23 +--
>  aurweb/db.py   |  27 +++
>  aurweb/initdb.py   |  47 +
>  aurweb/schema.py   | 387 ++
>  schema/Makefile|  12 --
>  schema/aur-schema.sql  | 415 -
>  schema/reloadtestdb.sh |  29 ---
>  test/Makefile  |   6 +-
>  test/setup.sh  |   5 +-
>  10 files changed, 481 insertions(+), 482 deletions(-)
>  create mode 100644 aurweb/initdb.py
>  create mode 100644 aurweb/schema.py
>  delete mode 100644 schema/Makefile
>  delete mode 100644 schema/aur-schema.sql
>  delete mode 100755 schema/reloadtestdb.sh
> 
> diff --git a/INSTALL b/INSTALL
> index 7170aea1..68fe5dcd 100644
> --- a/INSTALL
> +++ b/INSTALL
> @@ -45,16 +45,16 @@ read the instructions below.
> [...]
> +5) Create a new MySQL database and a user and import the aurweb SQL schema:
> +
> +$ python -m aurweb.initdb
> +

I noticed that this step is slightly confusing: the command must be
executed from the aurweb/ subdirectory, not the top-level directory of
the project. I will probably add an additional line

$ cd /srv/http/aurweb/aurweb/

before the command.

Re: TAP conventions for aurweb’s test suites

[Lukas Fleischer]

On Sat, 22 Feb 2020 at 16:24:54, Frédéric Mangano-Tarumi wrote:
> Exactly. Using .t instead of .sh is about opening doors. Rewriting
> existing tests would have no benefit per se. The beauty of TAP\u2019s
> language-agnosticness is the liberty to use to right tool for the right
> job. I\u2019ll probably bring up testing in Python specifically soon.

Sounds great!

> > I like the idea of being able to use prove but I'd prefer still having a
> > way to run tests without having prove installed. How about adding a
> > "prove" target to the Makefile instead of changing `make check`?
> 
> In Arch Linux, prove is part of the perl package, which is a direct
> dependency of git and openssl. Even though Perl is being less and less
> used these days, I don\u2019t think you\u2019ll ever find a system without 
> Perl
> installed. Therefore, I personally don\u2019t think it\u2019s worth checking
> whether prove is installed or not.

It is true that Perl is currently installed on pretty much every Arch
installation but we are working on changing that. Getting rid of the
OpenSSL dependency on Perl [1] is a first step. You are right that even
after those changes, Perl will likely continue to be installed on most
systems; at least the interactively used ones.

> If we do want to support systems without prove anyway, I suggest we find
> a way to let make detect if prove is installed, and depending on the
> result change the behavior of make check. `make prove` has no utility
> as someone who knows about prove would call it directly instead.
> [...]
> Usually, when a project uses make, it provides a Makefile at the root of
> the project, with many rules including check, and is run from the root
> of the project. test/Makefile is like hidden and lonely. I wouldn\u2019t mind
> keeping make check, but I think most developers would rather know they
> can type \u201cprove a.t b.t\u201d to run specific tests and have nice 
> reports.
> `make check` is simpler for users, but prove is better for developers,
> and given the nature of aurweb we should target the latter.

I guess you are right. We could recommend using prove and provide the
Makefile only for compatibility. I don't feel very strongly about this,
but even though the Makefile is hidden, keeping a simple useful 7-line
file won't hurt. All this should be explained in a README file.

> aurweb could benefit from a Makefile if it had rules like \u201cmake
> test-env\u201d to automatically execute the steps in TESTING, \u201cmake 
> run\u201d to
> start the PHP\u202ftest server, etc. To exaggerate a bit, having the
> developer manually set up their database and then come up with \u201chey we
> have a hidden make check rule to save you from typing prove\u201d somewhat
> feels weird.

True. For that matter, there are plans to add a Dockerfile to the source
tree to aid with building a testing environment. We are still working on
this.

> Don\u2019t worry! I\u2019ll send patches progressively and you\u2019ll tell 
> me if
> something feels too disruptive.

Awesome, thanks again for your work!

[1] https://www.archlinux.org/todo/perl-transient-openssl-dependencies/

[PATCH] Fix HTML code in the account search results table

[Lukas Fleischer]

Do not add an opening  tag for every row. Instead, wrap all rows
in .

While at it, also simplify the code used to color the rows.

Signed-off-by: Lukas Fleischer 
---
 web/template/account_search_results.php | 20 +---
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/web/template/account_search_results.php 
b/web/template/account_search_results.php
index 81cd818..0f7eb7a 100644
--- a/web/template/account_search_results.php
+++ b/web/template/account_search_results.php
@@ -16,17 +16,9 @@ else:



-$row):
-   if ($i % 2):
-   $c = "even";
-   else:
-   $c = "odd";
-   endif;
-   ?>
-   
-   
+   
+$row): ?>
+   
">


@@ -49,10 +41,8 @@ else:



-   
+   
+   

 

-- 
2.25.1

Re: TAP conventions for aurweb’s test suites

[Lukas Fleischer]

On Fri, 21 Feb 2020 at 23:41:06, Frédéric Mangano-Tarumi wrote:
> prove comes with its conventions, which are from the original TAP.
> First, it expects test programs to end with `.t`, which is restrictive
> enough to exclude non-test files, but doesn\u2019t bind to any specific
> language since the shebang is used to know how to run each test. This is
> by the way a great opportunity to start writing tests in Python too.

The current tests mostly execute commands and compare their outputs, and
I don't see much benefit in rewriting them in Python. That being said, I
think we could benefit from additional unit tests.

> Another convention is to name the test directory `t` and to call prove
> from the root of the project. This one is harder to use since all the
> existing tests assume they\u2019re run from the `test` directory. A simple cd
> at the beginning of every script is probably a good solution, but let\u2019s
> leave that problem open for now. While we can run prove from the `test`
> directory, running tests from the root has the notable advantage of
> letting us import aurweb Python modules without hacking PYTHONPATH.
> 
> Here are various suggestions:
> 
> 1. Change the extension of all the test programs to `.t`.

Sounds good.

> 2. Create `test\README` to tell developers about TAP and prove.

Great idea!

> 3. Make `make check` call prove.

I like the idea of being able to use prove but I'd prefer still having a
way to run tests without having prove installed. How about adding a
"prove" target to the Makefile instead of changing `make check`?

> 4. Delete the `make t1234-xxx.sh` rules.
> 5. Delete the whole Makefile altogether.

Why? Those are small, super low maintenance, and provide the benefit of
being able to easily run the test suite without prove.

> 6. Run the test programs from the project root.
> 7. Rename the `test` directory to `t`.

Sounds good.

> 1 to 4 are cheap and have a significant added value in my opinion.
> 5 to 7 less so, but I think that if we want to use TAP, we should stick
> to its conventions.

I agree with sticking to TAP conventions but it should be easy to keep
almost everything working as-is at the same time without any extra work.

Thanks for bringing this up!

Lukas

[PATCH 1/2] README.md: fix a small typo

[Lukas Fleischer]

From: Yaron Shahrabani 

Signed-off-by: Lukas Fleischer 
---
 README.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/README.md b/README.md
index a4ab584..b2095b3 100644
--- a/README.md
+++ b/README.md
@@ -9,7 +9,7 @@ and installed using the Arch Linux package manager `pacman`.
 The aurweb project includes
 
 * A web interface to search for packaging scripts and display package details.
-* A SSH/Git interface to submit and update packages and package meta data.
+* An SSH/Git interface to submit and update packages and package meta data.
 * Community features such as comments, votes, package flagging and requests.
 * Editing/deletion of packages and accounts by Trusted Users and Developers.
 * Area for Trusted Users to post AUR-related proposals and vote on them.
-- 
2.25.1

[PATCH 2/2] README.md: add references to Transifex

[Lukas Fleischer]

Signed-off-by: Lukas Fleischer 
---
 README.md | 8 
 1 file changed, 8 insertions(+)

diff --git a/README.md b/README.md
index b2095b3..f7285a5 100644
--- a/README.md
+++ b/README.md
@@ -38,3 +38,11 @@ Links
 * Questions, comments, and patches related to aurweb can be sent to the AUR
   development mailing list: aur-dev@archlinux.org -- mailing list archives:
   https://mailman.archlinux.org/mailman/listinfo/aur-dev
+
+Translations
+
+
+Translations are welcome via our Transifex project at
+https://www.transifex.com/lfleischer/aurweb; see `doc/i18n.txt` for details.
+
+![Transifex](http://www.transifex.net/projects/p/aurweb/chart/image_png)
-- 
2.25.1

Re: [aur-dev][PATCH 0/2] php warning fixes

[Lukas Fleischer]

On Wed, 12 Feb 2020 at 21:16:36, Eli Schwartz wrote:
> I had this sitting around since December and forgot to send it in,
> offered FWIW. It's slightly different in implementation and has more
> wordy commit messages.

Thanks. I will discard the other patch I submitted earlier (Verify that
returned rows exist before extracting columns) and apply these two
instead.

[PATCH v2] Verify that returned rows exist before extracting columns

[Lukas Fleischer]

Fix PHP notices such as "Trying to access array offset on value of type
bool" or "Trying to access array offset on value of type null".

Signed-off-by: Lukas Fleischer 
---
 web/html/login.php   |  4 +++-
 web/lib/aur.inc.php  | 21 +
 web/lib/pkgfuncs.inc.php |  3 +++
 3 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/web/html/login.php b/web/html/login.php
index 0145441..5308147 100644
--- a/web/html/login.php
+++ b/web/html/login.php
@@ -6,7 +6,9 @@ include_once("aur.inc.php");
 $disable_http_login = config_get_bool('options', 'disable_http_login');
 if (!$disable_http_login || (isset($_SERVER['HTTPS']) && $_SERVER['HTTPS'])) {
$login = try_login();
-   $login_error = $login['error'];
+   if ($login) {
+   $login_error = $login['error'];
+   }
 }
 
 html_header('AUR ' . __("Login"));
diff --git a/web/lib/aur.inc.php b/web/lib/aur.inc.php
index e9530fc..2507df6 100644
--- a/web/lib/aur.inc.php
+++ b/web/lib/aur.inc.php
@@ -197,6 +197,9 @@ function username_from_id($id) {
}
 
$row = $result->fetch(PDO::FETCH_NUM);
+   if (!$row) {
+   return null;
+   }
return $row[0];
 }
 
@@ -222,6 +225,9 @@ function username_from_sid($sid="") {
}
$row = $result->fetch(PDO::FETCH_NUM);
 
+   if (!$row) {
+   return null;
+   }
return $row[0];
 }
 
@@ -339,6 +345,9 @@ function email_from_sid($sid="") {
}
$row = $result->fetch(PDO::FETCH_NUM);
 
+   if (!$row) {
+   return null;
+   }
return $row[0];
 }
 
@@ -365,6 +374,9 @@ function account_from_sid($sid="") {
}
$row = $result->fetch(PDO::FETCH_NUM);
 
+   if (!$row) {
+   return null;
+   }
return $row[0];
 }
 
@@ -390,6 +402,9 @@ function uid_from_sid($sid="") {
}
$row = $result->fetch(PDO::FETCH_NUM);
 
+   if (!$row) {
+   return null;
+   }
return $row[0];
 }
 
@@ -512,6 +527,9 @@ function uid_from_username($username) {
}
 
$row = $result->fetch(PDO::FETCH_NUM);
+   if (!$row) {
+   return null;
+   }
return $row[0];
 }
 
@@ -546,6 +564,9 @@ function uid_from_email($email) {
}
 
$row = $result->fetch(PDO::FETCH_NUM);
+   if (!$row) {
+   return null;
+   }
return $row[0];
 }
 
diff --git a/web/lib/pkgfuncs.inc.php b/web/lib/pkgfuncs.inc.php
index a4cd17a..b30bfa9 100644
--- a/web/lib/pkgfuncs.inc.php
+++ b/web/lib/pkgfuncs.inc.php
@@ -147,6 +147,9 @@ function pkg_from_name($name="") {
return;
}
$row = $result->fetch(PDO::FETCH_NUM);
+   if (!$row) {
+   return null;
+   }
return $row[0];
 }
 
-- 
2.25.0

[PATCH] Verify that return rows exist before extracting columns

[Lukas Fleischer]

Signed-off-by: Lukas Fleischer 
---
 web/lib/aur.inc.php  | 21 +
 web/lib/pkgfuncs.inc.php |  3 +++
 2 files changed, 24 insertions(+)

diff --git a/web/lib/aur.inc.php b/web/lib/aur.inc.php
index e9530fc..2507df6 100644
--- a/web/lib/aur.inc.php
+++ b/web/lib/aur.inc.php
@@ -197,6 +197,9 @@ function username_from_id($id) {
}
 
$row = $result->fetch(PDO::FETCH_NUM);
+   if (!$row) {
+   return null;
+   }
return $row[0];
 }
 
@@ -222,6 +225,9 @@ function username_from_sid($sid="") {
}
$row = $result->fetch(PDO::FETCH_NUM);
 
+   if (!$row) {
+   return null;
+   }
return $row[0];
 }
 
@@ -339,6 +345,9 @@ function email_from_sid($sid="") {
}
$row = $result->fetch(PDO::FETCH_NUM);
 
+   if (!$row) {
+   return null;
+   }
return $row[0];
 }
 
@@ -365,6 +374,9 @@ function account_from_sid($sid="") {
}
$row = $result->fetch(PDO::FETCH_NUM);
 
+   if (!$row) {
+   return null;
+   }
return $row[0];
 }
 
@@ -390,6 +402,9 @@ function uid_from_sid($sid="") {
}
$row = $result->fetch(PDO::FETCH_NUM);
 
+   if (!$row) {
+   return null;
+   }
return $row[0];
 }
 
@@ -512,6 +527,9 @@ function uid_from_username($username) {
}
 
$row = $result->fetch(PDO::FETCH_NUM);
+   if (!$row) {
+   return null;
+   }
return $row[0];
 }
 
@@ -546,6 +564,9 @@ function uid_from_email($email) {
}
 
$row = $result->fetch(PDO::FETCH_NUM);
+   if (!$row) {
+   return null;
+   }
return $row[0];
 }
 
diff --git a/web/lib/pkgfuncs.inc.php b/web/lib/pkgfuncs.inc.php
index a4cd17a..b30bfa9 100644
--- a/web/lib/pkgfuncs.inc.php
+++ b/web/lib/pkgfuncs.inc.php
@@ -147,6 +147,9 @@ function pkg_from_name($name="") {
return;
}
$row = $result->fetch(PDO::FETCH_NUM);
+   if (!$row) {
+   return null;
+   }
return $row[0];
 }
 
-- 
2.25.0

[PATCH v5 2/2] Make SMTP port and authentication configurable

[Lukas Fleischer]

Add more options to configure the smtplib implementation for sending
notification emails.

The port can be changed using the new smtp-port option.

Encryption can be configured using smtp-use-ssl and smtp-use-starttls.
Keep in mind that you usually also need to change the port when enabling
either of these options.

Authentication can be configured using smtp-user and smtp-password.
Authentication is disabled if either of these values is empty.

Signed-off-by: Lukas Fleischer 
---
 aurweb/scripts/notify.py | 20 +++-
 conf/config.defaults |  5 +
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/aurweb/scripts/notify.py b/aurweb/scripts/notify.py
index 6c5c709..5b18a47 100755
--- a/aurweb/scripts/notify.py
+++ b/aurweb/scripts/notify.py
@@ -65,6 +65,7 @@ class Notification:
 return body.rstrip()
 
 def send(self):
+sendmail = aurweb.config.get('notifications', 'sendmail')
 sender = aurweb.config.get('notifications', 'sender')
 reply_to = aurweb.config.get('notifications', 'reply-to')
 reason = self.__class__.__name__
@@ -95,8 +96,25 @@ class Notification:
 else:
 # send email using smtplib; no local MTA required
 server_addr = aurweb.config.get('notifications', 'smtp-server')
+server_port = aurweb.config.getint('notifications', 
'smtp-port')
+use_ssl = aurweb.config.getboolean('notifications', 
'smtp-use-ssl')
+use_starttls = aurweb.config.getboolean('notifications', 
'smtp-use-starttls')
+user = aurweb.config.get('notifications', 'smtp-user')
+passwd = aurweb.config.get('notifications', 'smtp-password')
+
+if use_ssl:
+server = smtplib.SMTP_SSL(server_addr, server_port)
+else:
+server = smtplib.SMTP(server_addr, server_port)
+
+if use_starttls:
+server.ehlo()
+server.starttls()
+server.ehlo()
+
+if user and passwd:
+server.login(user, passwd)
 
-server = smtplib.SMTP(server_addr)
 server.set_debuglevel(0)
 server.sendmail(sender, recipient, msg.as_bytes())
 server.quit()
diff --git a/conf/config.defaults b/conf/config.defaults
index 23d46b0..b69d031 100644
--- a/conf/config.defaults
+++ b/conf/config.defaults
@@ -49,6 +49,11 @@ window_length = 86400
 notify-cmd = /usr/local/bin/aurweb-notify
 sendmail =
 smtp-server = localhost
+smtp-port = 25
+smtp-use-ssl = 0
+smtp-use-starttls = 0
+smtp-user =
+smtp-password =
 sender = not...@aur.archlinux.org
 reply-to = nore...@aur.archlinux.org
 
-- 
2.25.0

[PATCH v5 1/2] Support smtplib for sending emails

[Lukas Fleischer]

Support mail delivery without a local MTA. Instead, an SMTP server can
now be configured using the smtp-server option in the [notifications]
section. In order to use this option, the value of the sendmail option
must be empty.

Signed-off-by: Lukas Fleischer 
---
Disable debug output.

 aurweb/scripts/notify.py | 22 ++
 conf/config.defaults |  3 ++-
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/aurweb/scripts/notify.py b/aurweb/scripts/notify.py
index b0f218b..6c5c709 100755
--- a/aurweb/scripts/notify.py
+++ b/aurweb/scripts/notify.py
@@ -1,6 +1,8 @@
 #!/usr/bin/env python3
 
 import email.mime.text
+import email.utils
+import smtplib
 import subprocess
 import sys
 import textwrap
@@ -63,7 +65,6 @@ class Notification:
 return body.rstrip()
 
 def send(self):
-sendmail = aurweb.config.get('notifications', 'sendmail')
 sender = aurweb.config.get('notifications', 'sender')
 reply_to = aurweb.config.get('notifications', 'reply-to')
 reason = self.__class__.__name__
@@ -79,13 +80,26 @@ class Notification:
 msg['Reply-to'] = reply_to
 msg['To'] = to
 msg['X-AUR-Reason'] = reason
+msg['Date'] = email.utils.formatdate(localtime=True)
 
 for key, value in self.get_headers().items():
 msg[key] = value
 
-p = subprocess.Popen([sendmail, '-t', '-oi'],
- stdin=subprocess.PIPE)
-p.communicate(msg.as_bytes())
+sendmail = aurweb.config.get('notifications', 'sendmail')
+if sendmail:
+# send email using the sendmail binary specified in the
+# configuration file
+p = subprocess.Popen([sendmail, '-t', '-oi'],
+ stdin=subprocess.PIPE)
+p.communicate(msg.as_bytes())
+else:
+# send email using smtplib; no local MTA required
+server_addr = aurweb.config.get('notifications', 'smtp-server')
+
+server = smtplib.SMTP(server_addr)
+server.set_debuglevel(0)
+server.sendmail(sender, recipient, msg.as_bytes())
+server.quit()
 
 
 class ResetKeyNotification(Notification):
diff --git a/conf/config.defaults b/conf/config.defaults
index c519eae..23d46b0 100644
--- a/conf/config.defaults
+++ b/conf/config.defaults
@@ -47,7 +47,8 @@ window_length = 86400
 
 [notifications]
 notify-cmd = /usr/local/bin/aurweb-notify
-sendmail = /usr/bin/sendmail
+sendmail =
+smtp-server = localhost
 sender = not...@aur.archlinux.org
 reply-to = nore...@aur.archlinux.org
 
-- 
2.25.0

[PATCH v4 2/2] Make SMTP port and authentication configurable

[Lukas Fleischer]

Add more options to configure the smtplib implementation for sending
notification emails.

The port can be changed using the new smtp-port option.

Encryption can be configured using smtp-use-ssl and smtp-use-starttls.
Keep in mind that you usually also need to change the port when enabling
either of these options.

Authentication can be configured using smtp-user and smtp-password.
Authentication is disabled if either of these values is empty.

Signed-off-by: Lukas Fleischer 
---
Fix variable naming inconsistency with the user and passwd variables.

 aurweb/scripts/notify.py | 20 +++-
 conf/config.defaults |  5 +
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/aurweb/scripts/notify.py b/aurweb/scripts/notify.py
index 941187e..31b78e9 100755
--- a/aurweb/scripts/notify.py
+++ b/aurweb/scripts/notify.py
@@ -65,6 +65,7 @@ class Notification:
 return body.rstrip()
 
 def send(self):
+sendmail = aurweb.config.get('notifications', 'sendmail')
 sender = aurweb.config.get('notifications', 'sender')
 reply_to = aurweb.config.get('notifications', 'reply-to')
 reason = self.__class__.__name__
@@ -95,8 +96,25 @@ class Notification:
 else:
 # send email using smtplib; no local MTA required
 server_addr = aurweb.config.get('notifications', 'smtp-server')
+server_port = aurweb.config.getint('notifications', 
'smtp-port')
+use_ssl = aurweb.config.getboolean('notifications', 
'smtp-use-ssl')
+use_starttls = aurweb.config.getboolean('notifications', 
'smtp-use-starttls')
+user = aurweb.config.get('notifications', 'smtp-user')
+passwd = aurweb.config.get('notifications', 'smtp-password')
+
+if use_ssl:
+server = smtplib.SMTP_SSL(server_addr, server_port)
+else:
+server = smtplib.SMTP(server_addr, server_port)
+
+if use_starttls:
+server.ehlo()
+server.starttls()
+server.ehlo()
+
+if user and passwd:
+server.login(user, passwd)
 
-server = smtplib.SMTP(server_addr)
 server.set_debuglevel(1)
 server.sendmail(sender, recipient, msg.as_bytes())
 server.quit()
diff --git a/conf/config.defaults b/conf/config.defaults
index 23d46b0..b69d031 100644
--- a/conf/config.defaults
+++ b/conf/config.defaults
@@ -49,6 +49,11 @@ window_length = 86400
 notify-cmd = /usr/local/bin/aurweb-notify
 sendmail =
 smtp-server = localhost
+smtp-port = 25
+smtp-use-ssl = 0
+smtp-use-starttls = 0
+smtp-user =
+smtp-password =
 sender = not...@aur.archlinux.org
 reply-to = nore...@aur.archlinux.org
 
-- 
2.25.0

[PATCH v4 1/2] Support smtplib for sending emails

[Lukas Fleischer]

Support mail delivery without a local MTA. Instead, an SMTP server can
now be configured using the smtp-server option in the [notifications]
section. In order to use this option, the value of the sendmail option
must be empty.

Signed-off-by: Lukas Fleischer 
---
 aurweb/scripts/notify.py | 22 ++
 conf/config.defaults |  3 ++-
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/aurweb/scripts/notify.py b/aurweb/scripts/notify.py
index b0f218b..941187e 100755
--- a/aurweb/scripts/notify.py
+++ b/aurweb/scripts/notify.py
@@ -1,6 +1,8 @@
 #!/usr/bin/env python3
 
 import email.mime.text
+import email.utils
+import smtplib
 import subprocess
 import sys
 import textwrap
@@ -63,7 +65,6 @@ class Notification:
 return body.rstrip()
 
 def send(self):
-sendmail = aurweb.config.get('notifications', 'sendmail')
 sender = aurweb.config.get('notifications', 'sender')
 reply_to = aurweb.config.get('notifications', 'reply-to')
 reason = self.__class__.__name__
@@ -79,13 +80,26 @@ class Notification:
 msg['Reply-to'] = reply_to
 msg['To'] = to
 msg['X-AUR-Reason'] = reason
+msg['Date'] = email.utils.formatdate(localtime=True)
 
 for key, value in self.get_headers().items():
 msg[key] = value
 
-p = subprocess.Popen([sendmail, '-t', '-oi'],
- stdin=subprocess.PIPE)
-p.communicate(msg.as_bytes())
+sendmail = aurweb.config.get('notifications', 'sendmail')
+if sendmail:
+# send email using the sendmail binary specified in the
+# configuration file
+p = subprocess.Popen([sendmail, '-t', '-oi'],
+ stdin=subprocess.PIPE)
+p.communicate(msg.as_bytes())
+else:
+# send email using smtplib; no local MTA required
+server_addr = aurweb.config.get('notifications', 'smtp-server')
+
+server = smtplib.SMTP(server_addr)
+server.set_debuglevel(1)
+server.sendmail(sender, recipient, msg.as_bytes())
+server.quit()
 
 
 class ResetKeyNotification(Notification):
diff --git a/conf/config.defaults b/conf/config.defaults
index c519eae..23d46b0 100644
--- a/conf/config.defaults
+++ b/conf/config.defaults
@@ -47,7 +47,8 @@ window_length = 86400
 
 [notifications]
 notify-cmd = /usr/local/bin/aurweb-notify
-sendmail = /usr/bin/sendmail
+sendmail =
+smtp-server = localhost
 sender = not...@aur.archlinux.org
 reply-to = nore...@aur.archlinux.org
 
-- 
2.25.0

[PATCH] Update README and convert to Markdown syntax

[Lukas Fleischer]

Signed-off-by: Lukas Fleischer 
---
 README => README.md | 34 --
 1 file changed, 8 insertions(+), 26 deletions(-)
 rename README => README.md (73%)

diff --git a/README b/README.md
similarity index 73%
rename from README
rename to README.md
index e633ec3..a4ab584 100644
--- a/README
+++ b/README.md
@@ -17,32 +17,14 @@ The aurweb project includes
 Directory Layout
 
 
-aurweb::
-   aurweb Python modules.
-
-conf::
-   Configuration and configuration templates.
-
-doc::
-   Project documentation.
-
-po::
-   Translation files for strings in the aurweb interface.
-
-schema::
-   Schema for the SQL database. Script for dummy data generation.
-
-scripts::
-   Scripts for AUR maintenance.
-
-test::
-   Test suite and test cases.
-
-upgrading::
-   Instructions for upgrading setups from one release to another.
-
-web::
-   Web interface for the AUR.
+* `aurweb`: aurweb Python modules, Git interface and maintenance scripts
+* `conf`: configuration and configuration templates
+* `doc`: project documentation
+* `po`: translation files for strings in the aurweb interface
+* `schema`: schema for the SQL database
+* `test`: test suite and test cases
+* `upgrading`: instructions for upgrading setups from one release to another
+* `web`: web interface for the AUR
 
 Links
 -
-- 
2.25.0

[PATCH v3 1/2] Support smtplib for sending emails

[Lukas Fleischer]

Support mail delivery without a local MTA. Instead, an SMTP server can
now be configured using the smtp-server option in the [notifications]
section. In order to use this option, the value of the sendmail option
must be empty.

Signed-off-by: Lukas Fleischer 
---
Reintroduce the option to use a local sendmail binary (needed for the
test suite) and only use smtplib if no such binary is configured.

 aurweb/scripts/notify.py | 22 ++
 conf/config.defaults |  3 ++-
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/aurweb/scripts/notify.py b/aurweb/scripts/notify.py
index b0f218b..941187e 100755
--- a/aurweb/scripts/notify.py
+++ b/aurweb/scripts/notify.py
@@ -1,6 +1,8 @@
 #!/usr/bin/env python3
 
 import email.mime.text
+import email.utils
+import smtplib
 import subprocess
 import sys
 import textwrap
@@ -63,7 +65,6 @@ class Notification:
 return body.rstrip()
 
 def send(self):
-sendmail = aurweb.config.get('notifications', 'sendmail')
 sender = aurweb.config.get('notifications', 'sender')
 reply_to = aurweb.config.get('notifications', 'reply-to')
 reason = self.__class__.__name__
@@ -79,13 +80,26 @@ class Notification:
 msg['Reply-to'] = reply_to
 msg['To'] = to
 msg['X-AUR-Reason'] = reason
+msg['Date'] = email.utils.formatdate(localtime=True)
 
 for key, value in self.get_headers().items():
 msg[key] = value
 
-p = subprocess.Popen([sendmail, '-t', '-oi'],
- stdin=subprocess.PIPE)
-p.communicate(msg.as_bytes())
+sendmail = aurweb.config.get('notifications', 'sendmail')
+if sendmail:
+# send email using the sendmail binary specified in the
+# configuration file
+p = subprocess.Popen([sendmail, '-t', '-oi'],
+ stdin=subprocess.PIPE)
+p.communicate(msg.as_bytes())
+else:
+# send email using smtplib; no local MTA required
+server_addr = aurweb.config.get('notifications', 'smtp-server')
+
+server = smtplib.SMTP(server_addr)
+server.set_debuglevel(1)
+server.sendmail(sender, recipient, msg.as_bytes())
+server.quit()
 
 
 class ResetKeyNotification(Notification):
diff --git a/conf/config.defaults b/conf/config.defaults
index c519eae..23d46b0 100644
--- a/conf/config.defaults
+++ b/conf/config.defaults
@@ -47,7 +47,8 @@ window_length = 86400
 
 [notifications]
 notify-cmd = /usr/local/bin/aurweb-notify
-sendmail = /usr/bin/sendmail
+sendmail =
+smtp-server = localhost
 sender = not...@aur.archlinux.org
 reply-to = nore...@aur.archlinux.org
 
-- 
2.25.0

[PATCH v3 2/2] Make SMTP port and authentication configurable

[Lukas Fleischer]

Add more options to configure the smtplib implementation for sending
notification emails.

The port can be changed using the new smtp-port option.

Encryption can be configured using smtp-use-ssl and smtp-use-starttls.
Keep in mind that you usually also need to change the port when enabling
either of these options.

Authentication can be configured using smtp-user and smtp-password.
Authentication is disabled if either of these values is empty.

Signed-off-by: Lukas Fleischer 
---
 aurweb/scripts/notify.py | 20 +++-
 conf/config.defaults |  5 +
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/aurweb/scripts/notify.py b/aurweb/scripts/notify.py
index 941187e..3417721 100755
--- a/aurweb/scripts/notify.py
+++ b/aurweb/scripts/notify.py
@@ -65,6 +65,7 @@ class Notification:
 return body.rstrip()
 
 def send(self):
+sendmail = aurweb.config.get('notifications', 'sendmail')
 sender = aurweb.config.get('notifications', 'sender')
 reply_to = aurweb.config.get('notifications', 'reply-to')
 reason = self.__class__.__name__
@@ -95,8 +96,25 @@ class Notification:
 else:
 # send email using smtplib; no local MTA required
 server_addr = aurweb.config.get('notifications', 'smtp-server')
+server_port = aurweb.config.getint('notifications', 
'smtp-port')
+use_ssl = aurweb.config.getboolean('notifications', 
'smtp-use-ssl')
+use_starttls = aurweb.config.getboolean('notifications', 
'smtp-use-starttls')
+smtp_user = aurweb.config.get('notifications', 'smtp-user')
+smtp_passwd = aurweb.config.get('notifications', 
'smtp-password')
+
+if use_ssl:
+server = smtplib.SMTP_SSL(server_addr, server_port)
+else:
+server = smtplib.SMTP(server_addr, server_port)
+
+if use_starttls:
+server.ehlo()
+server.starttls()
+server.ehlo()
+
+if user and password:
+server.login(user, passwd)
 
-server = smtplib.SMTP(server_addr)
 server.set_debuglevel(1)
 server.sendmail(sender, recipient, msg.as_bytes())
 server.quit()
diff --git a/conf/config.defaults b/conf/config.defaults
index 23d46b0..b69d031 100644
--- a/conf/config.defaults
+++ b/conf/config.defaults
@@ -49,6 +49,11 @@ window_length = 86400
 notify-cmd = /usr/local/bin/aurweb-notify
 sendmail =
 smtp-server = localhost
+smtp-port = 25
+smtp-use-ssl = 0
+smtp-use-starttls = 0
+smtp-user =
+smtp-password =
 sender = not...@aur.archlinux.org
 reply-to = nore...@aur.archlinux.org
 
-- 
2.25.0

[PATCH v2 1/2] Use smtplib for sending emails

[Lukas Fleischer]

Support mail delivery without a local MTA. Instead, an SMTP server can
now be configured using the smtp-server option in the [notifications]
section.

Signed-off-by: Lukas Fleischer 
---
Change the default configuration to use localhost.

 aurweb/scripts/notify.py | 13 -
 conf/config.defaults |  2 +-
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/aurweb/scripts/notify.py b/aurweb/scripts/notify.py
index b0f218b..35d2701 100755
--- a/aurweb/scripts/notify.py
+++ b/aurweb/scripts/notify.py
@@ -1,7 +1,8 @@
 #!/usr/bin/env python3
 
 import email.mime.text
-import subprocess
+import email.utils
+import smtplib
 import sys
 import textwrap
 
@@ -63,7 +64,7 @@ class Notification:
 return body.rstrip()
 
 def send(self):
-sendmail = aurweb.config.get('notifications', 'sendmail')
+server_addr = aurweb.config.get('notifications', 'smtp-server')
 sender = aurweb.config.get('notifications', 'sender')
 reply_to = aurweb.config.get('notifications', 'reply-to')
 reason = self.__class__.__name__
@@ -79,13 +80,15 @@ class Notification:
 msg['Reply-to'] = reply_to
 msg['To'] = to
 msg['X-AUR-Reason'] = reason
+msg['Date'] = email.utils.formatdate(localtime=True)
 
 for key, value in self.get_headers().items():
 msg[key] = value
 
-p = subprocess.Popen([sendmail, '-t', '-oi'],
- stdin=subprocess.PIPE)
-p.communicate(msg.as_bytes())
+server = smtplib.SMTP(server_addr)
+server.set_debuglevel(1)
+server.sendmail(sender, recipient, msg.as_bytes())
+server.quit()
 
 
 class ResetKeyNotification(Notification):
diff --git a/conf/config.defaults b/conf/config.defaults
index c519eae..e77ccf4 100644
--- a/conf/config.defaults
+++ b/conf/config.defaults
@@ -47,7 +47,7 @@ window_length = 86400
 
 [notifications]
 notify-cmd = /usr/local/bin/aurweb-notify
-sendmail = /usr/bin/sendmail
+smtp-server = localhost
 sender = not...@aur.archlinux.org
 reply-to = nore...@aur.archlinux.org
 
-- 
2.25.0

[PATCH v2 2/2] Make SMTP port and authentication configurable

[Lukas Fleischer]

Signed-off-by: Lukas Fleischer 
---
Change the default configuration to use localhost.

 aurweb/scripts/notify.py | 19 ++-
 conf/config.defaults |  5 +
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/aurweb/scripts/notify.py b/aurweb/scripts/notify.py
index 35d2701..ddf4736 100755
--- a/aurweb/scripts/notify.py
+++ b/aurweb/scripts/notify.py
@@ -65,6 +65,11 @@ class Notification:
 
 def send(self):
 server_addr = aurweb.config.get('notifications', 'smtp-server')
+server_port = aurweb.config.getint('notifications', 'smtp-port')
+use_ssl = aurweb.config.getboolean('notifications', 'smtp-use-ssl')
+use_starttls = aurweb.config.getboolean('notifications', 
'smtp-use-starttls')
+smtp_user = aurweb.config.get('notifications', 'smtp-user')
+smtp_passwd = aurweb.config.get('notifications', 'smtp-password')
 sender = aurweb.config.get('notifications', 'sender')
 reply_to = aurweb.config.get('notifications', 'reply-to')
 reason = self.__class__.__name__
@@ -85,7 +90,19 @@ class Notification:
 for key, value in self.get_headers().items():
 msg[key] = value
 
-server = smtplib.SMTP(server_addr)
+if use_ssl:
+server = smtplib.SMTP_SSL(server_addr, server_port)
+else:
+server = smtplib.SMTP(server_addr, server_port)
+
+if use_starttls:
+server.ehlo()
+server.starttls()
+server.ehlo()
+
+if user and password:
+server.login(user, passwd)
+
 server.set_debuglevel(1)
 server.sendmail(sender, recipient, msg.as_bytes())
 server.quit()
diff --git a/conf/config.defaults b/conf/config.defaults
index e77ccf4..40cad04 100644
--- a/conf/config.defaults
+++ b/conf/config.defaults
@@ -48,6 +48,11 @@ window_length = 86400
 [notifications]
 notify-cmd = /usr/local/bin/aurweb-notify
 smtp-server = localhost
+smtp-port = 25
+smtp-use-ssl = 0
+smtp-use-starttls = 0
+smtp-user =
+smtp-password =
 sender = not...@aur.archlinux.org
 reply-to = nore...@aur.archlinux.org
 
-- 
2.25.0

[PATCH] Make SMTP port and authentication configurable

[Lukas Fleischer]

Signed-off-by: Lukas Fleischer 
---
 aurweb/scripts/notify.py | 19 ++-
 conf/config.defaults |  5 +
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/aurweb/scripts/notify.py b/aurweb/scripts/notify.py
index 35d2701..ddf4736 100755
--- a/aurweb/scripts/notify.py
+++ b/aurweb/scripts/notify.py
@@ -65,6 +65,11 @@ class Notification:
 
 def send(self):
 server_addr = aurweb.config.get('notifications', 'smtp-server')
+server_port = aurweb.config.getint('notifications', 'smtp-port')
+use_ssl = aurweb.config.getboolean('notifications', 'smtp-use-ssl')
+use_starttls = aurweb.config.getboolean('notifications', 
'smtp-use-starttls')
+smtp_user = aurweb.config.get('notifications', 'smtp-user')
+smtp_passwd = aurweb.config.get('notifications', 'smtp-password')
 sender = aurweb.config.get('notifications', 'sender')
 reply_to = aurweb.config.get('notifications', 'reply-to')
 reason = self.__class__.__name__
@@ -85,7 +90,19 @@ class Notification:
 for key, value in self.get_headers().items():
 msg[key] = value
 
-server = smtplib.SMTP(server_addr)
+if use_ssl:
+server = smtplib.SMTP_SSL(server_addr, server_port)
+else:
+server = smtplib.SMTP(server_addr, server_port)
+
+if use_starttls:
+server.ehlo()
+server.starttls()
+server.ehlo()
+
+if user and password:
+server.login(user, passwd)
+
 server.set_debuglevel(1)
 server.sendmail(sender, recipient, msg.as_bytes())
 server.quit()
diff --git a/conf/config.defaults b/conf/config.defaults
index af85ce8..83820c0 100644
--- a/conf/config.defaults
+++ b/conf/config.defaults
@@ -48,6 +48,11 @@ window_length = 86400
 [notifications]
 notify-cmd = /usr/local/bin/aurweb-notify
 smtp-server = orion.archlinux.org
+smtp-port = 587
+smtp-use-ssl = 0
+smtp-use-starttls = 1
+smtp-user = aur
+smtp-password = aur
 sender = not...@aur.archlinux.org
 reply-to = nore...@aur.archlinux.org
 
-- 
2.25.0

[PATCH] Use smtplib for sending emails

[Lukas Fleischer]

Support mail delivery without a local MTA. Instead, an SMTP server can
now be configured using the smtp-server option in the [notifications]
section.

Signed-off-by: Lukas Fleischer 
---
 aurweb/scripts/notify.py | 13 -
 conf/config.defaults |  2 +-
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/aurweb/scripts/notify.py b/aurweb/scripts/notify.py
index b0f218b..35d2701 100755
--- a/aurweb/scripts/notify.py
+++ b/aurweb/scripts/notify.py
@@ -1,7 +1,8 @@
 #!/usr/bin/env python3
 
 import email.mime.text
-import subprocess
+import email.utils
+import smtplib
 import sys
 import textwrap
 
@@ -63,7 +64,7 @@ class Notification:
 return body.rstrip()
 
 def send(self):
-sendmail = aurweb.config.get('notifications', 'sendmail')
+server_addr = aurweb.config.get('notifications', 'smtp-server')
 sender = aurweb.config.get('notifications', 'sender')
 reply_to = aurweb.config.get('notifications', 'reply-to')
 reason = self.__class__.__name__
@@ -79,13 +80,15 @@ class Notification:
 msg['Reply-to'] = reply_to
 msg['To'] = to
 msg['X-AUR-Reason'] = reason
+msg['Date'] = email.utils.formatdate(localtime=True)
 
 for key, value in self.get_headers().items():
 msg[key] = value
 
-p = subprocess.Popen([sendmail, '-t', '-oi'],
- stdin=subprocess.PIPE)
-p.communicate(msg.as_bytes())
+server = smtplib.SMTP(server_addr)
+server.set_debuglevel(1)
+server.sendmail(sender, recipient, msg.as_bytes())
+server.quit()
 
 
 class ResetKeyNotification(Notification):
diff --git a/conf/config.defaults b/conf/config.defaults
index c519eae..af85ce8 100644
--- a/conf/config.defaults
+++ b/conf/config.defaults
@@ -47,7 +47,7 @@ window_length = 86400
 
 [notifications]
 notify-cmd = /usr/local/bin/aurweb-notify
-sendmail = /usr/bin/sendmail
+smtp-server = orion.archlinux.org
 sender = not...@aur.archlinux.org
 reply-to = nore...@aur.archlinux.org
 
-- 
2.25.0

Re: Automated database migrations with Alembic

[Lukas Fleischer]

Hi Frédéric,

Thanks for the proposal!

On Fri, 07 Feb 2020 at 01:51:28, Frédéric Mangano-Tarumi wrote:
> We want to be able to deploy new versions of aurweb from Ansible without
> requiring manual intervention for data migration.

That is correct. We are still working closely with the DevOps team on
migrating everything to Ansible but expect completion by the end of this
week.

> Proposed solution: Alembic
> ==
> [...]

I like the idea of using SQLAlchemy and Alembic for just the schema and
migrations as a first step. It's important that nothing in our PHP code
breaks (at least nothing that cannot be fixed with some simple code
changes). I don't see where things could potentially break but it's
better to do some testing.

Following that, I guess we could also port the existing Python code to
use SQLAlchemy? There currently seem to be ~100 database queries that we
would need to work on.

> The migrations scripts are written in Python and are intended to be used
> with SQLAlchemy, but that\u2019s not a requirement. Regardless, I do suggest
> we migrate the whole schema to SQLAlchemy because (a) it takes care of
> the SQLite/MySQL compatibility for us, (b) Alembic is able to
> automatically generate migrations from SQLAlchemy schemas[2], and (c) we
> did plan to do that eventually, didn\u2019t we?

Both (a) and (b) sound great!

There have been proposals of doing a full rewrite of aurweb using Flask
and SQLAlchemy. However, nothing is set in stone yet. I expect this
rewrite to be a long-term project with a timeline of at least 12 months,
considering that we need a *lot* of testing, both automated and manual,
while backporting new changes at the same time. Doing a partial
migration to SQLAlchemy certainly won't hurt, unless for some reason, we
decide to use a different database toolkit for the rewrite.

> Alembic supports multi-database setups but don\u2019t technically support
> migrating other data files like INI configurations, though it is
> something we might need to migrate in the future. We can still write
> custom code in the migrations, but with the risk that the files may get
> out of sync with their migration metadata in the database, which may
> happen after manual interventions. I think it\u2019s worth it.

It's likely that we need to migrate other things in the future but I
expect that to be pretty rare (you can check the text files in
upgrading/ for how often that happened in the past). As long as there is
some way of adding other types of migrations in the future, we should
focus on the database now and take care of other things as they come up.

> To convert our current SQL schema, sqlacodegen[3] can generate the
> SQLAlchemy code for us. Alembic can also create migrations to migrate
> from zero to the full database, but I suggest we keep the SQLAlchemy
> schema up-to-date in the code. Migrations would only run on existing
> database, as described in the Alembic cookbook[4].

Sounds all good to me. We should leave this discussion open for a couple
of days for others to chime in before starting with an implementation.

Lukas