Fix issues with duplicated file names in the same directory

[JammingBen]

Description

Lots of frontend actions use the filename as identifier. This works fine for the normal filelist, as the filename is unique per directory. However, in other views like "Shared by link" for instance, it's totally possible that we have multiple files with the same name in one directory. Is this the case, many of these actions will fail or result in strange behavior (show detail view, rename, delete, show sharing dialogue, ...)

This PR fixes those issues by taking the currently clicked row into account.

Related Issue

• Fixes https://github.com/owncloud/enterprise/issues/4412

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Database schema changes (next release will require increase of minor version instead of patch)
• Breaking change (fix or feature that would cause existing functionality to change)
• Technical debt
• Tests only (no source changes)

Checklist:

• Code changes
• Unit tests added
• Acceptance tests added
• Documentation ticket raised:
• Changelog item, see TEMPLATE

[update-docs]

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

[JammingBen]

@jvillafanez Although this is not yet complete, could you please have a look at the overall approach?

To be honest, I don't like it really much myself. It would be cleaner to replace the filename in these actions with the fileId or even the fileInfoModel. But I guess this would break things for other apps... So I decided to additionally add the fileId param, which is not required though. This will at least (or worst...) keep things as they were before.

[jvillafanez]

If it's just a visualization problem, I think it's better to focus on that. Just show the path or other information that could help to distinguish the files. I'm assuming that the actions are properly executed against the right files.

At this point, it seems too late to try to introduce the usage of the fileids. It would be better to start from scratch rather than try to force the usage of fileids.

[JammingBen]

If it's just a visualization problem, I think it's better to focus on that. Just show the path or other information that could help to distinguish the files. I'm assuming that the actions are properly executed against the right files.

That's right, we are just talking about visualization issues. But it's not only about showing something to distinguish such files in the UI. We also need to distinguish them in our code and functionality. As a result I don't think it is that easy to solve.

The method that finds most of these elements is findFileEl (https://github.com/owncloud/core/blob/master/apps/files/js/filelist.js#L931). It returns all trs matching the data-file attribute (= file name). We could make this attribute unique, by adding the fileId or path for instance. But this attribute is also used in many places to get the real filename. Thus changing it will result in other issues.

Another solution would be to add a new attribute, let's say unique-file-name. But then we would have to change the findFileEl method to look for this new attribute instead. And adjust all the places where it gets called. Also, this would probably require the fileId again...

[jvillafanez]

Does it affect only to link shares? If so, maybe it's better to overwrite the methods in the share list, or maybe just make the changes needed there. At least, I think the code changes would be more isolated.

[JammingBen]

Unfortunately this affects all of the following lists:

• Favorites
• Tags
• Shared with you
• Shared with others
• Shared by link

... with pretty much every action: showing details, showing share context, renaming, deleting, ...

I'm really wondering why this error was just noticed now. It seems like a pretty common issue to me.

[jvillafanez]

Question: does the problem come only from the findFileEl? Is it possible to select the right element taking into account the click position (assuming the rest of the code works without problem)?

[JammingBen]

I did some more research on this.

does the problem come only from the findFileEl?

Mainly, yes. If this method would work with a unique parameter instead of the name, we would solve most of the problems. However, there might still be some tiny issues where we need to adjust. For instance https://github.com/owncloud/core/blob/master/apps/files/js/filelist.js#L456 -> _currentFileModel is returned when it is set and when the filename matches with the clicked element. Same problem, the name is not enough, we need something unique here.

Is it possible to select the right element taking into account the click position (assuming the rest of the code works without problem)?

I didn't find a way to get the clicked row yet. We are in the context of the filelist, so we don't now the currently clicked file. There is _currentFileModel, but it is set after the call to findFileEl.

I tried to implement another click event which listens to any click on a row. But it does not work for elements with existing event listeners that stop further events from being executed.

I'm currently checking for other ways to get the clicked file.

[phil-davis]

I remember noticing these sort of issues in the past. With the development phoenix web we stopped doing "routine" expansion of the existing webUI to cover all these cases - as listed above there are quite a few views that can easily have many files with the same name listed. I expect that there are cases where the user will have difficulty actually selecting the file that they want from multiple similar-looking files in the list.

We can easily expand the UI test coverage if needed.

[JammingBen]

I'm afraid there is no real elegant solution to the problem. Maybe this is still the best way to go:

I tried to implement another click event which listens to any click on a row. But it does not work for elements with existing event listeners that stop further events from being executed.

What that means in detail:

• Add a new property currentClickedRow to the filelist class
• Implement an event listener that sets this property
• Add event emitters for this event whenever a row is clicked
• Adjust the findFileEl method to take currentClickedRow into account (only if it is set)

We can easily expand the UI test coverage if needed.

I think OC wont drop the current UI in the foreseeable future, so this would be great indeed!

[jvillafanez]

Yes, that's mostly the idea I had. Still ugly, but hopefully the changes aren't so spread.

[JammingBen]

@phil-davis There is already an acceptance test that addresses this topic, at least partly: https://github.com/owncloud/core/pull/38415/files#diff-2f00159b8d264fe88306f30276f42542327bb49b4324c4daa88e8045ba4d270eR32

The "correct" behavior was even described before, but commented out.

Overall I guess this is ready to review. There is still an annoying behavior in such scenarios though: When having 3 folders named "folder" in the file list and clicking on the last one, it will be rendered again at the top of the 3 folders. This is because Backbone re-renders elements when they are updated (via set()). There is a parameter silent: true to overcome this, however, I don't think we should use it. This way we would sacrifice the functionality to refresh updated elements "on the fly".

[phil-davis]

@JammingBen I added a commit that deletes the bug-demo scenario and enables the "gold" scenario for oC10.

For oC10 bugs, we usually write a bug-demo scenario in a separate feature file, that demonstrates the "bad" behavior, and passes in the oC10 with the bug. We also write a "gold" scenario that demonstrates how the scenario should work. The "gold" scenario is skipped on oC10.

When the bug is fixed, we can delete the bug-demo scenario and remove the skip from the "gold" scenario.

[phil-davis]

LGTM - needs a dev review from someone who knows the detail of the JavaScript

[jvillafanez]

You can use context.fileList.$fileList._setCurrentRow(...) here without triggering an event.

[JammingBen]

You're right. Well, then we could remove all that event stuff and just work with direct method calls, right?!

[jvillafanez]

While I'd expect something more straightforward such as _setCurrentRow($rowEl), if you want to use an object as parameter, that object should be documented. I mean, the docs doesn't say anything about how the data is expected to be, so we have to rely on reading the function or checking how the function is being used.

You can add something like:

Following attributes must be present in the "data" parameter:
* currentRow: the jQuery element associated to the "selected" row of the filelist.

Optional attributes could also be added later if needed.

[JammingBen]

I just used data because this method was planned to be called via event emitter only. Now if we call that method directly (and it has no other purpose than setting the clicked row-element), I also prefer something like $rowEl 👍

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information