Closes https://codeberg.org/forgejo/forgejo/issues/6093
This PR adds support for **multi-line review comments** on pull requests, allowing reviewers to select a range of lines in diffs instead of only a single line — similar to GitHub's implementation.
### Tests for Go changes
- I added test coverage for Go changes...
- [X] in their respective `*_test.go` for unit tests.
- [X] `make pr-go` before pushing
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12582
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
First, why was this header here in the first place? Cloudflare!
Cloudflare had a optimization setting called "auto-minfy" and would
minify HTML,JS,CSS - this included removing extra whitespaces from
`<code>` elements. That's a problem because files are shown per-line
with a `<code>` element and thus results in indentation being completely
gone. Gitea added a FAQ entry for this [1], but on the same day decided
to add the workaround in Gitea, the `no-transform` header [2].
I can't find a reference of this option and some posts suggests it's
been removed. Thus it no longer serves a need to be present in Forgejo.
That wasn't my intentional motivation to remove this. This header is
also causing that HAProxy will not compress responses [3] from Forgejo
which is not ideal for Codeberg, this behavior cannot be turned off or
be worked around.
Potential risk, some other CDN or some other Cloudflare option might
still do this removal of whitespace in `<code>` HTML tags, it seems
better to disable the feature than to have Forgejo add a header which is
also causing other side-effects. I'm not aware of this another CDN of
Cloudflare option so I don't want to mark it as breaking.
[1]: https://github.com/go-gitea/gitea/pull/20430
[2]: https://github.com/go-gitea/gitea/pull/20432
[3]: https://docs.haproxy.org/3.3/configuration.html#:~:text=the%20response%20contains%20the%20%22no-transform%22%20value%20in%20the%20%22Cache-control%22%20%20%20%20%20header
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12905
Reviewed-by: Otto <otto@codeberg.org>
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
Reviewed-by: 0ko <0ko@noreply.codeberg.org>
`TestAPIAuthWithAuthorizedIntegration` has [occasionally failed](https://codeberg.org/forgejo/forgejo/actions/runs/166572/jobs/8/attempt/1#jobstep-4-2101) in the integration test steps with the unexpected output `authorized integration: parse JWT error: token is unverifiable: error while executing keyfunc: no key identified`. This indicates that the authorized integration being tested was fully validated until the JWT signature, but the signature was not accessible from the JWKS remote, which is hosted within the integration test. This doesn't seem to make sense to me.
This PR extends "no key identified" to indicate what key came from the JWT, and what keys were present in the JWKS file, so that I can see why they're not matching. This information could be generally useful in a 401 error when trying to understand authorized integration failures and doesn't pose a security risk as the keys are public information.
## Checklist
The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. All work and communication must conform to Forgejo's [AI Agreement](https://codeberg.org/forgejo/governance/src/branch/main/AIAgreement.md). There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org).
### Tests for Go changes
- I added test coverage for Go changes...
- [ ] in their respective `*_test.go` for unit tests.
- [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server.
- I ran...
- [ ] `make pr-go` before pushing
### Documentation
- [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change.
- [x] I did not document these changes and I do not expect someone else to do it.
### Release notes
- [ ] This change will be noticed by a Forgejo user or admin (feature, bug fix, performance, etc.). I suggest to include a release note for this change.
- [x] This change is not visible to a Forgejo user or admin (refactor, dependency upgrade, etc.). I think there is no need to add a release note for this change.
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12903
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
This test fails very often in CI. This should hopefully improve it.
With a downclocked CPU I've got 2 errors in 10 tries. After adding
```
await page.waitForLoadState();
```
I got no errors in 20 tries. But my testing methodology is unreliable so there's no guarantee that this helps:
* `Locator.click()` documentation says that it already waits by default for caused navigation to finish
* I added some `date` time measurement and `waitForLoadState` was clocking in about 5-15 ms, which shouldn't be causing the issue for `expect` that times out in 3000ms
...but e2e issues were `element(s) not found` on timeout, not wrong state, so my assumption is that somehow it is asserting the wrong snapshot of a page that isn't fully loaded
Additionally, I've converted _revertion_ logic to _make consistent before testing_, which is better at handling test retries in case that the _revertion_ part of the code was never ran in previous failure. In such case test retry is guaranteed to fail as well, at least when testing locally, not sure about CI.
Additionally, a completely unrelated comment fix in Playwright config.
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12871
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Thanks to forgejo/forgejo!10397 (by @voidcontext), the binary called on git hooks can now be dynamically set.
**This means that we can now run tests without needing to run `make gitea` first**! No more `Could not find gitea binary` or head-banging, when one forgets to re-compile it 🎉
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12855
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Fixes#12660, reported by @mahlzahn.
### Tests for Go changes
- I added test coverage for Go changes...
- [ ] in their respective `*_test.go` for unit tests.
- [x] in the `tests/integration` directory if it involves interactions with a live Forgejo server.
- I ran...
- [ ] <s>`make pr-go` before pushing</s> because the OOM killer kills it before it does anything useful
### Documentation
- [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change.
- [x] I did not document these changes and I do not expect someone else to do it.
### Release notes
- [x] This change will be noticed by a Forgejo user or admin (feature, bug fix, performance, etc.). I suggest to include a release note for this change.
- [ ] This change is not visible to a Forgejo user or admin (refactor, dependency upgrade, etc.). I think there is no need to add a release note for this change.
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12734
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
I removed the empty `p` elements for accessibility as mentioned in this https://codeberg.org/forgejo/forgejo/pulls/12786#issuecomment-16247117. I also adjusted the spacing for the "reference" text with the divider to have the copy button have the same spacing as other buttons.
## Testing
Navigate to an issue and observe the spacing of the time tracker, reference and dependencies sections.
<table style="width:100%">
<tr>
<td>Before</td>
<td>After</td>
</tr>
<tr>
<td><img width="250" src="/attachments/a4e14050-1d14-457c-9d4f-ec9ac1ae3bcc" /></td>
<td><img width="250" src="/attachments/3ab4445c-9f14-49d7-ac77-a7d69e914e58" /></td>
</tr>
</table>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12864
Reviewed-by: 0ko <0ko@noreply.codeberg.org>
Extracted from #12824 on suggestion of @limiting-factor; refactored after @Gusted pointed out forgejo/forgejo!12335.
Behavior change: previously a missing `hooks` folder in a repository tree (should not happen before forgejo/forgejo!12335) would return a 500 on `/api/v1/repos/%s/hooks/git`. It now returns a 200, with the same reply as an empty `hooks` folder.
Test has been added to ensure correct handling of missing `hooks` folder and of its creation if necessary.
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12833
Reviewed-by: limiting-factor <limiting-factor@noreply.codeberg.org>
Some minor changes: coherent distances between heading and descriptions, drop a divider between time tracking related sections.
### Test
1. Go to an issue, add some time tracking and potentially dependencies.
2. Observe improved distances with a user with and once with a user without write permission.
The content of the `release-notes/<pull request number>.md` file will serve as the basis for the release notes. If the file does not exist, the title of the pull request will be used instead.
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12786
Reviewed-by: 0ko <0ko@noreply.codeberg.org>
Table `package_blob` has both unique indexes, and standard indexes, defined on each of its hash columns. This PR removes the standard indexes and leaves just the unique indexes, which will reduce disk space usage and improve performance on writing to the table.
Manual testing: confirmed that the indexes are removed by inspecting the database table in my dev environment before and after the migration.
I've reviewed other database tables for similar situations where a unique index is present and non-unique indexes are present but therefore not needed. I've found no other cases.
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12836
Reviewed-by: Andreas Ahlenstorf <aahlenst@noreply.codeberg.org>
This
1. hides the “New issue” button, which currently leads to a 404 page, because it is not useful to have it, e.g., here: https://code.forgejo.org/forgejo/act/issues/169
2. removes the UI ability on projects in archived repos to move around issues and project columns. When a user performs such an action, currently it is shown as it were successful, but actually all requests lead to 404s without warning.
3. hides the hints for synching a fork or creating a pull request for recently pushed branches (which again would lead to 404s)
4. hides the branch selector (only shows the branch) on single issue/PR pages, which is disfunctional on archived repos
Thus, both these changes do not change anything related to what happens to issues or projects in archived repos, but only reduces 404 errors.
I don’t think this needs to be tested more than manually.
## Test
Create first a repository with at least **one issue** in **one project** and edit a file by creating a **new branch**. Then, perform following actions, once in the state where the repository is normal and once when it is archived:
action|unarchived|archived
---|---|---
\1. go to the issue|“New issue” button functional|“New issue” button removed
\2. click on the branch selector|menu opens|menu is now disabled and shows the branch
\3. go to the project|“New issue” button functional|“New issue” button removed
\4. try to move around columns|modifies columns|not possible anymore
\5. try to move around the issue|modifies issue location|not possible anymore
\6. go to the code view|see hint "You pushed …"|do not see this hint anymore
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12773
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
For the same reason the merge box is displayed when the user can delete the branch from which the pull request was proposed, the trust panel must be displayed when runs are waiting approval, either for information or to approve/deny runs from untrusted users.
Closesforgejo/forgejo#12576
---
Note that since this is a followup of https://codeberg.org/forgejo/forgejo/pulls/12704/files and it has not yet been released, there is no need for a mention in the release notes.
## Checklist
The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. All work and communication must conform to Forgejo's [AI Agreement](https://codeberg.org/forgejo/governance/src/branch/main/AIAgreement.md). There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org).
### Tests for Go changes
(can be removed for JavaScript changes)
- I added test coverage for Go changes...
- [ ] in their respective `*_test.go` for unit tests.
- [x] in the `tests/integration` directory if it involves interactions with a live Forgejo server.
- I ran...
- [x] `make pr-go` before pushing
### Documentation
- [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change.
- [x] I did not document these changes and I do not expect someone else to do it.
### Release notes
- [ ] This change will be noticed by a Forgejo user or admin (feature, bug fix, performance, etc.). I suggest to include a release note for this change.
- [x] This change is not visible to a Forgejo user or admin (refactor, dependency upgrade, etc.). I think there is no need to add a release note for this change.
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12785
Reviewed-by: Robert Wolff <mahlzahn@posteo.de>
Reviewed-by: Andreas Ahlenstorf <aahlenst@noreply.codeberg.org>
When the status of a user makes it implicitly trusted to run actions (for instance when it becomes a member of the Owners team of an organization), the runs that were blocked before they became trusted will need to be approved or denied.
The trust management panel was not displayed if the poster of the pull request was trusted. It is now displayed regardless of the current trust status of the user.
Closesforgejo/forgejo#12811
---
## Alternative implementation
An alternative implementation would be to approve all pending runs whenever the trust status of a user changes. That would require that change to happen when the user joins a team with permissions to run actions, becomes an admin, a collaborator to a repository with write access or when a repository ownership is transferred to an organization where the user already has such rights.
Such an implementation would save the effort of manually taking care of the runs pending approval for the now trusted user. But it would also be fragile to maintain because it would need a complete inventory of all the ways a user can become trusted. Or some kind of notification triggered whenever such an event happens, which is not currently in place.
Since this is a rare case and the manual operation is simple, I think the easiest fix consisting of showing the trust panel regardless of the trust status of the user is acceptable.
## Lingering rows in `ActionUser`
If a newly trusted user is explicitly always trusted **after** being implicitly trusted, a row is created in `ActionUser`. But the `Revoke` button will never show, because the user is now implicitly trusted. This leaves a lingering row in the `ActionUser` table. Such a row will be [removed](https://codeberg.org/forgejo/forgejo/pulls/9397/commits/e41bcf5048efaa36d19f21d67e37377e0f77c422) eventually and not clutter the table.
## Checklist
The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. All work and communication must conform to Forgejo's [AI Agreement](https://codeberg.org/forgejo/governance/src/branch/main/AIAgreement.md). There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org).
### Tests for Go changes
(can be removed for JavaScript changes)
- I added test coverage for Go changes...
- [ ] in their respective `*_test.go` for unit tests.
- [x] in the `tests/integration` directory if it involves interactions with a live Forgejo server.
- I ran...
- [x] `make pr-go` before pushing
### Documentation
- [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change.
- [x] I did not document these changes and I do not expect someone else to do it.
### Release notes
- [x] This change will be noticed by a Forgejo user or admin (feature, bug fix, performance, etc.). I suggest to include a release note for this change.
- [ ] This change is not visible to a Forgejo user or admin (refactor, dependency upgrade, etc.). I think there is no need to add a release note for this change.
*The decision if the pull request will be shown in the release notes is up to the mergers / release team.*
The content of the `release-notes/<pull request number>.md` file will serve as the basis for the release notes. If the file does not exist, the title of the pull request will be used instead.
<!--start release-notes-assistant-->
## Release notes
<!--URL:https://codeberg.org/forgejo/forgejo-->
- User Interface bug fixes
- [PR](https://codeberg.org/forgejo/forgejo/pulls/12812): <!--number 12812 --><!--line 0 --><!--description Zml4OiBzaG93IHRoZSBhY3Rpb25zIHRydXN0IG1hbmFnZW1lbnQgcGFuZWwgd2hlbiBydW5zIGZyb20gdHJ1c3RlZCB1c2VycyBhcmUgcGVuZGluZyBhcHByb3ZhbA==-->fix: show the actions trust management panel when runs from trusted users are pending approval<!--description-->
<!--end release-notes-assistant-->
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12812
Reviewed-by: Robert Wolff <mahlzahn@posteo.de>
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
