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

Fix modified due date message #20388

Merged
merged 5 commits into from
Jul 19, 2022
Merged

Conversation

lhsazevedo
Copy link
Contributor

Fix #20288

Preview:
image

Copy link
Member

@delvh delvh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine.
Although we could even think about switching the key around
(modified the due date from %s to %s %s sounds more natural to me)

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jul 17, 2022
@silverwind
Copy link
Member

Agree, please swap "to" and "from" in options/locale/locale_en-US.ini and then swap the date arguments in the template.

Swap to/from in due_date_modified issue comment as it sounds more
natural.

Co-authored-by: delvh <dev.lh@web.de>
@Gusted Gusted added this to the 1.18.0 milestone Jul 18, 2022
@GiteaBot GiteaBot 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 Jul 18, 2022
@wxiaoguang
Copy link
Contributor

Maybe %[1]s %[2]s %[3]s is more clear than %s %s %s, and it's friendly for other languages.

Copy link
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this does not need a template change

@lhsazevedo
Copy link
Contributor Author

lhsazevedo commented Jul 19, 2022

I think this does not need a template change

The problem with the current template is that it passes only two arguments to Tr, and due_date_modified requires three:

Current:

{{$.locale.Tr "repo.issues.due_date_modified" (.Content | ParseDeadline) $createdStr | Safe}}
                                               ------------------------   ---------
                                                    1st arg (array)        2nd arg

Proposed fix:

{{$parsedDeadline := .Content | ParseDeadline}}
{{$.locale.Tr "repo.issues.due_date_modified" (index $parsedDeadline 0) (index $parsedDeadline 1) $createdStr | Safe}}
                                               -----------------------   -----------------------   ---------
                                                     1st argument                2nd arg            3rd arg

Note that now we're passing the array items in the original order as the from/to swap is being handled by indexed verbs in the translation format string (%[2]s %[1]s %[3]s).

@6543
Copy link
Member

6543 commented Jul 19, 2022

Passin args as befor will help translators ... tanks

@codecov-commenter
Copy link

Codecov Report

Merging #20388 (aab1020) into main (d6779c7) will increase coverage by 46.90%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           main   #20388       +/-   ##
=========================================
+ Coverage      0   46.90%   +46.90%     
=========================================
  Files         0      976      +976     
  Lines         0   135196   +135196     
=========================================
+ Hits          0    63409    +63409     
- Misses        0    64014    +64014     
- Partials      0     7773     +7773     
Impacted Files Coverage Δ
routers/web/org/home.go 61.29% <0.00%> (ø)
modules/lfs/endpoint.go 79.41% <0.00%> (ø)
modules/git/batch_reader.go 55.88% <0.00%> (ø)
services/repository/files/patch.go 0.00% <0.00%> (ø)
modules/git/commit_info_nogogit.go 59.34% <0.00%> (ø)
routers/api/packages/nuget/links.go 100.00% <0.00%> (ø)
modules/log/stack.go 78.26% <0.00%> (ø)
modules/doctor/checkOldArchives.go 22.85% <0.00%> (ø)
models/repo/wiki.go 75.00% <0.00%> (ø)
models/git/commit_status.go 52.86% <0.00%> (ø)
... and 966 more

@6543 6543 merged commit e519249 into go-gitea:main Jul 19, 2022
@lhsazevedo lhsazevedo deleted the due_date_modified branch July 19, 2022 21:40
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jul 20, 2022
* giteaofficial/main:
  [skip ci] Updated translations via Crowdin
  Dismiss prior pull reviews if done via web in review dismiss (go-gitea#20197)
  Fix modified due date message (go-gitea#20388)
  Fix public org members displayed too many informations (go-gitea#20403)
  Add two factor status to admin cmd display (go-gitea#20401)
  Use tippy.js for context popup (go-gitea#20393)
vsysoev pushed a commit to IntegraSDL/gitea that referenced this pull request Aug 10, 2022
@tyroneyeh
Copy link
Contributor

This need backport v1.17?

@silverwind
Copy link
Member

It includes a translation so it'll be more work to backport including translations, but if someone wants to do it, it should be fine to backport.

@tyroneyeh
Copy link
Contributor

Maybe just backport the template, it's easy without the locale

@zeripath
Copy link
Contributor

zeripath commented Sep 2, 2022

to my eyes the locale change is compatible with the previous locale string so I think it's fine.

Backport the whole thing.

@lhsazevedo
Copy link
Contributor Author

I think it's fine as well, as the order was swapped in the locale line itself.

Can I open a PR to the 1.17 branch?

@zeripath
Copy link
Contributor

zeripath commented Sep 2, 2022

yup, you may find the following script helpful

#!/bin/sh
PR="$1"
SHA="$2"
VERSION="$3"

if [ -z "$SHA" ]; then
    SHA=$(gh api /repos/go-gitea/gitea/pulls/$PR -q '.merge_commit_sha')
fi

if [ -z "$VERSION" ]; then
    VERSION="v1.17"
fi

echo git checkout origin/release/"$VERSION" -b backport-$PR-$VERSION
git checkout origin/release/"$VERSION" -b backport-$PR-$VERSION
git cherry-pick $SHA && git commit --amend && git push ihsazevedo backport-$PR-$VERSION && xdg-open https://github.com/go-gitea/gitea/compare/release/"$VERSION"...ihsazevedo:backport-$PR-$VERSION

lhsazevedo added a commit to lhsazevedo/gitea that referenced this pull request Sep 2, 2022
@jolheiser jolheiser added the backport/done All backports for this PR have been created label Sep 2, 2022
techknowlogick pushed a commit that referenced this pull request Sep 2, 2022
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing translations on changing due date of an issue