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

Share item model attributes javascript api v2 #35836

Merged
merged 1 commit into from
Aug 23, 2019

Conversation

mrow4a
Copy link
Contributor

@mrow4a mrow4a commented Jul 16, 2019

This PR:

  • depreciates share attributes javascript api v1 which relied on "in-core registration of attributes" via ShareItemModel.registerShareAttribute
  • new js attr api v2 allows apps to extend ShareItemModel.updateShare and ShareItemModel.addShare function to pass the custom 'attributes'. This allows for much more customizations to checkbozes instead of relying on core api.
  • target 10.3?

Tested matrix of v1/v2 with:

Screenshot at Jul 16 09-06-08

@codecov
Copy link

codecov bot commented Jul 17, 2019

Codecov Report

Merging #35836 into master will increase coverage by 0.14%.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #35836      +/-   ##
==========================================
+ Coverage   53.85%      54%   +0.14%     
==========================================
  Files          63       63              
  Lines        7377     7403      +26     
  Branches     1301     1307       +6     
==========================================
+ Hits         3973     3998      +25     
  Misses       3019     3019              
- Partials      385      386       +1
Flag Coverage Δ
#javascript 54% <92.85%> (+0.14%) ⬆️
Impacted Files Coverage Δ
core/js/sharedialogshareelistview.js 81.14% <100%> (+0.65%) ⬆️
core/js/shareitemmodel.js 80.92% <92.75%> (+0.92%) ⬆️

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 95e7aa0...e99e2f2. Read the comment docs.

@micbar
Copy link
Contributor

micbar commented Jul 26, 2019

We are blocked, needs review from @PVince81

@CLAassistant
Copy link

CLAassistant commented Aug 7, 2019

CLA assistant check
All committers have signed the CLA.

@mrow4a mrow4a force-pushed the share_item_model_attr_api_v2 branch 2 times, most recently from 6f1223f to bad8336 Compare August 15, 2019 20:41
@mrow4a
Copy link
Contributor Author

mrow4a commented Aug 15, 2019

@pmaier1 @PVince81 @DeepDiver1975 updated top post with tested matrix of v1/v2.

@mrow4a mrow4a force-pushed the share_item_model_attr_api_v2 branch from bad8336 to 3f7e5cc Compare August 18, 2019 16:02
@mrow4a
Copy link
Contributor Author

mrow4a commented Aug 19, 2019

@DeepDiver1975 @pmaier1 @micbar all tests green

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.

Looks fine overall.

One thing I'm not very clear about is that some comment says that the API v2 requires overriding the ShareItemModel.addShare/updateShare, but I don't see any overriding done in the unit tests. What I see however, is that the model has an "attributes" JS property that can be set.

I guess the overriding by apps is only needed in case special handling is needed ? Can you clarify ?

).toEqual(attributesToExpect);
});

it('uses/hides attributes with permission filters of registered attributes', function() {
it('updates attribute with new enabled value correctly when boyh shareAttributesApi v1 and v2 are used', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

"boyh" => "both"

@mrow4a
Copy link
Contributor Author

mrow4a commented Aug 20, 2019

@PVince81 yes, this is used when some special handling is needed:

  • if app has custom checkboxes not relying on other permissions (edit, share etc) app could just send a request to api server directly
  • if app checkbox changes with permissions (edit, share etc), addShare/updateShare is wrapped to intercept that change.

If this is possible, I can try to write a test that wrapps the updateShare call. Should I?

@PVince81
Copy link
Contributor

If this is possible, I can try to write a test that wrapps the updateShare call. Should I?

the test might not be that useful as the code path has no effect on the rest of the logic, considering that the overriding is mostly there for rendering purposes ?

if you think there's more to it, please go ahead and add tests. (and fix the typo)

then we're good to go :-)

@mrow4a mrow4a force-pushed the share_item_model_attr_api_v2 branch from 3f7e5cc to e99e2f2 Compare August 22, 2019 19:18
@mrow4a
Copy link
Contributor Author

mrow4a commented Aug 22, 2019

@PVince81 added tests for wrapping - check for can be wrapped in js tests.

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.

👍

@PVince81 PVince81 merged commit 231e425 into master Aug 23, 2019
@PVince81 PVince81 deleted the share_item_model_attr_api_v2 branch August 23, 2019 06:47
@LinneyS
Copy link
Contributor

LinneyS commented Aug 23, 2019

@mrow4a
Sorry, but I don’t understand which branch should I use now to test the new logic?

@mrow4a mrow4a restored the share_item_model_attr_api_v2 branch August 24, 2019 13:46
@mrow4a
Copy link
Contributor Author

mrow4a commented Aug 24, 2019

@LinneyS please for now use share_item_model_attr_api_v2 (https://github.com/owncloud/core/tree/share_item_model_attr_api_v2) , in the future owncloud master (which is current target for next oc10 release), but this fix is required there #36086 .

@mrow4a
Copy link
Contributor Author

mrow4a commented Aug 24, 2019

@LinneyS I would recommend also not to rush with implementing new logic in your app, we are still searching for best recommended "styling". I would be happy also to submit some PR to your working branch for share attributes, or in talk.owncloud.com we could have a chat and make sure we are all aligned.

@davitol davitol mentioned this pull request Sep 3, 2019
13 tasks
@mrow4a mrow4a deleted the share_item_model_attr_api_v2 branch March 21, 2020 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants