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

[full-ci] Reload file list after accepting a remote share #6942

Merged
merged 5 commits into from
May 20, 2022

Conversation

JammingBen
Copy link
Contributor

@JammingBen JammingBen commented May 11, 2022

Description

We've fixed a bug where the file list would not reload after accepting a remote share.

Related Issue

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

@JammingBen JammingBen self-assigned this May 11, 2022
@ownclouders
Copy link
Contributor

ownclouders commented May 11, 2022

Results for oC10iPhoneNotifications https://drone.owncloud.com/owncloud/web/25654/48/1
💥 The acceptance tests pipeline failed. The build has been cancelled.

@JammingBen
Copy link
Contributor Author

I'm not sure about the best solution here. I've now implemented it to always reload the file list when accepting a federated share. If we only want to reload when necessary, it gets a bit more complicated:

Especially the latter would require some more logic. Logic, that shouldn't exist in the web runtime. Hence we would need to write something in the files app.

@kulmann Any opinions here?

@JammingBen JammingBen force-pushed the accept-remote-share-reload branch 2 times, most recently from f20a30c to 2e169f5 Compare May 17, 2022 08:06
@JammingBen JammingBen added Status:Needs-Info Status:Needs-Review Needs review from a maintainer labels May 17, 2022
@JammingBen JammingBen changed the title Reload file list after accepting a remote share [full-ci] Reload file list after accepting a remote share May 17, 2022
@JammingBen JammingBen force-pushed the accept-remote-share-reload branch from 2e169f5 to 3c9eafe Compare May 17, 2022 13:30
@JammingBen JammingBen marked this pull request as ready for review May 17, 2022 15:31
Copy link
Contributor

@pascalwengerter pascalwengerter left a comment

Choose a reason for hiding this comment

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

In general LGTM, would like to hear @fschade & @kulmann 's opinions first though

@JammingBen JammingBen force-pushed the accept-remote-share-reload branch from 39d6887 to ac84f88 Compare May 18, 2022 08:18
Copy link
Contributor

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

Nice improvement to use the event bus for this. 👍 Small nitpick regarding a state value comparison.

@kulmann
Copy link
Contributor

kulmann commented May 20, 2022

Especially the latter would require some more logic. Logic, that shouldn't exist in the web runtime. Hence we would need to write something in the files app.

That should happen in the backend anyway. We don't create the default share_folder location in the frontend code. Or did I misread your comment?

@JammingBen
Copy link
Contributor Author

That should happen in the backend anyway. We don't create the default share_folder location in the frontend code. Or did I misread your comment?

The backend creates it, yes, but the frontend would need to know that to accordingly reload the page. Anyway, I think we're fine by always reloading when accepting a share, no matter in which directory you're currently in.

@kulmann
Copy link
Contributor

kulmann commented May 20, 2022

That should happen in the backend anyway. We don't create the default share_folder location in the frontend code. Or did I misread your comment?

The backend creates it, yes, but the frontend would need to know that to accordingly reload the page. Anyway, I think we're fine by always reloading when accepting a share, no matter in which directory you're currently in.

Yes, same opinion 😇 Fine this way for the time being.

@JammingBen JammingBen force-pushed the accept-remote-share-reload branch from ac84f88 to 2243183 Compare May 20, 2022 07:58
@JammingBen JammingBen removed Status:Needs-Review Needs review from a maintainer Status:Needs-Info labels May 20, 2022
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

11.8% 11.8% Coverage
0.0% 0.0% Duplication

@JammingBen JammingBen merged commit 4f0c7f0 into master May 20, 2022
@delete-merged-branch delete-merged-branch bot deleted the accept-remote-share-reload branch May 20, 2022 08:40
@kulmann kulmann mentioned this pull request May 25, 2022
25 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants