Kallithea issues archive

I'm very glad that we are not the only one with this problem. @heyleke has proposed several patches in the past to cover this scenario, but they have not been merged so far. I am planning to rework these patches but I have few time to spare.

The solution looks as follows: inline comments are saved as drafts on the server whenever they are made. No mail is sent at that time. When the user commits the comments by using the form at the bottom of the changeset/pull request, the draft flag is set to False turning them into real comments. At that moment one notification mail is sent with the accumulation of all comments on that changeset / pull request.

Some of the discussion points from the past:

• the e-mail is now just a list of all comments, without context, and with more than one comment this is not ideal. So, e-mail content should be improved, e.g. adding context lines with each comment
• there is still quite some code duplication between comment handling of changeset / pull request. Some improvements have already been made in this area, but I identified more cleanup possibilities.
• how to make sure that the user does not forget that there are draft comments waiting to be committed? This is a UI problem.

You can find the solution of @heyleke in his repo: https://bitbucket.org/heyleke/kallithea/commits/all (the top two commits are PoC/WIP and is not what we are actually running).

I recently started rebasing these patches but it is not yet finished; the form handling for comments has changed quite a bit since the original patches were made. I pushed the current state at https://bitbucket.org/patrickdepinguin/kallithea/commits/488d62047ab61960917e6384639b42e330380665 Note that the top commit is still broken -- the UI is not working correctly now.

I should clarify that I am not planning on submitting exactly that rebased version to the Kallithea project; the intention of the rebase is to get a functional version of what we have today -- but on top of a recent Kallithea version (we are now stuck on an old version).

Separate from the rebase, I will work on the items mentioned above, and then reintegrate the patches into something that can be accepted.

FYI I have a celery beat pull request in the pipeline, which would allow to send forgotten comments once an hour, once a day or something like that.

I would like to help, i.e. submitting PRs, but I can't figure out the state of work and the planned developments.

Ping me if needed.

Let me try to clarify: the patches that I describe are being used in our Kallithea instance at work. Because they are based on an older Kallithea version, and newer versions of Kallithea have had changes in the comment area that make a rebase non-trivial, we are stuck at that older version. On the other hand, these changes were previously proposed upstream but rejected in this form, so work is needed to make them upstreamable. So, my plan is two-fold:

Path A: rebase the existing changes to current Kallithea. Main motivation is that this would deblock our own instance at work, i.e. we can upgrade to a newer version of Kallithea while preserving this bulk comment feature we are used to.

Path B: rework and upstreaming of the changes. Plan in more detail: 1. remove existing duplication, e.g. controllers/changeset.py and controllers/pullrequest.py have very similar but still somewhat different comment handling, see 'def comment()'. I already had a look and I think this can be merged. Similar line up between pullrequest and changeset was done in the past in the 'templates' directory. 2. extend email templates with context, i.e. let each comment be surrounded by some source lines, similar to the view in the web UI. This will make emails that contain more than one comment still understandable (in our current patches the different comments are one after the other). 3. with that preparation done, apply a reworked version of our patches.

As my first focus is path A, you are more than welcome to already start part B, in particular points 1 and 2. I could try to guide you further if you are open to that.

I have brought path A to something that seems to work now. It needs to have more complete testing, though. Your help is definitely welcome here. Please check https://bitbucket.org/patrickdepinguin/kallithea/commits/73aea01cac3fca1c6deec0723225e50b48c48a07 and its ancestors.

Ping @heyleke

I pushed some small updates, check my repo at https://bitbucket.org/patrickdepinguin/kallithea/commits/f1ad7d7a68d01af980cd0d5709a8cc34c8cb7810 and ancestors. Changes:

• rename 'Save' on general comments to 'Save and commit all draft comments' (plus associated logic changes to cope with this)
• change textual indication of drafts in comment block

@patrickdepinguin I finally started to have a look at your rebase, gearbox + kallithea up and running, hopefully I'll have some feedback soon

@patrickdepinguin from functional point of view, all seem fine? No more preview on trunk kallithea, so that makes the gui logic a bit simpler. Maybe, we need to have a look how to add the edit button again, interesting for the 'drafts', now there is only the delete option?

@heyleke Thanks for looking into it. I would consider the 'Edit' button a new 'feature' and thus postpone it to path B. I think this would need some extra backend code, to edit comments that have already added to the database as draft.

Hello, is this feature still being considered? Could it be integrated as an optional, experimental feature that we could enable through a flag in the configuration file?

Yes, this is still being considered.

However, at this moment there is no active development on it. Over recent times we have been working hard to finalize 0.4.0 and 0.4.1 which are now released. Next major goal is to finish SSH support as 0.5.0.

I could send out the patches that I use in my work instance. They are basically what I sent earlier but rebased. These changes were not accepted as they are and need to be reworked. They have also some problems, like the comment count also taking into account draft comments.

I would highly appreciate the rebased patch, thanks i am afraid to do the merging myself and in our case we can wait with the update until it is integrated, SSH is not that important for our workflows.

Hi @Thomas De Schampheleire I’ve tried to rebase the changes myself from the repository you’ve linked in the official repository from Kallithea over the 0.4.1 version but turns out there are more changes than I can handle safely. Could I maybe bother you again to provide the rebased patch? Thanks.

@Sebastian G Sorry, this slipped my attention. I did a rebase just now (was indeed not trivial), please see this revision and its ancestors.

https://bitbucket.org/patrickdepinguin/kallithea/commits/c0a3733504a3286ef52d6a9cae241dd1b93e814e

(see overview of commits here: https://bitbucket.org/patrickdepinguin/kallithea/commits/ )

I only did basic testing locally, this rebased version is not yet in production. Please let me know in case there are issues.

Note the ordering of the alembic versions in the ‘db’ commit: in our database we already have the draft column, so I added the alembic revision that renames the hooks on top of that (e.g. updating revision_down). In your case, you may want to modify this commit to let the draft column be added on top of the hook rename one, if you already have that. Just update the revisions in the alembic files.