Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pull request review/approval and comment on code #3748

Merged
merged 104 commits into from
Aug 6, 2018

Conversation

lafriks
Copy link
Member

@lafriks lafriks commented Apr 2, 2018

This is merge of my started work on PR approval feature and @lunny comment on code.

Will obsolete #2583 and resolve #124 and #3580

Screenshot:
attels

Work needs to be done:

  • Implement UI for code review and comments
  • Implement database changes and needed handlers
  • Review display in comment section
  • Add tests

EDIT by @JonasFranzDEV :
TODOs and other stuff from the merged PR lafriks/gitea#2:

This comment will summarize all progress and updates.

Reviews

Reviews should bundle all code comments created for the initial review. All code comments created for this review are linked to the review model.

Comment types

CommentTypeCode: A comment at a line of a code of a PR
CommentTypeReview: Notifies the participants about the review

TODO

  • Render code comments on the right place
  • Add support for comments on both sides
  • Show code comments in the "conversation" after a review got submitted
  • Show single code comment in the "conversation" (won't be implemented, because it would make the comment list bloated. GitHub only shows single comments in the diff view too)
  • Remove dummy data from "code comment form" (line, path, side)
  • Add support for "Add single comments"
  • Order/Separate code comments by thread/review (not implemented, see Order code comments by review #4389)
    • At the moment only the first comment for a line gets shown in the conversation. No replies!
  • Blocked by Add line blame git#116
  • Edit comments
  • Delete comments
  • Show actions / notifications for review comments
  • Show actions / notifications for review (without comments) (not implemented, see Send notifications until review gets submitted #4388)
  • Fix some problems with invalidating comments written for the previous code
  • Improve UI
    • Show comments only on one side (split view)
    • "Cancel" button functionality
    • Show add comment button only at PR diff views
    • Show forms only to signed in users
    • Preview tab is broken
    • Add icon is shown at hunk line

Preview(s)

comments_and_review
review comments
comment at conversation detail

EDIT END

@lafriks lafriks added type/feature Completely new functionality. Can only be merged if feature freeze is not active. pr/wip This PR is not ready for review labels Apr 2, 2018
@lafriks lafriks added this to the 1.5.0 milestone Apr 2, 2018
@lafriks lafriks changed the title Pull request review/approve and comment on code Pull request review/approval and comment on code Apr 2, 2018
@sapk
Copy link
Member

sapk commented Apr 3, 2018

Maybe we could try to store it in git-appraise in place of database ?
You seem to have work on it in #733 and it iwill also obsolete it.

@bkcsoft bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 3, 2018
@lafriks
Copy link
Member Author

lafriks commented Apr 3, 2018

@sapk that won't be that easy. That was my first idea when starting to work on git-appraise but the problem is how to integrate it with gitea forks & PR's architecture. For now everything will be stored in database and later I will see how to get it all working together also with git-appraise.

@jonasfranz
Copy link
Member

What is the difference between "Add single comment" and "Add comment"?

@lafriks
Copy link
Member Author

lafriks commented Apr 3, 2018

@JonasFranzDEV it will not show both buttons. Add single comment will be visible only when no review is started and will add comment without starting new review. When you have already started review (added at least one code comment with Start review only buttons Cancel and Add comment will be visible

@lunny
Copy link
Member

lunny commented Apr 3, 2018

All the code review buttons are not available.

@thehowl thehowl mentioned this pull request Apr 22, 2018
@lunny lunny mentioned this pull request May 4, 2018
2 tasks
jonasfranz added 2 commits May 6, 2018 23:06
Add IssueComment types

Signed-off-by: Jonas Franz <info@jonasfranz.software>

(cherry picked from commit 2b4daab)
Signed-off-by: Jonas Franz <info@jonasfranz.software>
Signed-off-by: Jonas Franz <info@jonasfranz.software>
@lunny lunny mentioned this pull request May 8, 2018
20 tasks
Add ReviewID to findComments

Signed-off-by: Jonas Franz <info@jonasfranz.software>
Add migration for review
Other small changes

Signed-off-by: Jonas Franz <info@jonasfranz.software>
Signed-off-by: Jonas Franz <info@jonasfranz.software>
Signed-off-by: Jonas Franz <info@jonasfranz.software>
Signed-off-by: Jonas Franz <info@jonasfranz.software>
Signed-off-by: Jonas Franz <info@jonasfranz.software>
Signed-off-by: Jonas Franz <info@jonasfranz.software>
Show Review comments on comment stream

Signed-off-by: Jonas Franz <info@jonasfranz.software>
@jonasfranz
Copy link
Member

I'm currently trying to contribute to this PR by creating a PR at @lafriks repo. I already did some progress there. Feel free to check it out: https://github.com/lafriks/gitea/pull/2

Showing buttons in context

Signed-off-by: Jonas Franz <info@jonasfranz.software>
Signed-off-by: Jonas Franz <info@jonasfranz.software>
Signed-off-by: Jonas Franz <info@jonasfranz.software>
Signed-off-by: Jonas Franz <info@jonasfranz.software>
@jonasfranz
Copy link
Member

@appleboy resolved

jonasfranz and others added 2 commits August 5, 2018 15:32
Signed-off-by: Jonas Franz <info@jonasfranz.software>
@bkcsoft bkcsoft added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Aug 6, 2018
@jonasfranz jonasfranz merged commit 6e64f9d into go-gitea:master Aug 6, 2018
@adelowo
Copy link
Member

adelowo commented Aug 6, 2018

🥇 🥇

@Caballerog
Copy link

Is in the actual release?

Docker pull in 3, 2, 1 ...

Thanks.

@chupasaurus
Copy link

@Caballerog this feature will be released in next (1.6) release.

@ptman
Copy link
Contributor

ptman commented Aug 6, 2018

@chupasaurus has 1.5.0 been released yet?

@chupasaurus
Copy link

@ptman Nope, not yet. Check this link to see how Gitea release cycle looks like.

@mkienenb
Copy link

mkienenb commented Aug 6, 2018

@Caballerog If you are impatient to try it out but don't want to build it from source, you can grab it from here the master release link [1] and install it over your existing binary as described [2] in the "Updating to a new version" section.

[1]
https://dl.gitea.io/gitea/master/

[2]
https://docs.gitea.io/en-us/install-from-binary/

That's what I did.

Thank you everyone else for making this happen!

@lafriks lafriks deleted the feat/approval branch August 6, 2018 15:55
@sondr3
Copy link
Contributor

sondr3 commented Aug 6, 2018

@chupasaurus Gitea always takes a bit longer than predicted to release, partially I think because they accept too many features each release. This has been discussed before in #2283 and probably lots of other places as well 😃

@lunny
Copy link
Member

lunny commented Aug 7, 2018

@sondr3 I think there are two main reasons. One is what you said, another is all of the maintainers are part-time for the project.

@sondr3
Copy link
Contributor

sondr3 commented Aug 7, 2018

That too, I do not envy you the task of maintaining a project of this size, I have one with a magnitude less users and that was still stressful and took a lot of time. Don’t worry about it, it’s a minor quibble.

aswild added a commit to aswild/gitea that referenced this pull request Oct 24, 2018
Prepare for wild/v1.6 branch

* BREAKING
  * Respect email privacy option in user search via API (go-gitea#4512)
  * Simply remove tidb and deps (go-gitea#3993)
  * Swagger.v1.json template (go-gitea#3572)
* FEATURE
  * Pull request review/approval and comment on code (go-gitea#3748)
  * Added dependencies for issues (go-gitea#2196) (go-gitea#2531)
  * Add the ability to have built in themes in Gitea and provide dark theme arc-green (go-gitea#4198)
  * Add sudo functionality to the API (go-gitea#4809)
  * Add oauth providers via cli (go-gitea#4591)
  * Disable merging a WIP Pull request (go-gitea#4529)
  * Force user to change password (go-gitea#4489)
  * Add letsencrypt to Gitea (go-gitea#4189)
  * Add push webhook support for mirrored repositories (go-gitea#4127)
  * Add csv file render support defaultly (go-gitea#4105)
  * Add Recaptcha functionality to Gitea (go-gitea#4044)
* BUGFIXES
  * Fix release creation via API (go-gitea#5076)
  * Remove links from topics in edit mode  (go-gitea#5026)
  * Fix missing AppSubUrl in few more templates (fixup) (go-gitea#5021)
  * Fix missing AppSubUrl in some templates (go-gitea#5020)
  * Hide outdated comments in file view (go-gitea#5017)
  * Upgrade gopkg.in/testfixtures.v2 (go-gitea#4999)
  * Disable debug routes unless PPROF is enabled in configuration (go-gitea#4995)
  * Fix user menu item styling (go-gitea#4985)
  * Fix layout of the topics editing form (go-gitea#4971)
  * Fix null pointer dereference in ParseCommitWithSignature (go-gitea#4962)
  * Fix url in discord webhook (go-gitea#4953)
  * Detect charset and convert non UTF-8 files for display (go-gitea#4950)
  * Make sure to catch the right error so it is displayed on the UI (go-gitea#4945)
  * Fix(topics): don't redirect to explore page. (go-gitea#4938)
  * Fix bug forget to remove Stopwatch when remove repository (go-gitea#4928)
  * Fix bug when repo remained bare if multiple branches pushed in single push (go-gitea#4923)
  * Fix: Let's Encrypt configuration settings (go-gitea#4911)
  * Fix: Crippled diff (go-gitea#4726) (go-gitea#4900)
  * Fix trimming of markup section names (go-gitea#4863)
  * Issues api allow pulls and fix go-gitea#4832 (go-gitea#4852)
  * Do not autocreate directory for new users/orgs (go-gitea#4828) (go-gitea#4849)
  * Fix redirect with non-ascii branch names (go-gitea#4764) (go-gitea#4810)
  * Fix missing release title in webhook (go-gitea#4783) (go-gitea#4796)
  * User shouldn't be able to approve or reject his/her own PR (go-gitea#4729)
  * Make sure to reset commit count in the cache on mirror syncing (go-gitea#4720)
  * Fixed bug where team with admin privelege type doesn't get any unit  (go-gitea#4719)
  * Fix incorrect caption of webhook setting (go-gitea#4701) (go-gitea#4717)
  * Allow WIP marker to contains < or > (go-gitea#4709)
  * Hide org/create menu item in Dashboard if user has no rights (go-gitea#4678) (go-gitea#4680)
  * Site admin could create repos even MAX_CREATION_LIMIT=0 (go-gitea#4645)
  * Fix custom templates being ignored (go-gitea#4638)
  * Fix starring icon after semantic ui update (go-gitea#4628)
  * Fix Split-View line adjustment (go-gitea#4622)
  * Fix integer constant overflows in tests (go-gitea#4616)
  * Push whitelist now doesn't apply to branch deletion (go-gitea#4601) (go-gitea#4607)
  * Fix bugs when too many IN variables (go-gitea#4594)
  * Fix failure on creating pull request with assignees (go-gitea#4419) (go-gitea#4583)
  * Fix panic issue on update avatar email (go-gitea#4580) (go-gitea#4581)
  * Fix status code label for a successful webhook (go-gitea#4540)
  * An inactive user shouldn't be able to be added as a collaborator (go-gitea#4535)
  * Don't fail silently if trying to add a collaborator twice (go-gitea#4533)
  * Fix incorrect MergeWhitelistTeamIDs check in CanUserMerge function (go-gitea#4519) (go-gitea#4525)
  * Fix out-of-transaction query in removeOrgUser (go-gitea#4521) (go-gitea#4522)
  * Fix migration from older releases (go-gitea#4495)
  * Accept 'Data:' in commit graph (go-gitea#4487)
  * Update xorm to latest version and fix correct `user` table referencing in sql (go-gitea#4473)
  * Relative URLs for LibreJS page (go-gitea#4460)
  * Redirect to correct page after using scratch token (go-gitea#4458)
  * Fix column droping for MSSQL that need new transaction for that (go-gitea#4440)
  * Replace src with raw to fix image paths (go-gitea#4377)
  * Add default merge options when creating new repository (go-gitea#4369)
  * Fix docker build (go-gitea#4358)
  * Fixes repo membership check in API (go-gitea#4341)
  * Dep upgrade mysql lib (go-gitea#4161)
  * Fix some issues with special chars in branch names (go-gitea#3767)
  * Responsive design fixes (go-gitea#4508)
* ENHANCEMENT
  * Fix milestones sorted wrongly (go-gitea#4987)
  * Allow api to create tags for releases if they don't exist (go-gitea#4890)
  * Fix go-gitea#4877 to follow the OpenID Connect Audiences spec (go-gitea#4878)
  * Enforce token on api routes [fixed critical security issue go-gitea#4357] (go-gitea#4840)
  * Update legacy branch and tag URLs in dashboard to new format (go-gitea#4812)
  * Slack webhook channel name cannot be empty or just contain an hashtag (go-gitea#4786)
  * Add whitespace handling to PR-comparsion (go-gitea#4683)
  * Make reverse proxy auth optional (go-gitea#4643)
  * MySQL TLS (go-gitea#4642)
  * Make sure to set PR split view when creating/previewing a pull request  (go-gitea#4617)
  * Log user in after a successful sign up (go-gitea#4615)
  * Fix typo IsPullReuqestBroken -> IsPullRequestBroken (go-gitea#4578)
  * Allow admin toggle forcing a password change for newly created users (go-gitea#4563)
  * Update jQuery to v1.12.4 (go-gitea#4551)
  * Env var GITEA_PUSHER_EMAIL (go-gitea#4516)
  * Feat(repo): support search repository by topic name (go-gitea#4505)
  * Small improvements to dependency UI (go-gitea#4503)
  * Make max commits in graph configurable (go-gitea#4498)
  * Add valid for lfs oid (go-gitea#4461)
  * Add shortcut to save wiki page (go-gitea#4452)
  * Allow administrator to create repository for any organization (go-gitea#4368)
  * Fix repository last updated time update when delete a user who watched the repo (go-gitea#4363)
  * Switch plaintext scratch tokens to use hash instead (go-gitea#4331)
  * Increase default TOTP secret size to 320 bits (go-gitea#4287)
  * Keep preseeded database password (go-gitea#4284)
  * Implemented hover text showing user FullName (go-gitea#4261)
  * Add ability to delete a token (go-gitea#4235)
  * Fix typos in i18n variable names. (go-gitea#4080)
  * Api: repos/search: add parameters to control the sort order (go-gitea#3964)
  * Add missing path in the Docker app.ini template (go-gitea#2181)
  * Add file name and branch to page title (go-gitea#4902)
  * Offline use of google fonts (go-gitea#4872)
  * Add missing History link to directory listings v2 (go-gitea#4829)
  * Locale for Edit and Remove due date issue (go-gitea#4802)
  * Disable 'May Import Local Repository' when is disabled by setting (Is… (go-gitea#4780)
  * API /admin/users/{username} missing parameter (go-gitea#4775)
  * Display error when adding a user to a team twice (go-gitea#4746)
  * Remove UsePrivilegeSeparation from the Docker sshd_config, see go-gitea#2876 (go-gitea#4722)
  * Focus title input when clicking helper link (go-gitea#4696)
  * Add vendor to user reserved words and format words list according alphabet (go-gitea#4685)
  * Add gitea/issues link to 500 page (go-gitea#4654)
  * Hide home button when landing page is not set to home (go-gitea#4651)
  * Remove link to GitHub issues in 404 template (go-gitea#4639)
  * Cmd/serve: pprof cpu and memory profile dumps to disk (go-gitea#4560)
  * Add flash message after an account has been successfully activated (go-gitea#4510)
  * Prevent html entity escaping on delete branch (go-gitea#4471)
  * Locale for button Edit on protected branch (go-gitea#4442)
  * Update notification icon (go-gitea#4343)
  * Added front-end topics validation (go-gitea#4316)
  * Don't display buttons if there are no system notifications (go-gitea#4280)
  * Issue due date api (go-gitea#3890)
* SECURITY
  * Improve URL validation for external wiki  and external issues (go-gitea#4710)
  * Make cookies HttpOnly and obey COOKIE_SECURE flag (go-gitea#4706)
  * Don't disclose emails of all users when sending out emails (go-gitea#4664)
  * Check that repositories can only be migrated to own user or organizations (go-gitea#4366)
* TRANSLATION
  * Fix punctuation in English translation (go-gitea#4958)
  * Fix translation (go-gitea#4355)
@lafriks lafriks added the type/changelog Adds the changelog for a new Gitea version label Nov 10, 2018
@olekukonko
Copy link

when is the ETA for this functionality?

@lafriks
Copy link
Member Author

lafriks commented Apr 4, 2019

It is already available for quite some time now. Starting with 1.6.0 version

@olekukonko
Copy link

Thanks for the prompt response.

@frankgerhardt
Copy link

Nice feature. I have tried both "Request Change" and "Approve" but really got lost as a reviewer when there were newer commits by the developers. Older comments where shown as out of date but for me as reviewer still valid. I could not see how to track all comments and progress on cleaning them up. Is there any documentation about the workflow? Any blog posts or anything else to understand how this shall be used?

@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
@delvh delvh removed the type/changelog Adds the changelog for a new Gitea version label Oct 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Commits and pull files