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

fix: Git connected Application forking should use default branch application #13255

Merged
merged 5 commits into from
Apr 29, 2022
Merged

fix: Git connected Application forking should use default branch application #13255

merged 5 commits into from
Apr 29, 2022

Conversation

AnaghHegde
Copy link
Member

@AnaghHegde AnaghHegde commented Apr 25, 2022 •
edited
Loading

Description

For git connected application user can update the default branch. When the user updated the default branch forking service should use the default branched application not the original(appsmith default) application.

Fixes #13185

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • JUnit Tests
  • Locally

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@vercel
Copy link

vercel bot commented Apr 25, 2022 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
appsmith ✅ Ready (Inspect) Visit Preview Apr 29, 2022 at 11:56AM (UTC)

@AnaghHegde AnaghHegde self-assigned this Apr 25, 2022
@AnaghHegde AnaghHegde requested a review from abhvsn April 25, 2022 05:53
@github-actions github-actions bot added Bug Something isn't working Git Product Issues related to version control product High This issue blocks a user from building or impacts a lot of users Needs Triaging Needs attention from maintainers to triage Platform Pod Release Platform Administration Pod Issues related to platform administration & management and removed Bug Something isn't working labels Apr 25, 2022
@AnaghHegde
Copy link
Member Author

/ok-to-test sha=54f835d

@github-actions
Copy link

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2218286104.
Workflow: Appsmith External Integration Test Workflow.
Commit: 54f835d.
PR: 13255.

@github-actions
Copy link

UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2218286104.
Commit: 54f835d.
Results:

Click to view performance test results

Run 1 Run 2 Run 3 Run 4 Run 5 Median Mean SD.Sample SD.Population
SELECT_CATEGORY
scripting 761.7 798.04 694.83 703.11 818.23 761.7 755.18 7.32 6.54
painting 6.95 8.99 3.83 4.15 4.01 4.15 5.59 41.14 36.67
rendering 319.6 314.65 279.61 312.34 285.11 312.34 302.26 6.11 5.46
BIND_TABLE_DATA
scripting 2382.5 2276.67 2332.53 2283.09 2279.22 2283.09 2310.8 2.00 1.79
painting 21.2 14.88 21.31 17.88 19.51 19.51 18.96 14.14 12.61
rendering 613.22 596.26 640 581.39 662.41 613.22 618.66 5.29 4.73
CLICK_ON_TABLE_ROW
scripting 2119.07 2128.1 1984.78 1795.73 1980.39 1984.78 2001.61 6.75 6.03
painting 23.79 18.85 13.62 16.98 17.51 17.51 18.15 20.33 18.18
rendering 303.88 311.51 286.78 291.76 288.08 291.76 296.4 3.65 3.26
UPDATE_POST_TITLE
scripting 3127.82 3013.69 2906.47 2937.4 3317.67 3013.69 3060.61 5.46 4.88
painting 28.39 17.65 18.4 13.2 19.71 18.4 19.47 28.51 25.53
rendering 383.96 376.66 357.89 370.01 416.15 376.66 380.93 5.75 5.14
OPEN_MODAL
scripting 1241.47 3211.02 2252.69 1917 1305.5 1917 1985.54 40.56 36.28
painting 11.24 10.96 12.55 14.55 10.37 11.24 11.93 14.00 12.49
rendering 417.63 422.78 418.95 559.07 430.48 422.78 449.78 13.63 12.19
CLOSE_MODAL
scripting 650.76 659.24 781.77 768.87 685.06 685.06 709.14 8.73 7.81
painting 4.98 7.8 4.32 5.11 7.5 5.11 5.94 26.77 23.91
rendering 239.95 252.49 257.4 284.17 258.85 257.4 258.57 6.24 5.58

abhvsn
abhvsn reviewed Apr 25, 2022
View reviewed changes
...h-server/src/main/java/com/appsmith/server/solutions/ce/ApplicationForkingServiceCEImpl.java Outdated
@@ -38,7 +39,20 @@ public class ApplicationForkingServiceCEImpl implements ApplicationForkingServic

public Mono<Application> forkApplicationToOrganization(String srcApplicationId, String targetOrganizationId) {
final Mono<Application> sourceApplicationMono = applicationService.findById(srcApplicationId, AclPermission.READ_APPLICATIONS)
.switchIfEmpty(Mono.error(new AppsmithException(AppsmithError.NO_RESOURCE_FOUND, FieldName.APPLICATION, srcApplicationId)));
.switchIfEmpty(Mono.error(new AppsmithException(AppsmithError.NO_RESOURCE_FOUND, FieldName.APPLICATION, srcApplicationId)))
.flatMap(application -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make changes to upstream method as it has branch reference already:

public Mono<Application> forkApplicationToOrganization(String srcApplicationId,
                                                           String targetOrganizationId,
                                                           String branchName)

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please move this to requested location just like we had done for cloneApplication?

abhvsn
abhvsn reviewed Apr 25, 2022
View reviewed changes
Copy link
Contributor

@abhvsn abhvsn left a comment

Choose a reason for hiding this comment

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

Can you please fix this for other APIs as well:

  1. Duplicate
  2. Export

@AnaghHegde
Copy link
Member Author

@abhvsn We do not fetch the application there. It would be good to add it here right?

@github-actions github-actions bot added the Bug Something isn't working label Apr 26, 2022
@AnaghHegde AnaghHegde requested a review from abhvsn April 27, 2022 06:58
@abhvsn
Copy link
Contributor

abhvsn commented Apr 28, 2022

@abhvsn We do not fetch the application there. It would be good to add it here right?

But here we expect we have the actual srcApplicationId, which is a abstraction layer we have between the actual business logic and the controller

abhvsn
abhvsn requested changes Apr 28, 2022
View reviewed changes
Copy link
Contributor

@abhvsn abhvsn left a comment

Choose a reason for hiding this comment

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

Please follow the same implementation that we have for duplicate application API. Also can you please add the same for export application

@AnaghHegde AnaghHegde requested a review from abhvsn April 29, 2022 06:45
abhvsn
abhvsn requested changes Apr 29, 2022
View reviewed changes
...h-server/src/main/java/com/appsmith/server/solutions/ce/ApplicationForkingServiceCEImpl.java Outdated
return applicationService.findBranchedApplicationId(
application.getGitApplicationMetadata().getDefaultBranchName(),
srcApplicationId,
AclPermission.MANAGE_APPLICATIONS
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update this with read_permission as forking is enabled for view access as well for sample apps

@abhvsn abhvsn self-requested a review April 29, 2022 12:01
@AnaghHegde
Copy link
Member Author

/ok-to-test sha=bb5825e

@github-actions
Copy link

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2245224721.
Workflow: Appsmith External Integration Test Workflow.
Commit: bb5825e.
PR: 13255.

@AnaghHegde AnaghHegde merged commit 3e3a326 into release Apr 29, 2022
@AnaghHegde AnaghHegde deleted the bug/fork-default-branch branch April 29, 2022 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abhvsn abhvsn abhvsn approved these changes

Assignees

@AnaghHegde AnaghHegde

Labels
Bug Something isn't working Git Product Issues related to version control product High This issue blocks a user from building or impacts a lot of users Needs Triaging Needs attention from maintainers to triage Platform Administration Pod Issues related to platform administration & management Release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: The application doesn't fork based on the new default branch
3 participants
@AnaghHegde @abhvsn @areyabhishek