upload: Enable File Upload for users. #1414

Subhasish-Behera · 2023-07-04T17:49:50Z

What does this PR do, and why?

External discussion & connections

Discussed in #zulip-terminal in File Upload #T529 #T1414
Fully fixes Uploading/Attaching Files #529
Partially fixes issue #
Builds upon previous unmerged work in PR #
Is a follow-up to work in PR #
Requires merge of PR #
Merge will enable work on #

How did you test this?

Manually - Behavioral changes
Manually - Visual changes
Adapting existing automated tests
Adding automated tests for new behavior (or missing tests)
Existing automated tests should already cover this (only a refactor of tested code)

Self-review checklist for each commit

It is a minimal coherent idea
It has a commit summary following the documented style (title & body)
It has a commit summary describing the motivation and reasoning for the change
It individually passes linting and tests
It contains test additions for any new behavior
It flows clearly from a previous branch commit, and/or prepares for the next commit

Visual changes

mounilKshah

Thank you for working on this @Subhasish-Behera
I ran the code from the PR on my local machine. I was able to view the link generated when a file is uploaded by entering the path in the popup.

Further steps can be manipulating the write_box such that the generated link gets attached in the message editor.

zulipterminal/core.py

zulipterminal/ui_tools/views.py

zulipterminal/core.py

mounilKshah

This version of the PR is much closer to the bare bones approach which I had mentioned on the stream earlier. The commit structure looks fine to me. I ran this on my local machine, and it worked fine when the extra / was not present in the appended URL.

Also, I think it was a good idea to change the position of the cursor after the URL is appended to the compose box content.

mounilKshah · 2023-07-26T01:03:02Z

zulipterminal/ui_tools/boxes.py

+        edit_widget = self.contents[self.FOCUS_CONTAINER_MESSAGE][
+            self.FOCUS_MESSAGE_BOX_BODY
+        ]
+        edit_widget.edit_text += f"[{file_name}](/{str(uri)})"


Upon sending the message and opening the file, the URL wasn't able to fetch the file. This may be because of the extra / before the uri, because the URI returned from the server already has a / at the start.

Also, the function already has the type of uri as str, so there might be no need to typecast it to string.

The file upload works now, but the str conversion remains - is there a reason for this?

zulipterminal/model.py

zulipterminal/ui_tools/views.py

neiljp

@Subhasish-Behera As per Mounil's review, this works with a very minor change to the file prefix 👍

Other than Mounil's and my inline comments, and ensuring the link is generated correctly, the next steps would be to add some tests for commits, in particular those handling the logic. That should help with thinking about the path entered and what the server expects and provides back to the application.

zulipterminal/config/keys.py

neiljp · 2023-07-26T02:21:59Z

zulipterminal/model.py

+    def get_file_upld_uri(self, file_loctn: str) -> str:
+        with open(file_loctn, "rb") as fp:
+            result = self.client.upload_file(fp)
+        return result["uri"]


Please use full words for function/variable names - we're not short of space here :)

I know we need to be more consistent here, but also note that while this method does return the URI, it doesn't only get the URI but also uploads the file first.

neiljp · 2023-07-26T02:32:09Z

zulipterminal/ui_tools/boxes.py

@@ -710,6 +710,14 @@ def autocomplete_emojis(

        return emoji_typeahead, emojis

+    def insert_uri(self, file_name: str, uri: str) -> None:


The following may feel minor, but is useful in the same way as naming things well :)

It would be useful to ensure that at calling sites users know what the parameters to a function are - if only inserting a URI, why are there two parameters? :)

eg. I could call this as insert_uri(x,y), and it wouldn't be clear. I could try and name x and y clearly at the call site, but that doesn't guarantee that they are meaningful.

For a few parameters you could do something like action_xthing_with_ything(x,y) and the order can help, but otherwise it's useful to use * to require a named parameter. For example:

def action_xthing(xthing, *, ything): ... action_xthing(x, ything=y)

