Removed all hardcoded string in DeleteHelper #3052

PavelAplevich · 2019-07-02T13:17:06Z

Hello, i did small changes for you.

Removed all hardcoded strings in DeleteHelper and refractor they ID fo better navigation in Strings.xml

Tested on my Redmi4x and Nexus One(emul) API 25 and 22. Everything works correctly.

This reverts commit 4bc55fc.

pullrequest

✅ A review job has been created and sent to the PullRequest network.

@PavelAplevich you can click here to see the review status or cancel the code review job.

tests-checker

Could you please add tests to make sure this change works as expected?

neslihanturan · 2019-07-02T13:21:36Z

Can you please rebase your branch to fix conflicts. It is a good practice to fetch and merge upstream/master before creating a new branch.

PavelAplevich · 2019-07-02T16:16:44Z

I removed the conflict in the branches.If I do something wrong, please tell me.
I have little experience using git and make mistake

neslihanturan · 2019-07-02T17:04:34Z

Thanks @PavelAplevich , git part seems fine now. But after your changes some tests fails:

fr.free.nrw.commons.delete.DeleteHelperTest > makeDeletionForNullToken FAILED
    java.lang.NullPointerException at DeleteHelperTest.kt:79
fr.free.nrw.commons.delete.DeleteHelperTest > makeDeletion FAILED
    java.lang.NullPointerException at DeleteHelperTest.kt:63
254 tests completed, 2 failed

Can you please review them?

neslihanturan

Tests should pass

PavelAplevich · 2019-07-02T18:53:24Z

@neslihanturan I will try, but I still can not understand what the problem is. I need some time

neslihanturan · 2019-07-02T19:09:48Z

Take your time @PavelAplevich , I see some possible problems on the code now, I will submit a detailed review tomorrow, it should help.

PavelAplevich · 2019-07-02T20:21:20Z

@neslihanturan I think I solved the problem.

codecov-io · 2019-07-02T21:36:25Z

Codecov Report

Merging #3052 into master will not change coverage.
The diff coverage is 35.29%.

@@          Coverage Diff           @@
##           master   #3052   +/-   ##
======================================
  Coverage    4.41%   4.41%           
======================================
  Files         259     259           
  Lines       12309   12309           
  Branches     1056    1056           
======================================
  Hits          544     544           
  Misses      11726   11726           
  Partials       39      39

