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

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

Merged
merged 1 commit into from
Mar 10, 2020

Conversation

karakayasemi
Copy link
Contributor

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

@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.


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

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 :-/

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

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

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

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

@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
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

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

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

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

Choose a reason for hiding this comment

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

👍

@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicate link share entry in OCS response with favorites
3 participants