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

Null check before new TagService #27308

Merged
merged 5 commits into from May 2, 2017

Conversation

Projects
None yet
5 participants
@Luncher91
Contributor

Luncher91 commented Mar 5, 2017

By ordering public shares, UserFolder and UserSession are null and lead to an Error while creating a TagService.

Description

The UserSession and the UserFolder will be checked for being null before creating a new TagService object.

Related Issue

None

Motivation and Context

  1. Being not logged in
  2. open a public share
  3. sort the files by name
    -> error while loading page
  4. the page reloads automatically

How Has This Been Tested?

changed in my version from the apt repository and reproduced these steps.

Screenshots (if appropriate):

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)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
Null check before new TagService
By ordering public shares, UserFolder and UserSession are null and lead to an Error while creating a TagService.
@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Mar 5, 2017

@Luncher91, thanks for your PR! By analyzing the history of the files in this pull request, we identified @PVince81, @tobiasKaminsky and @DeepDiver1975 to be potential reviewers.

mention-bot commented Mar 5, 2017

@Luncher91, thanks for your PR! By analyzing the history of the files in this pull request, we identified @PVince81, @tobiasKaminsky and @DeepDiver1975 to be potential reviewers.

@CLAassistant

This comment has been minimized.

Show comment
Hide comment
@CLAassistant

CLAassistant Mar 5, 2017

CLA assistant check
All committers have signed the CLA.

CLAassistant commented Mar 5, 2017

CLA assistant check
All committers have signed the CLA.

Show outdated Hide outdated apps/files/lib/AppInfo/Application.php
$c->query('Tagger'),
$homeFolder
);
}

This comment has been minimized.

@PVince81

PVince81 Mar 6, 2017

Member

return null otherwise ?

@PVince81

PVince81 Mar 6, 2017

Member

return null otherwise ?

This comment has been minimized.

@PVince81

PVince81 Apr 3, 2017

Member

ping

@PVince81
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 6, 2017

Member

Tested with your steps but this PR doesn't fix the issue.

What I observe is not related to tags but the file list seems to call an API: http://localhost/owncloud/index.php/apps/files/api/v1/sorting which returns 401 Not Authenticated.

From what I see this endpoint only exists to be able to store the sorting settings when logged in.
The correct fix is to find a way to prevent sort persistence when dealing with a public page.

Need to look at https://github.com/owncloud/core/blob/v9.1.4/apps/files_sharing/js/public.js#L158 and find a way to set the "persist" always to false. Maybe overriding setSort could be the way to go.

Member

PVince81 commented Mar 6, 2017

Tested with your steps but this PR doesn't fix the issue.

What I observe is not related to tags but the file list seems to call an API: http://localhost/owncloud/index.php/apps/files/api/v1/sorting which returns 401 Not Authenticated.

From what I see this endpoint only exists to be able to store the sorting settings when logged in.
The correct fix is to find a way to prevent sort persistence when dealing with a public page.

Need to look at https://github.com/owncloud/core/blob/v9.1.4/apps/files_sharing/js/public.js#L158 and find a way to set the "persist" always to false. Maybe overriding setSort could be the way to go.

@DeepDiver1975 DeepDiver1975 added this to the 10.0 milestone Mar 16, 2017

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Mar 16, 2017

Member

@Luncher91 any update on this? THX

Member

DeepDiver1975 commented Mar 16, 2017

@Luncher91 any update on this? THX

@Luncher91

This comment has been minimized.

Show comment
Hide comment
@Luncher91

Luncher91 Mar 21, 2017

Contributor

Not yet. But I will look at the suggested files. @PVince81 considerations seem to be plausable on the first glance.

Contributor

Luncher91 commented Mar 21, 2017

Not yet. But I will look at the suggested files. @PVince81 considerations seem to be plausable on the first glance.

Update filelist.js
check if a user is logged in before trying to persist sorting
@Luncher91

This comment has been minimized.

Show comment
Hide comment
@Luncher91

Luncher91 Apr 1, 2017

Contributor

Please, can someone fix that unittest who is maybe more familiar with the intendet function?

Contributor

Luncher91 commented Apr 1, 2017

Please, can someone fix that unittest who is maybe more familiar with the intendet function?

Show outdated Hide outdated apps/files/lib/AppInfo/Application.php
$userSession = $c->query('ServerContainer')->getUserSession();
if(!is_null($userSession) && !is_null($homeFolder))
{

This comment has been minimized.

Show outdated Hide outdated apps/files/js/filelist.js
@@ -1534,7 +1534,7 @@
}
}
if (persist) {
if (persist && OC.getCurrentUser().uid != null) {

This comment has been minimized.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Apr 3, 2017

Member

No one can push on your fork, you'll have to fix it yourself.

Try running: make test-js-debug.
Then patch fileListSpec.js:

diff --git a/apps/files/tests/js/filelistSpec.js b/apps/files/tests/js/filelistSpec.js
index c5186a65c7..b9ef3f39d7 100644
--- a/apps/files/tests/js/filelistSpec.js
+++ b/apps/files/tests/js/filelistSpec.js
@@ -2299,6 +2299,14 @@ describe('OCA.Files.FileList tests', function() {
                });
        });
        describe('Sorting files', function() {
+               var currentUserStub;
+
+               beforeEach(function() {
+                       currentUserStub = sinon.stub(OC, 'getCurrentUser').returns({uid: 'user1'});
+               });
+               afterEach(function() {
+                       currentUserStub.restore();
+               });
                it('Toggles the sort indicator when clicking on a column header', function() {
                        var ASC_CLASS = fileList.SORT_INDICATOR_ASC_CLASS;
                        var DESC_CLASS = fileList.SORT_INDICATOR_DESC_CLASS;
Member

PVince81 commented Apr 3, 2017

No one can push on your fork, you'll have to fix it yourself.

Try running: make test-js-debug.
Then patch fileListSpec.js:

diff --git a/apps/files/tests/js/filelistSpec.js b/apps/files/tests/js/filelistSpec.js
index c5186a65c7..b9ef3f39d7 100644
--- a/apps/files/tests/js/filelistSpec.js
+++ b/apps/files/tests/js/filelistSpec.js
@@ -2299,6 +2299,14 @@ describe('OCA.Files.FileList tests', function() {
                });
        });
        describe('Sorting files', function() {
+               var currentUserStub;
+
+               beforeEach(function() {
+                       currentUserStub = sinon.stub(OC, 'getCurrentUser').returns({uid: 'user1'});
+               });
+               afterEach(function() {
+                       currentUserStub.restore();
+               });
                it('Toggles the sort indicator when clicking on a column header', function() {
                        var ASC_CLASS = fileList.SORT_INDICATOR_ASC_CLASS;
                        var DESC_CLASS = fileList.SORT_INDICATOR_DESC_CLASS;
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Apr 26, 2017

Member

@Luncher91 any update ?

Member

PVince81 commented Apr 26, 2017

@Luncher91 any update ?

@PVince81 PVince81 modified the milestones: 10.0.1, 10.0 Apr 26, 2017

@Luncher91

This comment has been minimized.

Show comment
Hide comment
@Luncher91

Luncher91 Apr 29, 2017

Contributor

is there anything wrong with the build system (mysql docker timed out?) or did my fork actually fail?

Contributor

Luncher91 commented Apr 29, 2017

is there anything wrong with the build system (mysql docker timed out?) or did my fork actually fail?

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 2, 2017

Member

it's likely that the build system is moody today

Member

PVince81 commented May 2, 2017

it's likely that the build system is moody today

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 2, 2017

Member

👍 tested, works.

Member

PVince81 commented May 2, 2017

👍 tested, works.

@PVince81 PVince81 merged commit 36d6822 into owncloud:master May 2, 2017

3 of 4 checks passed

continuous-integration/jenkins/pr-head This commit cannot be built
Details
Scrutinizer 1711 Issues, 168 Patches
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment