Skip to content

Commit

Permalink
fix: [Git] Avoid 404 when checking out a branch (#21894)
Browse files Browse the repository at this point in the history
## Description

After checkout, we will now check if the resource the user was accessing
is available in the incoming branch. Instead of calling the apis to
check this, we will listen to the success action and then handle check
if the current resource is still available in the branch. If not, we
will navigate the user to the home page of the app so that they do not
see a 404 error


> Don't show a 404 error when a resource is not available in the checked
out branch, instead take them to the home page of the app

Fixes #17234
Fixes #20883


Media



## Type of change

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


## How Has This Been Tested?

- Manual
   - Have a git connected app
   - Create a new branch 
   - Create a new API/Query/Page on the new branch
   - Switch back to the original branch
- Test: The app should not show 404 error but be navigated to the home
page of the app

- Cypress
Updated the existing cypress tests that avoided the error to make sure
they test the fix instead

### Test Plan
> Add Testsmith test cases links that relate to this PR

### Issues raised during DP testing
> Link issues raised during DP testing for better visiblity and tracking
(copy link from comments dropped on this PR)


## Checklist:
### Dev activity
- [ ] 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
- [ ] PR is being merged under a feature flag


### QA activity:
- [ ] Test plan has been approved by relevant developers
- [ ] Test plan has been peer reviewed by QA
- [ ] Cypress test cases have been added and approved by either SDET or
manual QA
- [ ] Organized project review call with relevant stakeholders after
Round 1/2 of QA
- [ ] Added Test Plan Approved label after reveiwing all Cypress test
  • Loading branch information
hetunandu authored Apr 4, 2023
1 parent 0b9a9a2 commit 0094aab
Show file tree
Hide file tree
Showing 6 changed files with 93 additions and 19 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -109,10 +109,18 @@ describe("Git sync:", function () {
// reflect in api sidebar after the call passes.
// eslint-disable-next-line cypress/no-unnecessary-waiting
cy.wait(2000);
cy.GlobalSearchEntity("ParentPage1");
cy.contains("ParentPage1").click();

// A switch here should not show a 404 page
cy.switchGitBranch(parentBranchKey);
// When entity not found, takes them to the home page
cy.get(`.t--entity.page`)
.contains("Page1")
.closest(".t--entity")
.should("be.visible")
.should("have.class", "activePage");

cy.GlobalSearchEntity("ParentPage1");
cy.contains("ParentPage1").click();

cy.get(`.t--entity-name:contains("ChildPage1")`).should("not.exist");
cy.CheckAndUnfoldEntityItem("Queries/JS");
Expand Down Expand Up @@ -235,7 +243,7 @@ describe("Git sync:", function () {
});

// Validate the error faced when user switches between the branches
it("6. error faced when user switches branch with new page", function () {
it("6. no error faced when user switches branch with new page", function () {
cy.goToEditFromPublish(); //Adding since skipping 6th case
cy.generateUUID().then((uuid) => {
_.gitSync.CreateGitBranch(childBranchKey, true);
Expand All @@ -247,9 +255,13 @@ describe("Git sync:", function () {
cy.wait(400);
cy.get(gitSyncLocators.branchListItem).contains("master").click();
cy.wait(4000);
cy.contains("Page not found");
cy.get(`.t--entity.page`)
.contains("Page1")
.closest(".t--entity")
.should("be.visible")
.should("have.class", "activePage");
cy.get(".t--canvas-artboard").should("be.visible");
});
cy.go("back");
cy.reload();
});

Expand Down
2 changes: 1 addition & 1 deletion app/client/src/entities/Engine/AppEditorEngine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ export default class AppEditorEngine extends AppEngine {
branch: branchInStore,
}),
);
// init of temporay remote url from old application
// init of temporary remote url from old application
yield put(remoteUrlInputValue({ tempRemoteUrl: "" }));
// add branch query to path and fetch status
if (branchInStore) {
Expand Down
9 changes: 8 additions & 1 deletion app/client/src/entities/URLRedirect/URLAssembly.ts
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,7 @@ export class URLBuilder {
persistExistingParams = false,
suffix,
pageId,
branch,
} = builderParams;

if (!pageId) {
Expand All @@ -215,7 +216,13 @@ export class URLBuilder {
persistExistingParams,
);

const modifiedQueryParams = { ...queryParamsToPersist, ...params };
const branchParams = branch ? { branch: encodeURIComponent(branch) } : {};

const modifiedQueryParams = {
...queryParamsToPersist,
...params,
...branchParams,
};

const queryString = getQueryStringfromObject(modifiedQueryParams);

Expand Down
2 changes: 1 addition & 1 deletion app/client/src/pages/Editor/APIEditor/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ class ApiEditor extends React.Component<Props> {
pluginId,
plugins,
} = this.props;
if (!this.props.pluginId && this.props.match.params.apiId) {
if (!pluginId && apiId) {
return <EntityNotFoundPane />;
}
if (isCreating || !isEditorInitialized) {
Expand Down
4 changes: 0 additions & 4 deletions app/client/src/pages/Editor/EntityNotFoundPane.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,6 @@ function EntityNotFoundPane(props: Props) {
<div className="page-details">
<p className="bold-text">{createMessage(INVALID_URL_ERROR)}</p>
<p className="page-message">{createMessage(PAGE_NOT_FOUND_ERROR)}</p>
<p>Pathname: {history.location.pathname}</p>
<p>Search: {history.location.search}</p>
<p>Hash: {history.location.hash}</p>
<p>Action: {history.action}</p>
<Button
category={Category.secondary}
className="button-position"
Expand Down
73 changes: 66 additions & 7 deletions app/client/src/sagas/GitSyncSagas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,12 @@ import type { Workspace } from "@appsmith/constants/workspaceConstants";
import { log } from "loglevel";
import GIT_ERROR_CODES from "constants/GitErrorCodes";
import { builderURL } from "RouteBuilder";
import { APP_MODE } from "../entities/App";
import type { GitDiscardResponse } from "../reducers/uiReducers/gitSyncReducer";
import { APP_MODE } from "entities/App";
import type { GitDiscardResponse } from "reducers/uiReducers/gitSyncReducer";
import { FocusEntity, identifyEntityFromPath } from "navigation/FocusEntity";
import { getActions, getJSCollections } from "selectors/entitiesSelector";
import type { Action } from "entities/Action";
import type { JSCollectionDataState } from "reducers/entityReducers/jsActionsReducer";

export function* handleRepoLimitReachedError(response?: ApiResponse) {
const { responseMeta } = response || {};
Expand Down Expand Up @@ -316,7 +320,7 @@ function* updateGlobalGitConfig(action: ReduxAction<GitConfig>) {
const trimRemotePrefix = (branch: string) => branch.replace(/^origin\//, "");

function* switchBranch(action: ReduxAction<string>) {
let response: ApiResponse | undefined;
let response: ApiResponse<ApplicationPayload> | undefined;
try {
const branch = action.payload;
const applicationId: string = yield select(getCurrentApplicationId);
Expand All @@ -327,10 +331,65 @@ function* switchBranch(action: ReduxAction<string>) {
getLogToSentryFromResponse(response),
);

if (isValidResponse) {
const trimmedBranch = trimRemotePrefix(branch);
const updatedPath = addBranchParam(trimmedBranch);
history.push(updatedPath);
if (!response || !isValidResponse) {
return;
}

const trimmedBranch = trimRemotePrefix(branch);
const destinationHref = addBranchParam(trimmedBranch);

const entityInfo = identifyEntityFromPath(
destinationHref.slice(0, destinationHref.indexOf("?")),
);

// Check if page exists in the branch. If not, instead of 404, take them to
// the app home page
const page = response.data.pages.find(
(page) => page.id === entityInfo.pageId,
);
const homePage = response.data.pages.find((page) => page.isDefault);
if (!page) {
if (homePage) {
history.push(
builderURL({ pageId: homePage.id, branch: trimmedBranch }),
);
return;
}
}

// Page exists, so we will try to go to the destination
history.push(destinationHref);

let shouldGoToHomePage = false;
// It is possible that the action does not exist in the incoming branch
// so here instead of showing the 404 page, we will navigate them to the
// home page
if ([FocusEntity.API, FocusEntity.QUERY].includes(entityInfo.entity)) {
// Wait for fetch actions success, check if action id in actions state
// or else navigate to home
yield take(ReduxActionTypes.FETCH_ACTIONS_SUCCESS);
const actions: Action[] = yield select(getActions);
if (!actions.find((action) => action.id === entityInfo.id)) {
shouldGoToHomePage = true;
}
}

// Same for JS Objects
if (entityInfo.entity === FocusEntity.JS_OBJECT) {
yield take(ReduxActionTypes.FETCH_JS_ACTIONS_SUCCESS);
const jsActions: JSCollectionDataState = yield select(getJSCollections);
if (!jsActions.find((action) => action.config.id === entityInfo.id)) {
shouldGoToHomePage = true;
}
}

if (shouldGoToHomePage) {
if (homePage) {
// We will replace so that the user does not go back to the 404 url
history.replace(
builderURL({ pageId: homePage.id, persistExistingParams: true }),
);
}
}
} catch (e) {
// non api error
Expand Down

0 comments on commit 0094aab

Please sign in to comment.