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 to allow sharing with UID that is an integer #37327

Closed
wants to merge 2 commits into from
Closed

Conversation

phil-davis
Copy link
Contributor

Description

TBD - I will add some UI acceptance test cases also.

Related Issue

How Has This Been Tested?

  • unit tests
  • manually sharing in the UI

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

@codecov
Copy link

codecov bot commented May 1, 2020

Codecov Report

Merging #37327 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #37327   +/-   ##
=========================================
  Coverage     64.53%   64.53%           
- Complexity    19167    19168    +1     
=========================================
  Files          1266     1266           
  Lines         74957    74959    +2     
  Branches       1331     1331           
=========================================
+ Hits          48373    48376    +3     
+ Misses        26192    26191    -1     
  Partials        392      392           
Flag Coverage Δ Complexity Δ
#javascript 54.14% <ø> (ø) 0.00 <ø> (ø)
#phpunit 65.69% <100.00%> (+<0.01%) 19168.00 <0.00> (+1.00)
Impacted Files Coverage Δ Complexity Δ
lib/private/Share20/Share.php 96.66% <100.00%> (+0.72%) 65.00 <0.00> (+1.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 0cd0dc7...eac86e9. Read the comment docs.

@phil-davis
Copy link
Contributor Author

@karakayasemi please feel free to work on this - take whatever test code and other code is useful.
As discussed in the issue, maybe incoming data in the JSON like shareWith=1.200 needs to be parsed earlier so that 1.200 can be made into string "1.200" to avoid it ever actually turning into a float where we cannot then know if it was "1.2" or "1.20" or "1.200"

@karakayasemi
Copy link
Contributor

@phil-davis I see your point on the float type. Probably, shareWith: 1.200 is evaluated as shareWith: 1.2 in the client side before reaching the server. I do not think, we need to spend time to fix all client errors, correcting only numeric values are enough as you suggest.

For this PR, I want to correct js request and send shareWith parameter as string in JSON-encoded payload. Also, instead of changing an interface, controller can cast request parameter to string and call the interface method with correct type.
I will prepare a commit with these changes in the day.

@phil-davis
Copy link
Contributor Author

See #37336 for the next approach.

@phil-davis phil-davis closed this May 4, 2020
@phil-davis phil-davis deleted the fix-37324 branch June 18, 2020 03:14
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.

Can't share with user that only has numbers as name
2 participants