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.

Sign up for GitHub

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

Jump to bottom

[tests-only][full-ci]Reuse code from code for favorites.feature in ocis #4492

Merged
merged 2 commits into from
Sep 5, 2022
Merged

[tests-only][full-ci]Reuse code from code for favorites.feature in ocis #4492

merged 2 commits into from
Sep 5, 2022

Conversation

SagarGi
Copy link
Member

@SagarGi SagarGi commented Sep 1, 2022 •
edited
Loading

This PR of using the resource (code) from core as much as possible since there has been a duplication of code that already exists in core. A method to set the space id sets the space id of (Space) not implemented on core is added to make it work. This is an example with favorite.feature of ocis. A few adjustment has been done in core for this to works. Can be seen in this Core PR owncloud/core#40328.

And This PR depends on:

owncloud/core#40328

@SagarGi SagarGi self-assigned this Sep 1, 2022
@SagarGi SagarGi added the QA:team label Sep 1, 2022
@SagarGi SagarGi force-pushed the reuseCodeFromCodeForFavourites branch from f2d6368 to 7cf0fc5 Compare September 1, 2022 09:58
@SagarGi SagarGi mentioned this pull request Sep 1, 2022
Closed
11 tasks
@SagarGi SagarGi marked this pull request as ready for review September 1, 2022 11:06
@SagarGi SagarGi force-pushed the reuseCodeFromCodeForFavourites branch from 7cf0fc5 to 574eb5b Compare September 2, 2022 06:10
@SagarGi SagarGi mentioned this pull request Sep 2, 2022
Merged
11 tasks
@SagarGi SagarGi force-pushed the reuseCodeFromCodeForFavourites branch 2 times, most recently from 33e3098 to c77c33f Compare September 2, 2022 10:01
@SagarGi SagarGi force-pushed the reuseCodeFromCodeForFavourites branch from c77c33f to 51ef32c Compare September 5, 2022 03:39
@SagarGi SagarGi force-pushed the reuseCodeFromCodeForFavourites branch from 51ef32c to bbe4c75 Compare September 5, 2022 03:45
amrita-shrestha
amrita-shrestha reviewed Sep 5, 2022
View reviewed changes
Copy link
Contributor

@amrita-shrestha amrita-shrestha left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@SagarGi are you going to make a different issue/PR for the remaining duplication?
Their is remaining code to refactor sendPutRequestToUrl sendCreateFolderRequest() sendPutRequestToUrl .
I think we can directly use core code for HTTP requests. Do we need this function in ocis?

@SagarGi
Copy link
Member Author

SagarGi commented Sep 5, 2022 •
edited
Loading

@SagarGi are you going to make a different pr for the remaining duplication? Their is remaining code to refactor sendPutRequestToUrl sendCreateFolderRequest() sendPutRequestToUrl . I think we can directly use core code for HTTP requests. Do we need this function in ocis?

@amrita-shrestha We need to only refactor if we are implementing and duplicating the core code. I don't think we need to refactor those. I will be creating different issue if this PR is merge. This is a Sample PR for it only.

@amrita-shrestha
Copy link
Contributor

@SagarGi are you going to do adjustment for

@SagarGi are you going to make a different pr for the remaining duplication? Their is remaining code to refactor sendPutRequestToUrl sendCreateFolderRequest() sendPutRequestToUrl . I think we can directly use core code for HTTP requests. Do we need this function in ocis?

@amrita-shrestha We need to only refactor if we are implementing and duplicating the core code. I don't think we need to refactor those. I will be creating different issue if this PR is merge. This is a Sample PR for it only.

@SagarGi If you are going to create an issue for refactoring existing duplicate code as you told then that is nice. Otherwise, if we left other existing duplicated code like that then I think there will be no consistency, and newcomers/others will be confused.

amrita-shrestha
amrita-shrestha reviewed Sep 5, 2022
View reviewed changes
Copy link
Contributor

@amrita-shrestha amrita-shrestha left a comment

Choose a reason for hiding this comment

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

LGTM

@SagarGi
Copy link
Member Author

SagarGi commented Sep 5, 2022

@SagarGi are you going to do adjustment for

@SagarGi are you going to make a different pr for the remaining duplication? Their is remaining code to refactor sendPutRequestToUrl sendCreateFolderRequest() sendPutRequestToUrl . I think we can directly use core code for HTTP requests. Do we need this function in ocis?

@amrita-shrestha We need to only refactor if we are implementing and duplicating the core code. I don't think we need to refactor those. I will be creating different issue if this PR is merge. This is a Sample PR for it only.

@SagarGi If you are going to create an issue for refactoring existing duplicate code as you told then that is nice. Otherwise, if we left other existing duplicated code like that then I think there will be no consistency, and newcomers/others will be confused.

@amrita-shrestha Yes i will be creating once this PR gets merged. We will be clearing duplication code where we have added during shifting (/Shares) related issue.

SwikritiT
SwikritiT reviewed Sep 5, 2022
View reviewed changes
Copy link
Contributor

@SwikritiT SwikritiT left a comment

Choose a reason for hiding this comment

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

apart from above little comment LGTM

@SagarGi SagarGi force-pushed the reuseCodeFromCodeForFavourites branch from bbe4c75 to 84c4806 Compare September 5, 2022 05:09
@sonarcloud
Copy link

sonarcloud bot commented Sep 5, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@phil-davis phil-davis mentioned this pull request Sep 5, 2022
Closed
13 tasks
@phil-davis
Copy link
Contributor

Note: this gets the core commit id up-to-date - issue owncloud/QA#763

@amrita-shrestha amrita-shrestha merged commit e27e5a2 into master Sep 5, 2022
@delete-merged-branch delete-merged-branch bot deleted the reuseCodeFromCodeForFavourites branch September 5, 2022 06:10
@SagarGi SagarGi mentioned this pull request Sep 5, 2022
Closed
ownclouders pushed a commit that referenced this pull request Sep 5, 2022
@amrita-shrestha
commit e27e5a2
00fffed 
Merge: c87de76 84c4806
Author: Amrita <[email protected]>
Date:   Mon Sep 5 11:55:12 2022 +0545

    Merge pull request #4492 from owncloud/reuseCodeFromCodeForFavourites

    [tests-only][full-ci]Reuse code from code for `favorites.feature` in ocis
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amrita-shrestha amrita-shrestha amrita-shrestha left review comments

@SwikritiT SwikritiT SwikritiT left review comments

@phil-davis phil-davis phil-davis approved these changes

@saw-jan saw-jan Awaiting requested review from saw-jan

@individual-it individual-it Awaiting requested review from individual-it

@grgprarup grgprarup Awaiting requested review from grgprarup

Assignees

@SagarGi SagarGi

Labels
QA:team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@SagarGi @amrita-shrestha @phil-davis @SwikritiT