I expect you've iterated on what this method does, but it's good to reflect the behavior in the name, eg. perhaps now this should be append_*?

Makes sense.

zulipterminal/ui_tools/views.py

neiljp · 2023-07-26T02:54:41Z

zulipterminal/ui_tools/views.py

+        self.write_box.insert_uri(file_name, self.uri)
+
+    def keypress(self, size: urwid_Size, key: str) -> str:
+        if is_command_key("FILE_UPLOAD", key):


This is the first commit where you use this string, and adding it makes it available in the UI, so it would be good to have that commit just before this one.

The commits haven't been reordered for this yet. Do you understand what I was requesting?

zulipterminal/core.py

zulipterminal/ui_tools/views.py

mounilKshah

Ran this code on my local machine and manually did a quick test. It works as per expectation 👍

I think it is good to start with writing tests for this PR.

neiljp · 2023-08-11T23:53:04Z

I left feedback in the stream, so removing needs-review flag, unless there are specific concerns?

get_file_upld_uri takes a file location as an argument and returns the URI.

insert_uri method takes the file_name and the uri generated for that file as arguments. The added text is in the format [<file_name>](/<uri>). At last the cursor is shited after the end of the added text.

FileUploadView's base class is PopUpView. On keypress of `FILE_UPLOAD` it passes the value of the edit_text to _handle_file_upload function. The __init__ have write_box as a parameter which is used to insert the URI in the write_box.The PopUpView is closed on 'ESC' and the upload is done via 'ENTER'.

show_file_upload_view in core.py creates an instance of FileUploadView when called. It is called by `ctrl + o`keypress on WriteBox.

If a passed file location is invalid then the get_file_upload_uri returns None and using that the error and reported.

Test cases of different file formats.Firstly a initial edit text is set which is appended by calling append_uri_and_filename.

mounilKshah

Thank you for implementing all the feedback and writing the tests. I tested the code on my local machine and it worked fine. I even tried uploading multiple files in the same message and that too worked without a glitch.
The tests seem fine to me. I think in the next iteration, apart from the minor suggestion, the commit structure can have the code block commits squashed together with their respective tests.

mounilKshah · 2023-09-11T00:59:28Z

tests/ui_tools/test_boxes.py

+        write_box.msg_write_box.edit_text = initial_edit_text
+
+        write_box.append_uri_and_filename(file_name, uri)
+        result_edit_text = write_box.msg_write_box.edit_text


Defining a variable can be avoided here as the value is being used only once.

mounilKshah · 2023-09-11T01:12:11Z

tests/model/test_model.py

+        self, mocker, model, file_name, upload_result, expected_result
+    ):
+        self.client.upload_file = mocker.Mock(return_value=upload_result)
+        with tempfile.TemporaryDirectory() as temp_dir:


Good thinking to use this.

In principle I agree, and using something like this for testing the file existence is certainly a valid approach. One of the downsides is that it involves filesystem actions in each test.

We actually use a similar approach for testing some of run.py right now, using some built-in fixtures that pytest provides. If we take this approach, it would likely be wise to use one of those.

An alternative approach would be to patch open - though that can be tricky since it is built-in - and set it to raise certain exceptions.

neiljp

@Subhasish-Behera Thanks for the updates and the tests 👍

This does work well, so I've focused more on the details here.

I agree with Mounil that squashing the appropriate test commits with the matching code would make this clearer, and likely also the two commits which build slightly on each other to wrap the upload.

Once your commits are squashed, I'd recommend checking their titles and any summary text, since many have not been updated while you've been working on the PR.

I'm looking forward to having this merged! :)

neiljp · 2023-09-19T00:36:42Z

zulipterminal/model.py

+        if os.path.exists(file_location):
+            with open(file_location, "rb") as fp:


Note that os.path.exists only checks that this is a valid path, not that it is a file or that it can be accessed by the active user.

There is also the slim possibility that we check it exists and then the file disappears before it is opened. The better pattern is therefore to try to open, and then handle any exceptions relevant to not being able to open the file, or the parent exception class of those we expect, and return None from there.

However, this function now returns None if there is a problem uploading, as well as if the file doesn't exist - the calling function (and user!) cannot distinguish between these cases. In the short term using display_error_if_present for reporting any server error could enable a distinction there - though note that after reporting the error it would still return None, so trigger the same error handling in the caller.

For now we should certainly use a try/except around the open to look for file issues via exception(s). You could experiment with display_error_if_present, but duplicate error messages may look strange.

At a later point it may be clearer to handle only the uploading of an already-opened source file here, and handle other errors where this function is called, making the showing of different errors much simpler - including those arising from different exceptions when opening a proposed upload.

Rereading this, it would definitely be better to use the exception approach here, and focus error handling for upload as occurring from the model, compared to error handling with accessing the file from the calling function.

It's possible there could be errors related to processing the upload related to the file handling, but that would be part of the upload process.

neiljp · 2023-09-19T00:38:25Z

zulipterminal/ui_tools/boxes.py

@@ -710,6 +710,14 @@ def autocomplete_emojis(

        return emoji_typeahead, emojis

+    def append_uri_and_filename(self, file_name: str, uri: str) -> None:


Nit: The name of the function suggests that the first argument is the uri and the second the file_name.

will change the order of the arguments.

neiljp · 2023-09-19T00:48:12Z

zulipterminal/ui_tools/views.py

+            file_name = file_path.name
+            self.write_box.append_uri_and_filename(file_name, self.uri)
+        else:
+            self.controller.report_error(["ERROR: Unable to get the URI"])


This error is triggered if the file to upload is the empty string, ie. just pressing enter with no contents in the box. As a user I'd expect that to be equivalent to exiting the box - so it doesn't even try and upload.

That would fit with doing more error handling in this location, as per another comment, though this would be easier to do without changing the way the function is called.

neiljp · 2023-09-19T00:55:14Z

zulipterminal/ui_tools/boxes.py

+        edit_widget = self.contents[self.FOCUS_CONTAINER_MESSAGE][
+            self.FOCUS_MESSAGE_BOX_BODY
+        ]
+        edit_widget.edit_text += f"[{file_name}](/{str(uri)})"


The file upload works now, but the str conversion remains - is there a reason for this?

neiljp · 2023-09-19T00:58:34Z

zulipterminal/ui_tools/views.py

+        self.write_box.insert_uri(file_name, self.uri)
+
+    def keypress(self, size: urwid_Size, key: str) -> str:
+        if is_command_key("FILE_UPLOAD", key):


The commits haven't been reordered for this yet. Do you understand what I was requesting?

neiljp · 2023-09-19T02:45:55Z

tests/ui_tools/test_popups.py

+                file_name, self.file_upload_view.uri
+            )
+        else:
+            self.controller.append_uri_and_filename.assert_not_called()


This doesn't exist? It likely still passes due to the perils of Mock without spec.

Got it. will remove it.

If you do wish to assert on the mocked details, then my point here was more that the object you're asserting on does not exist - and is different to that a few lines above.

neiljp · 2023-09-19T03:03:57Z

tests/ui_tools/test_popups.py

+
+        self.controller.model.get_file_upload_uri.assert_called_once_with(file_location)
+        if not expected_error_message:
+            file_name = Path(file_location).name


This file_name would be good as a test (case) parameter. It may look like a duplicate, but you're processing the result explicitly here instead, and it's clearer as a parameter to the test.

Cool. I think you are suggesting this

def test__handle_file_upload( self, file_location: str, file_name: str, expected_uri: str, expected_error_message: Optional[str], ) -> None:

and directly use the file_name parameter.

Right, the principle is that ideally a test is as simple as possible - less logic and calculation - and the test cases handle the input and corresponding expected output (expected_* parameters).

neiljp · 2023-09-19T03:17:55Z

tests/model/test_model.py

+            ),
+            case(
+                "existing_file.txt",
+                {"result": "failure", "error_message": "Upload failed"},


This matches to the code we have (result is not success), but I'm unsure it's the actual form that we would get from the API in the case of an error, which we should be using - at a bare minimum for the value of result. Does it currently match?

neiljp · 2023-09-19T03:19:49Z

tests/model/test_model.py

+        self, mocker, model, file_name, upload_result, expected_result
+    ):
+        self.client.upload_file = mocker.Mock(return_value=upload_result)
+        with tempfile.TemporaryDirectory() as temp_dir:


In principle I agree, and using something like this for testing the file existence is certainly a valid approach. One of the downsides is that it involves filesystem actions in each test.

We actually use a similar approach for testing some of run.py right now, using some built-in fixtures that pytest provides. If we take this approach, it would likely be wise to use one of those.

An alternative approach would be to patch open - though that can be tricky since it is built-in - and set it to raise certain exceptions.

neiljp · 2023-09-19T06:21:54Z

tests/model/test_model.py

+        "file_name, upload_result, expected_result",
+        [
+            case(
+                "existing_file.txt",


I don't see that we gain from using different file_name strings, when this variable is passed in as it is each time. Instead I'd suggest using a fixed string - if we continue to set a filename to create - and then using an alternative parameter here which may have a different path. That would allow using the same branch of the conditional each time, and using this new parameter to select the file that is generated or not.

I probably thought along the lines of testing the code for different file types like pdf and txt.

I think you are suggesting that file_name doesn't help in testing that.I will change as you said.
But should ZT-Client's code test if different file formats work?

I agree that this illustrates that various file names should be acceptable to the function. However, how many would we include to show that? Also, file extensions do not necessarily correspond to file types - and I suspect the client/server accepts arbitrary files anyhow? So, if we want to demonstrate that, we could include a fixed string in the test - or as a default value to a test parameter - which looks fairly random.

In case it needs clarifying, using a mock for client.upload means that only ZT code sees this file in any case - it doesn't touch the actual Zulip.Client, and of course not any server.

I mentioned using a path parameter combined with a fixed file_name, since you're not testing eg. ./file_name, path_to/file_name or similar vs just file_name. The file_name could be a constant, with the path varying in the test cases.

Firstly the TestWriteBox is imported as unlike other popups FileUploadView takes a argument write_box. A fixture write_box is created which creates a instance of TestWriteBox and returns its write_box fixture. There are two cases of keypress test- one for keys which dosent make the popup exit and the other for key that exits the popup. There are 4 testcases for _handle_file_upload. The tests are based on if the return of get_file_upload_uri is None or a valid uri. On that basis the errors/append_func are called or not is asserted.

There are three tests based on upload result and existence of file. tempfile is used to create and write file in case of existing files and in csase of non-existing files a invalid temp_file_path is created.

zulipbot · 2024-04-17T23:34:53Z

Heads up @Subhasish-Behera, we just merged some commits that conflict with the changes you made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

zulipbot added the size: L [Automatic label added by zulipbot] label Jul 4, 2023

Subhasish-Behera added the PR needs mentor review label Jul 11, 2023

mounilKshah reviewed Jul 13, 2023

View reviewed changes

mounilKshah added enhancement New feature or request PR needs review PR requires feedback to proceed area: event handling How events from the server are responded to area: message compose and removed PR needs mentor review labels Jul 16, 2023

Subhasish-Behera force-pushed the 2023-06-14-file-upload branch 2 times, most recently from 8d355d6 to d9df231 Compare July 25, 2023 22:49

Subhasish-Behera added the PR needs mentor review label Jul 25, 2023

mounilKshah suggested changes Jul 26, 2023

View reviewed changes

mounilKshah added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs mentor review labels Jul 26, 2023

neiljp reviewed Jul 26, 2023

View reviewed changes

neiljp removed the PR needs review PR requires feedback to proceed label Jul 26, 2023

Subhasish-Behera force-pushed the 2023-06-14-file-upload branch 2 times, most recently from 32abc79 to 6b8e464 Compare August 7, 2023 03:34

Subhasish-Behera added PR needs mentor review PR needs review PR requires feedback to proceed labels Aug 7, 2023

mounilKshah reviewed Aug 9, 2023

View reviewed changes

mounilKshah removed the PR needs mentor review label Aug 9, 2023

neiljp removed the PR needs review PR requires feedback to proceed label Aug 11, 2023

Subhasish-Behera force-pushed the 2023-06-14-file-upload branch 4 times, most recently from ff683ca to 2d76e2b Compare September 3, 2023 13:22

zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Sep 6, 2023

Subhasish-Behera force-pushed the 2023-06-14-file-upload branch from c84a30a to 6f11bed Compare September 6, 2023 04:38

Subhasish-Behera added 7 commits September 6, 2023 11:17

keys: Add hotkey ctrl + o for File Upload.

d62aa99

model: Add function to upload file.

5927ed8

get_file_upld_uri takes a file location as an argument and returns the URI.

boxes: Add insert_uri method for WriteBox.

94c863b

insert_uri method takes the file_name and the uri generated for that file as arguments. The added text is in the format [<file_name>](/<uri>). At last the cursor is shited after the end of the added text.

core/boxes: Logic for initial keypress on FILE_UPLOAD

6cd4f6b

show_file_upload_view in core.py creates an instance of FileUploadView when called. It is called by `ctrl + o`keypress on WriteBox.

model/views: Handle Invalid File Location and other possible errors.

0119c8d

If a passed file location is invalid then the get_file_upload_uri returns None and using that the error and reported.

test_boxes: test for append_uri_and_filename.

b9a618d

Test cases of different file formats.Firstly a initial edit text is set which is appended by calling append_uri_and_filename.

Subhasish-Behera force-pushed the 2023-06-14-file-upload branch from 6f11bed to d6d8b90 Compare September 6, 2023 06:13

Subhasish-Behera added PR needs review PR requires feedback to proceed PR needs mentor review labels Sep 7, 2023

mounilKshah reviewed Sep 11, 2023

View reviewed changes

mounilKshah removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback PR needs mentor review labels Sep 11, 2023

neiljp reviewed Sep 19, 2023

View reviewed changes

neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Sep 19, 2023

Subhasish-Behera added 2 commits September 24, 2023 04:43

test_model: Tests for get_file_upload_uri.

5d1b7e4

There are three tests based on upload result and existence of file. tempfile is used to create and write file in case of existing files and in csase of non-existing files a invalid temp_file_path is created.

Subhasish-Behera force-pushed the 2023-06-14-file-upload branch from d6d8b90 to 5d1b7e4 Compare October 15, 2023 08:28

zulipbot added the has conflicts label Apr 17, 2024

neiljp added missing feature: user A missing feature for all users, present in another Zulip client and removed enhancement New feature or request labels Apr 22, 2024

		@@ -710,6 +710,14 @@ def autocomplete_emojis(

		return emoji_typeahead, emojis

		def insert_uri(self, file_name: str, uri: str) -> None:

		if os.path.exists(file_location):
		with open(file_location, "rb") as fp:

		@@ -710,6 +710,14 @@ def autocomplete_emojis(

		return emoji_typeahead, emojis

		def append_uri_and_filename(self, file_name: str, uri: str) -> None:

upload: Enable File Upload for users. #1414

Are you sure you want to change the base?

upload: Enable File Upload for users. #1414

Conversation

Subhasish-Behera commented Jul 4, 2023 • edited by neiljp

What does this PR do, and why?

External discussion & connections

How did you test this?

Self-review checklist for each commit

Visual changes

mounilKshah left a comment • edited

Choose a reason for hiding this comment

mounilKshah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neiljp 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

mounilKshah left a comment

Choose a reason for hiding this comment

neiljp commented Aug 11, 2023

mounilKshah 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

neiljp 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

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

zulipbot commented Apr 17, 2024

Subhasish-Behera commented Jul 4, 2023 •

edited by neiljp

mounilKshah left a comment •

edited