Impacted Files	Coverage Δ
.../java/fr/free/nrw/commons/delete/DeleteHelper.java	`52.27% <35.29%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6fb3331...0048893. Read the comment docs.

neslihanturan

Please see my inline comment

neslihanturan · 2019-07-03T11:52:26Z

app/src/main/java/fr/free/nrw/commons/delete/DeleteHelper.java


        authCookie = sessionManager.getAuthCookie();
        mwApi.setAuthCookie(authCookie);

        Calendar calendar = Calendar.getInstance();
-        String fileDeleteString = "{{delete|reason=" + reason +


Hi @PavelAplevich , I don't think this should go to strings.xml. In string xml there shouldn't be any code (or markup language) like content. Strings are what translators will see and they should include just natural language expressions. So please revert any changes which does not fit this description.

+1 on neslihanturan's suggestion. Additionally, you might want to use string formatter to combine code like template and replaceable content for easier to read code.

pullrequest

if these strings will ever be translated please follow the pattern outlined here:
https://developer.android.com/guide/topics/resources/string-resource.html#FormattingAndStyling

Reviewed with ❤️ by PullRequest

pullrequest · 2019-07-03T22:52:05Z

app/src/main/java/fr/free/nrw/commons/delete/DeleteHelper.java

        String editToken;
        String authCookie;
-        String summary = "Nominating " + media.getFilename() + " for deletion.";
+        String summary = context.getString(R.string.delete_helper_delete_summary_1)
+                + media.getFilename() + context.getString(R.string.delete_helper_delete_summary_2);


For strings with placeholders, it would be cleaner (and safer) to use the built-in functionality described here: https://developer.android.com/guide/topics/resources/string-resource.html#FormattingAndStyling

When translating to other languages, concatenating strings this way won't always result in the desired output. This is where the filename would go in english, but grammar in other languages may put it somewhere else in the sentence. This applies to all of the below strings which you separated and concatenated.

pullrequest

With the minor suggestion improvement, the change looks good. Good job on pulling the string constants out!

Reviewed with ❤️ by PullRequest

PavelAplevich · 2019-07-05T12:33:03Z

I hope this time everything is done well)

neslihanturan

Some minors:)

neslihanturan · 2019-07-05T13:45:47Z

app/src/main/java/fr/free/nrw/commons/delete/DeleteHelper.java

@@ -71,7 +72,7 @@ public DeleteHelper(MediaWikiApi mwApi,
     * @param reason
     * @return
     */
-    private boolean delete(Media media, String reason) {
+    private boolean delete(Context context, Media media, String reason) {


I think context is not used in this method anymore, so please keep the method as it was previously

neslihanturan · 2019-07-05T13:46:51Z

app/src/main/java/fr/free/nrw/commons/delete/DeleteHelper.java

@@ -91,12 +92,12 @@ private boolean delete(Media media, String reason) {
                reason +
                " ~~~~";

-        String logPageString = "\n{{Commons:Deletion requests/" + media.getFilename() +
+        String logPageString = "\n" + "{{Commons:Deletion requests" + media.getFilename() +


I guess you forgot removing "+" from your previous implementation. This method shouldn't be edited at all if I don't miss any point.

neslihanturan · 2019-07-05T13:49:22Z

app/src/main/java/fr/free/nrw/commons/delete/DeleteHelper.java

        } else {
-            title += ": Failed";
-            message = "Could not request deletion.";
+            title += context.getString(R.string.delete_helper_show_deletion_notification_title_else);


Ideally this string should be just "Failed" and operation shuld be ": "+[string]

neslihanturan · 2019-07-05T13:51:01Z

app/src/main/res/values/strings.xml

+  <string name="delete_helper_ask_reason_and_execute_reason_copyright_internet_photo">Random photo from internet</string>
+  <string name="delete_helper_ask_reason_and_execute_reason_copyright_logo">Logo</string>
+  <string name="delete_helper_ask_reason_and_execute_reason_copyright_other">Other</string>
+  <string name="delete_helper_ask_reason_and_execute_alert_set_positive_button">Ok</string>


I am sure we have Ok and Cancel strings in our file already. Please do not add them again, it is duplicate.

neslihanturan · 2019-07-05T13:51:38Z

app/src/main/res/values/strings.xml

+  <string name="delete_helper_ask_reason_and_execute_reason_spam_nonsense">Nonsense</string>
+  <string name="delete_helper_ask_reason_and_execute_reason_spam_other">Other</string>
+  <string name="delete_helper_ask_reason_and_execute_reason_copyright_press_photo">Press photo</string>
+  <string name="delete_helper_ask_reason_and_execute_reason_copyright_internet_photo">Random photo from internet</string>


Previous string naming seemed better to me. This ones are too long

PavelAplevich · 2019-07-05T14:52:17Z

"decision problem with color of info icon #2940"
I wanted to do it in another pullrequest(

PavelAplevich

I create copy icon with right color, because drawabletint dont work with API lower 23

PavelAplevich · 2019-07-05T15:16:42Z

I dont undestand, how i can delete my comments and how i can comment what i want. In "Correction DeleteHelper and string.xml" I correcting hardcoded string. In "decision problem with color of info icon #2940" I tried to fix issue #2940 . But i do it in this pullrequest(

I found #2660 issue. Think, i solved this issue

pullrequest

✅ A review job has been created and sent to the PullRequest network.

@PavelAplevich you can click here to see the review status or cancel the code review job.

neslihanturan

Some minors

neslihanturan · 2019-07-05T20:04:57Z

app/src/main/res/drawable/ic_info_outline_blue_24dp.xml

@@ -0,0 +1,9 @@
+<vector xmlns:android="http://schemas.android.com/apk/res/android"


Why this file is edited?

neslihanturan · 2019-07-05T20:07:46Z

app/src/main/res/values/strings.xml

+  <string name="delete_helper_delete_summary_2">for deletion.</string>
+  <string name="delete_helper_show_deletion_title">Nominating for Deletion</string>
+  <string name="delete_helper_show_deletion_title_if">Success</string>
+  <string name="delete_helper_show_deletion_message_if_1">Successfully nominated</string>


This string naming is not clear. Additionally, you can insert variables into one string instead of using 2 strings. See the example at line 561:
%1$s is uploaded by: %2$s

neslihanturan · 2019-07-05T20:08:06Z

app/src/main/res/values/strings.xml

+  <string name="delete_helper_delete_summary_1">Nominating</string>
+  <string name="delete_helper_delete_summary_2">for deletion.</string>
+  <string name="delete_helper_show_deletion_title">Nominating for Deletion</string>
+  <string name="delete_helper_show_deletion_title_if">Success</string>


This variable naming is nothing about success

neslihanturan · 2019-07-05T20:08:21Z

app/src/main/res/layout/activity_review.xml

@@ -25,7 +25,7 @@
              android:layout_gravity="center_horizontal"
              android:paddingLeft="12dp"
              android:paddingRight="12dp"
-              android:drawableEnd="@drawable/ic_info_outline_24dp"
+              android:drawableEnd="@drawable/ic_info_outline_blue_24dp"


This file shouldn't be edited

neslihanturan · 2019-07-05T20:09:50Z

app/src/main/java/fr/free/nrw/commons/delete/DeleteHelper.java

-            title += ": Failed";
-            message = "Could not request deletion.";
+            title += ": " + context.getString(R.string.delete_helper_show_deletion_title_else);
+            message = context.getString(R.string.delete_helper_show_deletion_message_else) + ".";


This dot is part of natural language, so can be added string itself.

neslihanturan · 2019-07-05T20:10:03Z

app/src/main/java/fr/free/nrw/commons/delete/DeleteHelper.java

+            title += ": " + context.getString(R.string.delete_helper_show_deletion_title_if);
+            message = context.getString(R.string.delete_helper_show_deletion_message_if_1)+ " "
+                    + media.getDisplayTitle() + " "
+                    + context.getString(R.string.delete_helper_show_deletion_message_if_2) + ".";


dots can be added to string.

neslihanturan · 2019-07-05T20:10:24Z

app/src/main/java/fr/free/nrw/commons/delete/DeleteHelper.java

                "}}\n";
        SimpleDateFormat sdf = new SimpleDateFormat("yyyy/MM/dd", Locale.getDefault());
        String date = sdf.format(calendar.getTime());

-        String userPageString = "\n{{subst:idw|" + media.getFilename() +
+        String userPageString = "\n" + "{{subst:idw|" + media.getFilename() +


This liine shouldn't be edited.

neslihanturan · 2019-07-05T20:10:40Z

app/src/main/java/fr/free/nrw/commons/delete/DeleteHelper.java

@@ -91,12 +92,12 @@ private boolean delete(Media media, String reason) {
                reason +
                " ~~~~";

-        String logPageString = "\n{{Commons:Deletion requests/" + media.getFilename() +
+        String logPageString = "\n{{Commons:Deletion requests" + media.getFilename() +


This line shouldn't be edited.

neslihanturan · 2019-07-05T20:11:38Z

app/src/main/java/fr/free/nrw/commons/delete/DeleteHelper.java

@@ -59,7 +59,8 @@ public DeleteHelper(MediaWikiApi mwApi,
     * @return
     */
    public Single<Boolean> makeDeletion(Context context, Media media, String reason) {
-        viewUtil.showShortToast(context, "Trying to nominate " + media.getDisplayTitle() + " for deletion");
+        viewUtil.showShortToast(context, context.getString(R.string.delete_helper_make_deletion_toast_1)


Please use only one string here, and get media.getDisplayTitle() as a variable into it. For more info see my comment at strings.xml file.

PavelAplevich · 2019-07-06T01:33:13Z

@neslihanturan I dont undestand why i dont see problems in this files in my commit. Only added 1 file and change 1 line. Or #2940 already solved?

PavelAplevich · 2019-07-06T01:54:00Z

@neslihanturan Accept my apologies for my carelessness. I hope this time my changes in DeleteHelper and string.xml right

This reverts commit fe4ea52.

neslihanturan · 2019-07-06T11:41:13Z

app/src/main/res/drawable/ic_info_outline_blue_24dp.xml

@@ -0,0 +1,9 @@
+<vector xmlns:android="http://schemas.android.com/apk/res/android"


neslihanturan · 2019-07-06T11:43:07Z

Sorry @PavelAplevich but some of the irrelevant changes are still there. I noted it to the file. You can always click on files changed tab and see differences your PR made. Other parts seems correct to me, thanks for your fast responses and cooperation:)

This reverts commit 5895cc8.

PavelAplevich · 2019-07-06T14:29:21Z

I revert my commit. I think we can do #2940 issue in another pull request.
Everything should be right now.
I am very pleased to work with you :)

neslihanturan

Thanks @PavelAplevich !

PavelAplevich added 3 commits July 1, 2019 17:24

Removed hardcoded strings in DeleteHelper

4bc55fc

Revert "Removed hardcoded strings in DeleteHelper"

4176646

This reverts commit 4bc55fc.

Removed all hardcoded strings in DeleteHelper.

5e1a8f9

pullrequest bot reviewed Jul 2, 2019

View reviewed changes

tests-checker bot suggested changes Jul 2, 2019

View reviewed changes

Merge branch 'master' into master

b4fab6c

neslihanturan requested changes Jul 2, 2019

View reviewed changes

PavelAplevich added 2 commits July 2, 2019 23:17

fixed test problems

bd89cac

Merge remote-tracking branch 'origin/master'

f2ca935

neslihanturan requested changes Jul 3, 2019

View reviewed changes

pullrequest bot reviewed Jul 3, 2019

View reviewed changes

pullrequest bot reviewed Jul 4, 2019

View reviewed changes

Correction strings in DeleteHelper

5b67f87

neslihanturan requested changes Jul 5, 2019

View reviewed changes

decision problem with color of info icon #2940

5895cc8

Correction DeleteHelper and string.xml

9a8caa3

PavelAplevich commented Jul 5, 2019

View reviewed changes

PavelAplevich closed this Jul 5, 2019

PavelAplevich reopened this Jul 5, 2019

pullrequest bot reviewed Jul 5, 2019

View reviewed changes

minor fix

c78bb13

neslihanturan requested changes Jul 5, 2019

View reviewed changes

fixed problems in DeleteHelper.java and strings.xml

aae16d3

and another little fix

fe4ea52

Revert "and another little fix"

669619a

This reverts commit fe4ea52.

neslihanturan requested changes Jul 6, 2019

View reviewed changes

Revert "decision problem with color of info icon #2940"

0048893

This reverts commit 5895cc8.

neslihanturan approved these changes Jul 6, 2019

View reviewed changes

neslihanturan merged commit 108e28c into commons-app:master Jul 6, 2019

		@@ -0,0 +1,9 @@
		<vector xmlns:android="http://schemas.android.com/apk/res/android"

Removed all hardcoded string in DeleteHelper #3052

Removed all hardcoded string in DeleteHelper #3052

Conversation

PavelAplevich commented Jul 2, 2019

pullrequest bot left a comment

Choose a reason for hiding this comment

tests-checker bot left a comment

Choose a reason for hiding this comment

neslihanturan commented Jul 2, 2019

PavelAplevich commented Jul 2, 2019

neslihanturan commented Jul 2, 2019

neslihanturan left a comment

Choose a reason for hiding this comment

PavelAplevich commented Jul 2, 2019

neslihanturan commented Jul 2, 2019

PavelAplevich commented Jul 2, 2019

codecov-io commented Jul 2, 2019 • edited Loading

Codecov Report

neslihanturan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pullrequest bot Jul 3, 2019

Choose a reason for hiding this comment

pullrequest bot Jul 4, 2019

Choose a reason for hiding this comment

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot Jul 3, 2019

Choose a reason for hiding this comment

pullrequest bot left a comment

Choose a reason for hiding this comment

PavelAplevich commented Jul 5, 2019

neslihanturan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PavelAplevich commented Jul 5, 2019

PavelAplevich left a comment

Choose a reason for hiding this comment

PavelAplevich commented Jul 5, 2019 • edited Loading

pullrequest bot left a comment

Choose a reason for hiding this comment

neslihanturan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PavelAplevich commented Jul 6, 2019 • edited Loading

PavelAplevich commented Jul 6, 2019 • edited Loading

Choose a reason for hiding this comment

neslihanturan commented Jul 6, 2019

PavelAplevich commented Jul 6, 2019 • edited Loading

neslihanturan left a comment

Choose a reason for hiding this comment

codecov-io commented Jul 2, 2019 •

edited

Loading

PavelAplevich commented Jul 5, 2019 •

edited

Loading

PavelAplevich commented Jul 6, 2019 •

edited

Loading

PavelAplevich commented Jul 6, 2019 •

edited

Loading

PavelAplevich commented Jul 6, 2019 •

edited

Loading