X Tutup
Skip to content

fix OCS Share API response for requests contain "include_tags" parameter#37088

Merged
karakayasemi merged 1 commit intomasterfrom
fix-include-tags
Mar 10, 2020
Merged

fix OCS Share API response for requests contain "include_tags" parameter#37088
karakayasemi merged 1 commit intomasterfrom
fix-include-tags

Conversation

@karakayasemi
Copy link
Contributor

@karakayasemi karakayasemi commented Mar 6, 2020

Description

This PR simplifies tag generations for shares and fixes the duplicate share entry problem described in #37084 .
populateTags helper method is only used for tag generation for shares. There is no other usage of it in Github organization-wide. The method had too many nested loops. I simplified this method by customizing it for only shares.

@PVince81 Can you confirm the response with this PR so that I don't miss a thing?

Related Issue

How Has This Been Tested?

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

@karakayasemi karakayasemi self-assigned this Mar 6, 2020
@update-docs
Copy link

update-docs bot commented Mar 6, 2020

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.

PVince81
PVince81 reviewed Mar 6, 2020
View reviewed changes
apps/files/lib/Helper.php

foreach ($filesById as $key => $fileWithTags) {
foreach ($fileList as $key2 => $file) {
if ($file[$fileIdentifier] == $key) {
Copy link
Contributor

@PVince81 PVince81 Mar 6, 2020

Choose a reason for hiding this comment

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

I can't remember the reason for such complex code back then :-/

PVince81
PVince81 reviewed Mar 6, 2020
View reviewed changes
Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

Code looks fine. Can you add unit tests ?

@karakayasemi
Copy link
Contributor Author

karakayasemi commented Mar 6, 2020

Code looks fine. Can you add unit tests ?

If the response is okay, I can add some tests.

@codecov
Copy link

codecov bot commented Mar 6, 2020 •
edited
Loading

Codecov Report

Merging #37088 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #37088      +/-   ##
============================================
+ Coverage     64.76%   64.78%   +0.02%     
+ Complexity    19136    19132       -4     
============================================
  Files          1270     1270              
  Lines         74915    74912       -3     
  Branches       1328     1328              
============================================
+ Hits          48517    48531      +14     
+ Misses        26008    25991      -17     
  Partials        390      390
Flag Coverage Δ Complexity Δ
#javascript 54.18% <ø> (ø) 0.00 <ø> (ø)
#phpunit 65.96% <100.00%> (+0.02%) 19132.00 <5.00> (-4.00) ⬆️
Impacted Files Coverage Δ Complexity Δ
apps/files/lib/Helper.php 38.77% <100.00%> (+14.50%) 35.00 <5.00> (-5.00) ⬆️
...es_sharing/lib/Controller/Share20OcsController.php 94.20% <100.00%> (+0.37%) 209.00 <0.00> (ø)
lib/private/Archive/Archive.php 72.22% <0.00%> (-5.56%) 10.00% <0.00%> (ø%)
lib/private/Session/Internal.php 0.00% <0.00%> (ø) 26.00% <0.00%> (+1.00%)
lib/base.php 3.83% <0.00%> (+0.01%) 151.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d797555...a35be4d. Read the comment docs.

@PVince81
Copy link
Contributor

PVince81 commented Mar 6, 2020

I've retested on Phoenix with my scenario and there are no more duplicates. Thanks!

@karakayasemi
Copy link
Contributor Author

karakayasemi commented Mar 6, 2020

@PVince81 Manipulating ShareController tests is pretty complicated. I changed some ApiTest tests to cover requests with "include_tags = true" parameter.

@karakayasemi karakayasemi requested a review from PVince81 March 6, 2020 19:05
@karakayasemi karakayasemi force-pushed the fix-include-tags branch 2 times, most recently from 18cc718 to ec953f5 Compare March 7, 2020 15:38
@karakayasemi karakayasemi changed the title fix tag population for the shares of the same file fix OCS Share API response for requests contain "include_tags" parameter Mar 7, 2020
PVince81
PVince81 approved these changes Mar 10, 2020
View reviewed changes
Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍 from me. Would be good to also get approval from @micbar

micbar
micbar reviewed Mar 10, 2020
View reviewed changes
changelog/unreleased/37088 Outdated
Bugfix: fix OCS Share API response for requests contain "include_tags" parameter

Sending "include_tags" request parameter for OCS Share API was led to duplicated share entries in API response.
This problem has been fixed.
Copy link
Contributor

@micbar micbar Mar 10, 2020

Choose a reason for hiding this comment

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

@karakayasemi Can you describe the way it was fixed? "Reduced the complexity of ..."

Copy link
Contributor Author

@karakayasemi karakayasemi Mar 10, 2020

Choose a reason for hiding this comment

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

@micbar I extended the changelog entry with more details. Please, review one more time. Thanks.

Copy link
Contributor

@micbar micbar Mar 10, 2020

Choose a reason for hiding this comment

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

👍

@karakayasemi karakayasemi requested a review from micbar March 10, 2020 10:43
@karakayasemi karakayasemi merged commit ac41348 into master Mar 10, 2020
@delete-merged-branch delete-merged-branch bot deleted the fix-include-tags branch March 10, 2020 12:00
@karakayasemi karakayasemi restored the fix-include-tags branch March 10, 2020 12:00
@karakayasemi karakayasemi deleted the fix-include-tags branch March 10, 2020 12:00
@davitol davitol mentioned this pull request Mar 31, 2020
Closed
43 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jvillafanez jvillafanez Awaiting requested review from jvillafanez

2 more reviewers

@micbar micbar micbar approved these changes

@PVince81 PVince81 PVince81 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@karakayasemi karakayasemi

Labels

3 - To Review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Duplicate link share entry in OCS response with favorites

3 participants

@karakayasemi @PVince81 @micbar
X Tutup