X Tutup
Skip to content

Make sure to parse only if attributes are json string in reshare#36207

Merged
mrow4a merged 1 commit intomasterfrom
share_item_model_reshare_fix
Sep 23, 2019
Merged

Make sure to parse only if attributes are json string in reshare#36207
mrow4a merged 1 commit intomasterfrom
share_item_model_reshare_fix

Conversation

@mrow4a
Copy link
Contributor

@mrow4a mrow4a commented Sep 22, 2019 •
edited
Loading

@PVince81 @micbar it would be good to have this as part of 10.3 too, to make sure in reshare scenario we only parse JSONwith attributes if it was not already parsed.

This issue affects only resharing scenario with attributes on reshare when creating another reshare.

As part of #36186

@mrow4a mrow4a requested review from PVince81 and micbar September 22, 2019 12:13
@mrow4a mrow4a self-assigned this Sep 22, 2019
@codecov
Copy link

codecov bot commented Sep 22, 2019

Codecov Report

Merging #36207 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #36207   +/-   ##
=======================================
  Coverage      54%      54%           
=======================================
  Files          63       63           
  Lines        7408     7408           
  Branches     1309     1309           
=======================================
  Hits         4001     4001           
  Misses       3021     3021           
  Partials      386      386
Flag Coverage Δ
#javascript 54% <100%> (ø) ⬆️
Impacted Files Coverage Δ
core/js/shareitemmodel.js 80.59% <100%> (ø) ⬆️

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 2fdcd35...540059b. Read the comment docs.

@mrow4a mrow4a merged commit 1ecead6 into master Sep 23, 2019
@delete-merged-branch delete-merged-branch bot deleted the share_item_model_reshare_fix branch September 23, 2019 11:41
@mrow4a
Copy link
Contributor Author

mrow4a commented Sep 23, 2019

@micbar @PVince81 backport somewhere?

@micbar
Copy link
Contributor

micbar commented Sep 23, 2019

@mrow4a This was meant for 10.3.0?
Now it is on master. 10.3.0 is going to be tagged for RC1 in the release-10.3.0 branch.

@phil-davis phil-davis mentioned this pull request Sep 24, 2019
Merged
@phil-davis
Copy link
Contributor

phil-davis commented Sep 24, 2019

I made PR #36214 to cherry-pick this onto release-10.3.0 branch.
So someone can easily approve that if it is wanted for 10.3.0

@phil-davis
Copy link
Contributor

phil-davis commented Oct 3, 2019

@mrow4a was there anything observable on the UI that this fixes?
Any useful way to test that this is "a good thing"?

@mrow4a
Copy link
Contributor Author

mrow4a commented Oct 3, 2019

@phil-davis we can probably extend https://github.com/owncloud/core/blob/master/core/js/tests/specs/shareitemmodelSpec.js#L228 to test some edge cases

@phil-davis
Copy link
Contributor

phil-davis commented Oct 3, 2019

@mrow4a can you do that?
Just in master will be fine - that will provide CI test coverage, and the general CI acceptance tests plus general manual smoke testing in the release 10.3.0 branch already demonstrate that behaviour has not regressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@PVince81 PVince81 Awaiting requested review from PVince81

1 more reviewer

@micbar micbar micbar approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mrow4a mrow4a

Labels

3 - To Review enhancement Type:Bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mrow4a @micbar @phil-davis
X Tutup