Skip to content

Comments

Ocm to groups 20230717#40877

Closed
phil-davis wants to merge 17 commits intomasterfrom
ocm-to-groups-20230717
Closed

Ocm to groups 20230717#40877
phil-davis wants to merge 17 commits intomasterfrom
ocm-to-groups-20230717

Conversation

@phil-davis
Copy link
Contributor

@phil-davis phil-davis commented Jul 17, 2023 •
edited
Loading

Description

I have cherry-picked the real commits from the 221 commits currently in PR #40589

I think that this PR has the real code changes, with all the "merge to/from master" stuff that has got in there somehow.

Maybe this PR will be easier to review.

Related Issue

How Has This Been Tested?

CI

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

michielbdejong and others added 17 commits July 17, 2023 13:24
@phil-davis phil-davis mentioned this pull request Jul 17, 2023
Closed
11 tasks
jvillafanez
jvillafanez reviewed Jul 17, 2023
View reviewed changes
apps/files_sharing/lib/Controllers/ExternalSharesController.php
return null;
}

private function getManagerForShareType($share_type) {
Copy link
Member

@jvillafanez jvillafanez Jul 17, 2023

Choose a reason for hiding this comment

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

I think we should change this variable name, and other related ones.
I thought this was referring to the share constants (from the "lib/private/Share/Constants.php" file) but it doesn't seem to be the case. That's why I suggested to use a constant for the comparison below.

@phil-davis
Copy link
Contributor Author

phil-davis commented Jul 17, 2023

https://drone.owncloud.com/owncloud/core/38747/58/14

        Failed step: When user "Alice" gets the following properties of file "/tmp.txt" using the WebDAV API
        WebDav::listFolderAndReturnResponseXml Exception parsing response body: "<?xml version="1.0"?>
        <d:multistatus xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns" xmlns:oc="http://owncloud.org/ns"<?xml version="1.0" encoding="utf-8"?>
        <d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
          <s:exception>Error</s:exception>
          <s:message>Call to a member function getExpirationDate() on null</s:message>
        </d:error>
        " (Exception)

Quite a lot of test scenarios are failing. I was hoping that I could just cherry-pick the relevant code from #40589 and have a "clean" PR with green CI. But it seems not to be.

@michielbdejong @navid-shokri @soltanireza65 and/or whoever is getting this code going - maybe you can clean up your original PR and get it passing again?

@pako81
Copy link

pako81 commented Jul 31, 2023

@phil-davis close in favour of #40886 (which is getting green CI) ?

@phil-davis
Copy link
Contributor Author

phil-davis commented Jul 31, 2023

Yes, that has cleaned-up code with green CI.

@phil-davis phil-davis closed this Jul 31, 2023
@phil-davis phil-davis deleted the ocm-to-groups-20230717 branch July 31, 2023 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jvillafanez jvillafanez jvillafanez left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@phil-davis @pako81 @jvillafanez @michielbdejong @navid-shokri @shokri-navid @soltanireza65