Skip to content

Conversation

@PVince81
Copy link
Contributor

@PVince81 PVince81 commented Feb 8, 2015

Follow up of #63

Remaining tasks:

  • Add python docs for the ShareInfo methods (I can help if needed as I might be more familiar with what they represent)
  • remove PublicShare and UserShare classes if possible and replace with ShareInfo. But I suspect that share_with_link doesn't get a full ShareInfo instance, is that correct ? (we can also address this separately as it's more tricky)

@PVince81
Copy link
Contributor Author

PVince81 commented Feb 8, 2015

  • verify that "path" is the source path, not the target path (run as recipient and owner)

@PVince81 PVince81 mentioned this pull request Mar 12, 2015
@PVince81 PVince81 mentioned this pull request Aug 21, 2015
11 tasks
@SergioBertolinSG
Copy link
Contributor

Failures I've got while running tests of this branch http://paste.debian.net/348550/

@SergioBertolinSG
Copy link
Contributor

OK the 403 errors where returned because share user in target server wasn't created.

Two problems here:

  • share user should be created and deleted during the test in my opinion.
  • 403 answer is not well documented. Official doc names it as 403 - public upload was disabled by the admin. Which is not the case.

@SergioBertolinSG
Copy link
Contributor

Now failing owncloud/test/test.py::TestFileAccess::test_rename_unicode

ConnectionError: ('Connection aborted.', error(61, 'Connection refused'))

@SergioBertolinSG
Copy link
Contributor

Apart from the failing test 👍

@PVince81
Copy link
Contributor Author

Hmm, needs rebase due to conflicts

@SergioBertolinSG
Copy link
Contributor

Rebased.

@PVince81
Copy link
Contributor Author

Github still says it has conflicts. Make sure you pulled/fetched master before rebasing.

@SergioBertolinSG
Copy link
Contributor

Yes, last merge added a new one. Rebased again.

@PVince81
Copy link
Contributor Author

We might also want to change get_shares to return instances of ShareInfo and PublicShare, if possible. Might be complicated though. Maybe we can merge this first.
Also changing get_shares might break API consumers already using it... (smashbox, etc)

@SergioBertolinSG what do you think ?

@SergioBertolinSG
Copy link
Contributor

I've searched smashbox and there is no use of the method get_shares there.

Yes I think it is better to return instances.

@SergioBertolinSG
Copy link
Contributor

@PVince81 please review. I guess oc 7 fail with test_share_with_group can be addressed in an upcoming PR.

@SergioBertolinSG
Copy link
Contributor

These smashbox tests will need some changes.

./lib/owncloud/test_sharePropagation.py
./lib/owncloud/test_sharePropagationGroups.py
./lib/owncloud/test_sharePropagationInside.py
./lib/owncloud/test_sharePropagationInsideGroups.py

Better to change them or make share_id a public field?

@PVince81
Copy link
Contributor Author

Yeah, make share id a public field. Could be useful in other cases.

return value
except:
return None

class PublicShare():
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should this extend ShareInfo ?

Copy link
Contributor

Choose a reason for hiding this comment

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

They can be removed (UserShare and PublicShare). Is that a bad option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can remove UserShare.

But we cannot remove PublicShare because it exists since v0.1, that would break existing API.
This, unless ShareInfo's API is compatible. Goal is to not break scripts that were using v0.1.

@PVince81
Copy link
Contributor Author

Code looks good otherwise 😄

@SergioBertolinSG
Copy link
Contributor

@PVince81 please review.

return "UserShare(id=%i,path='%s',perms=%s)" % \
(self.share_id, self.share, self.perms)


class GroupShare():
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove GroupShare too ?

For some reason I don't see its usage on your branch, but I see it here https://github.com/owncloud/pyocclient/blob/master/owncloud/owncloud.py#L1212
You might need to rebase onto master to get this recently merged functionality

@SergioBertolinSG
Copy link
Contributor

@PVince81 Tests passing.

@PVince81
Copy link
Contributor Author

Please check if smashbox can still run against this branch, if yes then good to go 👍

@SergioBertolinSG
Copy link
Contributor

Smashbox sharing tests are working now.

@PVince81
Copy link
Contributor Author

👍

PVince81 pushed a commit that referenced this pull request Dec 22, 2015
[WIP] Added function to get share info
@PVince81 PVince81 merged commit 9be686c into master Dec 22, 2015
@PVince81 PVince81 deleted the shares-getshareinfo branch December 22, 2015 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants