Private link #1978

davigonz · 2017-06-01T13:12:34Z

Implements #1970
~~Requires owncloud/android-library#165~~

davigonz · 2017-06-01T13:16:21Z

@davivel , @jesmrec , ready for code review and QA

davivel · 2017-06-02T08:00:48Z

res/layout/share_file_layout.xml

@@ -50,6 +50,7 @@
                android:layout_marginRight="@dimen/standard_half_margin"
                android:layout_marginStart="4dp"
                android:layout_toEndOf="@+id/shareFileIcon"
+                android:layout_toLeftOf="@+id/getPrivateLinkButton"


Please, add also android:layout_toStartOf="@id/getPrivateLinkButton" for better support of RTL languages.

davivel · 2017-06-02T08:02:21Z

src/com/owncloud/android/ui/activity/ShareActivity.java

 import android.app.SearchManager;
+import android.content.Context;


Can this be removed?

davivel · 2017-06-02T08:02:46Z

src/com/owncloud/android/ui/adapter/SharePublicLinkListAdapter.java

@@ -29,6 +29,7 @@
 import android.widget.TextView;

 import com.owncloud.android.R;
+import com.owncloud.android.datamodel.OCFile;


Can this be removed?

davivel · 2017-06-02T08:03:53Z

src/com/owncloud/android/ui/fragment/ShareFileFragment.java

+            @Override
+            public boolean onLongClick(View view) {
+                // Show a toast message explaining what a private link is
+                Toast.makeText(getActivity(), R.string.private_link_info, Toast.LENGTH_LONG).show();


Nice idea! 👍

davivel · 2017-06-02T08:04:30Z

src/com/owncloud/android/ui/fragment/ShareFragmentListener.java

@@ -21,20 +21,26 @@

 package com.owncloud.android.ui.fragment;

+import android.accounts.Account;
+import android.content.Context;


Please, remove the imports that are not really used.

davivel · 2017-06-02T08:06:18Z

src/com/owncloud/android/ui/helpers/FileOperationsHelper.java

+     * @param file @param file {@link OCFile} which will be shared with internal users
+     * @param account
+     */
+    public void copyOrSendPrivateLink(Context context, Account account, OCFile file) {


There is no need to pass a context to the method. FileOperationsHelper has a field mFileActivity that can be used instead.

davivel · 2017-06-02T08:08:21Z

src/com/owncloud/android/ui/helpers/FileOperationsHelper.java

+
+            String fileId = Integer.valueOf(parsedRemoteId).toString();
+
+            Uri uri = Uri.parse(com.owncloud.android.lib.common.accounts.


Here first a String is parsed to Uri, then the Uri is serialized to String immediately. Why is it? Needs to be normalized?

Yeah, you're right. Parsing the String to Uri is not needed, I can use directly the link obtained in AccountUtils.getUrlForFile to share it later.

davivel · 2017-06-02T08:13:24Z

src/com/owncloud/android/ui/helpers/FileOperationsHelper.java

+        try {
+
+            // Parse remoteId
+            String remoteId = file.getRemoteId();


All this block would be better together in a method getPermalink(), or getPrivateLink(); might be in OCFile, and receive the base URL to the server as a parameter. Take into account that the manipulation of the remote Id is quite specific of private link right now, and the responsibility of this method is not generating the permalink itself, but that of displaying the menu of apps capable of handling the permalink.

davivel · 2017-06-02T08:14:17Z

src/com/owncloud/android/ui/helpers/FileOperationsHelper.java

+            shareLink(link);
+
+        } catch (com.owncloud.android.lib.common.accounts.AccountUtils.AccountNotFoundException e) {
+            e.printStackTrace();


I'd rather to use Log_OC.e(..., e), it prints the stack trace too.

jesmrec · 2017-06-02T08:59:44Z

Approved.

…app itself

jesmrec · 2017-06-05T09:21:08Z

Lot of changes in the code review. @davivel @davigonz will be needed a new QA review?

davigonz · 2017-06-05T11:45:57Z

@jesmrec All the changes are related to code refactoring, I've tested it and is working properly but you can test it again to be sure.

davivel · 2017-06-05T12:04:11Z

src/com/owncloud/android/datamodel/OCFile.java

+    public String getPrivateLink(Context context, Account account) {
+
+        // Parse remoteId
+        String parsedRemoteId = mRemoteId.substring(0, Math.min(mRemoteId.length(), 8));


Sorry, I don't understand what conversion are you doing here to get the string for the private link

The point here is that I get the complete remoteId from the server but I only need the first 8 characters.

Example:

remoteId from server: 00000003ocr2n5bhxjuy

00000003 => file id

ocr2n5bhxjuy => server instance ID

BTW, Math.min(mRemoteId.length() is not required, I will delete it

davivel · 2017-06-05T12:06:13Z

src/com/owncloud/android/datamodel/OCFile.java

+            if (baseUrl == null)
+                throw new AccountUtils.AccountNotFoundException(account, "Account not found", null);
+
+            privateLink = baseUrl + AccountUtils.INDEX_PATH + FILES_PATH + "/" + fileId;


I'd rather to have a single constant, fully meaningful by itself; something like PRIVATE_LINK_PATH = /index.php/f/" .

I had separated it into two different paths just in case we needed to use /index.php in the future, similarly to STATUS_PATH constant.

davigonz · 2017-06-06T08:33:23Z

@davivel @jesmrec , finished all the requested changes. Ready for approving and a second round of QA

jesmrec · 2017-06-06T10:37:59Z

@davigonz QA process was reviewed again succesfully.

…library

davivel · 2017-06-07T14:49:30Z

You did it

davigonz requested a review from davivel June 1, 2017 13:15

jesmrec mentioned this pull request Jun 2, 2017

Permalink owncloud/QA#452

Closed

davivel suggested changes Jun 2, 2017

View reviewed changes

jesmrec added the Approved by qa label Jun 2, 2017

davigonz added 7 commits June 5, 2017 09:47

Added icon to get permalink

af76485

Added copyOrSendPrivateLink method [WIP]

9635e70

Build url for file and show message with info about the private link

57e8142

New method in library for creating url for file

cf5e2a4

Minor fix while parsing file remote id

edb9a03

Delete unneded imports and refactor code

acb85ed

Log error properly

a3e2240

davigonz force-pushed the permalink branch from 71f3a5a to a3e2240 Compare June 5, 2017 07:47

Move method for creating a file private link from the library to the …

ed496a2

…app itself

davigonz changed the title ~~Permalink~~ Private link Jun 5, 2017

davivel suggested changes Jun 5, 2017

View reviewed changes

davigonz added 3 commits June 5, 2017 15:14

Delete Math.min(mRemoteId.length() from remote id conversion

23a5682

Add comments to explain the file id parsing

3938483

Update comment

a745263

davigonz added 2 commits June 6, 2017 18:32

Added new constant for private links

94973fa

Fix pointer to library branch, permalink doesn't need changes in the …

89b6001

…library

davivel approved these changes Jun 7, 2017

View reviewed changes

davivel merged commit 7b0046c into master Jun 7, 2017

davivel deleted the permalink branch June 7, 2017 14:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Private link #1978

Private link #1978

davigonz commented Jun 1, 2017 •

edited by davivel

Loading

davigonz commented Jun 1, 2017

davivel Jun 2, 2017

davivel Jun 2, 2017

davivel Jun 2, 2017

davivel Jun 2, 2017

davivel Jun 2, 2017

davivel Jun 2, 2017

davivel Jun 2, 2017

davigonz Jun 5, 2017

davivel Jun 2, 2017

davivel Jun 2, 2017

jesmrec commented Jun 2, 2017

jesmrec commented Jun 5, 2017

davigonz commented Jun 5, 2017

davivel Jun 5, 2017 •

edited by davigonz

Loading

davigonz Jun 5, 2017

davivel Jun 5, 2017 •

edited

Loading

davigonz Jun 5, 2017 •

edited

Loading

davigonz commented Jun 6, 2017

jesmrec commented Jun 6, 2017

davivel commented Jun 7, 2017

		import android.app.SearchManager;
		import android.content.Context;


		String fileId = Integer.valueOf(parsedRemoteId).toString();

		Uri uri = Uri.parse(com.owncloud.android.lib.common.accounts.

Private link #1978

Private link #1978

Conversation

davigonz commented Jun 1, 2017 • edited by davivel Loading

davigonz commented Jun 1, 2017

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

jesmrec commented Jun 2, 2017

jesmrec commented Jun 5, 2017

davigonz commented Jun 5, 2017

davivel Jun 5, 2017 • edited by davigonz Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davivel Jun 5, 2017 • edited Loading

Choose a reason for hiding this comment

davigonz Jun 5, 2017 • edited Loading

Choose a reason for hiding this comment

davigonz commented Jun 6, 2017

jesmrec commented Jun 6, 2017

davivel commented Jun 7, 2017

davigonz commented Jun 1, 2017 •

edited by davivel

Loading

davivel Jun 5, 2017 •

edited by davigonz

Loading

davivel Jun 5, 2017 •

edited

Loading

davigonz Jun 5, 2017 •

edited

Loading