Skip to content

Commit

Permalink
Fix: Version creation on empty/invalid version from id (ToolJet#2510)
Browse files Browse the repository at this point in the history 
* fix version creation on empty/invalid version from id

* fix spec

* hotfix: Edited changes not shown when switching versions (ToolJet#2386)

* fix edited changes not shown when switching versions

* fix version create from not picking editing version

* fix version switch not updated on editing version save
  • Loading branch information
@akshaysasidrn
akshaysasidrn authored Mar 15, 2022
1 parent 70379d4 commit 841710a
Show file tree
Hide file tree
Showing 4 changed files with 104 additions and 24 deletions.
11 changes: 11 additions & 0 deletions frontend/src/Editor/AppVersionsManager.jsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,17 @@ export const AppVersionsManager = function AppVersionsManager({
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

useEffect(() => {
setEditingAppVersion(editingVersion);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [editingVersion]);

useEffect(() => {
appVersions[appVersions.findIndex((appVersion) => appVersion.id === editingVersion.id)] = editingAppVersion;
setCreateAppVersionFrom(editingAppVersion);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [editingAppVersion]);

const wrapperRef = useRef(null);
useEffect(() => {
const handleClickOutside = (event) => {
Expand Down
9 changes: 8 additions & 1 deletion frontend/src/Editor/Editor.jsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -827,7 +827,14 @@ class Editor extends React.Component {
} else if (!isEmpty(this.state.editingVersion)) {
this.setState({ isSavingEditingVersion: true, showSaveDetail: true });
appVersionService.save(this.state.appId, this.state.editingVersion.id, this.state.appDefinition).then(() => {
this.setState({ isSavingEditingVersion: false });
this.setState({
isSavingEditingVersion: false,
editingVersion: {
...this.state.editingVersion,
...{ definition: this.state.appDefinition },
},
});

setTimeout(() => this.setState({ showSaveDetail: false }), 3000);
});
}
Expand Down
34 changes: 22 additions & 12 deletions server/src/services/apps.service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ export class AppsService {
}

async createVersion(user: User, app: App, versionName: string, versionFromId: string): Promise<AppVersion> {
const lastVersion = await this.appVersionsRepository.findOne({
const versionFrom = await this.appVersionsRepository.findOne({
where: { id: versionFromId },
});

Expand All @@ -299,12 +299,12 @@ export class AppsService {
manager.create(AppVersion, {
name: versionName,
app,
definition: lastVersion?.definition,
definition: versionFrom?.definition,
createdAt: new Date(),
updatedAt: new Date(),
})
);
await this.setupDataSourcesAndQueriesForVersion(manager, appVersion, lastVersion);
await this.setupDataSourcesAndQueriesForVersion(manager, appVersion, versionFrom);
});

return appVersion;
Expand All @@ -320,24 +320,34 @@ export class AppsService {
await getManager().transaction(async (manager) => {
await manager.delete(DataSource, { appVersionId: version.id });
await manager.delete(DataQuery, { appVersionId: version.id });
result = await manager.delete(AppVersion, { id: version.id, appId: app.id });
result = await manager.delete(AppVersion, {
id: version.id,
appId: app.id,
});
});

return result;
}

async setupDataSourcesAndQueriesForVersion(manager: EntityManager, appVersion: AppVersion, lastVersion: AppVersion) {
if (lastVersion) {
await this.createNewDataSourcesAndQueriesForVersion(manager, appVersion, lastVersion);
async setupDataSourcesAndQueriesForVersion(manager: EntityManager, appVersion: AppVersion, versionFrom: AppVersion) {
if (versionFrom) {
await this.createNewDataSourcesAndQueriesForVersion(manager, appVersion, versionFrom);
} else {
// TODO: Remove this when default version will be create when app creation is done
const totalVersions = await manager.count(AppVersion, {
where: { appId: appVersion.appId },
});

if (totalVersions > 1) {
throw new BadRequestException('More than one version found. Version to create from not specified.');
}
await this.associateExistingDataSourceAndQueriesToVersion(manager, appVersion);
}
}

async associateExistingDataSourceAndQueriesToVersion(manager: EntityManager, appVersion: AppVersion) {
const dataSources = await manager.find(DataSource, {
where: { appId: appVersion.appId },
where: { appId: appVersion.appId, appVersionId: null },
});
for await (const dataSource of dataSources) {
await manager.update(DataSource, dataSource.id, {
Expand All @@ -346,7 +356,7 @@ export class AppsService {
}

const dataQueries = await manager.find(DataQuery, {
where: { appId: appVersion.appId },
where: { appId: appVersion.appId, appVersionId: null },
});
for await (const dataQuery of dataQueries) {
await manager.update(DataQuery, dataQuery.id, {
Expand All @@ -358,13 +368,13 @@ export class AppsService {
async createNewDataSourcesAndQueriesForVersion(
manager: EntityManager,
appVersion: AppVersion,
lastVersion: AppVersion
versionFrom: AppVersion
) {
const oldDataSourceToNewMapping = {};
const oldDataQueryToNewMapping = {};

const dataSources = await manager.find(DataSource, {
where: { appVersionId: lastVersion.id },
where: { appVersionId: versionFrom.id },
});

for await (const dataSource of dataSources) {
Expand All @@ -384,7 +394,7 @@ export class AppsService {
}

const dataQueries = await manager.find(DataQuery, {
where: { appVersionId: lastVersion.id },
where: { appVersionId: versionFrom.id },
});
const newDataQueries = [];
for await (const dataQuery of dataQueries) {
Expand Down
74 changes: 63 additions & 11 deletions server/test/controllers/apps.e2e-spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,10 @@ describe('apps controller', () => {
});
const organization = adminUserData.organization;
const allUserGroup = await getManager().findOne(GroupPermission, {
where: { group: 'all_users', organization: adminUserData.organization },
where: {
group: 'all_users',
organization: adminUserData.organization,
},
});
const developerUserData = await createUser(app, {
email: 'developer@tooljet.io',
Expand Down Expand Up @@ -765,7 +768,7 @@ describe('apps controller', () => {
const application = await createApplication(app, {
user: adminUserData.user,
});

const version = await createApplicationVersion(app, application);
// setup app permissions for developer
const developerUserGroup = await getRepository(GroupPermission).findOne({
where: {
Expand All @@ -784,6 +787,7 @@ describe('apps controller', () => {
.set('Authorization', authHeaderForUser(userData.user))
.send({
versionName: 'v1',
versionFromId: version.id,
});

expect(response.statusCode).toBe(201);
Expand Down Expand Up @@ -812,7 +816,9 @@ describe('apps controller', () => {

expect(response.statusCode).toBe(201);

const v2 = await getManager().findOne(AppVersion, { where: { name: 'v2' } });
const v2 = await getManager().findOne(AppVersion, {
where: { name: 'v2' },
});
expect(v2.definition).toEqual(v1.definition);
});

Expand Down Expand Up @@ -947,40 +953,86 @@ describe('apps controller', () => {
expect(dataQueries).toHaveLength(3);
expect(dataSources.map((s) => s.appVersionId).includes(response.body.id)).toBeTruthy();
expect(dataQueries.map((q) => q.appVersionId).includes(response.body.id)).toBeTruthy();

// creating a new version from a non existing version id will throw error when more than 1 versions exist
await createDataSource(app, {
name: 'name',
kind: 'postgres',
application: application,
user: adminUserData.user,
});
await createDataQuery(app, {
application,
dataSource,
kind: 'restapi',
options: { method: 'get' },
});

response = await request(app.getHttpServer())
.post(`/api/apps/${application.id}/versions`)
.set('Authorization', authHeaderForUser(adminUserData.user))
.send({
versionName: 'v3',
versionFromId: 'a77b051a-dd48-4633-a01f-089a845d5f88',
});

expect(response.statusCode).toBe(400);
expect(response.body.message).toBe('More than one version found. Version to create from not specified.');
});

it('creates new credentials and copies cipher text on data source', async () => {
const adminUserData = await createUser(app, {
email: 'admin@tooljet.io',
});
const application = await importAppFromTemplates(app, adminUserData.user, 'customer-dashboard');
const dataSource = await getManager().findOne(DataSource, { where: { appId: application } });
const dataSource = await getManager().findOne(DataSource, {
where: { appId: application },
});

let dataSources = await getManager().find(DataSource);
let dataQueries = await getManager().find(DataQuery);
const credential = await getManager().findOne(Credential, {
where: { id: dataSource.options['password']['credential_id'] },
});
credential.valueCiphertext = 'strongPassword';
await getManager().save(credential);

const response = await request(app.getHttpServer())
let response = await request(app.getHttpServer())
.post(`/api/apps/${application.id}/versions`)
.set('Authorization', authHeaderForUser(adminUserData.user))
.send({
versionName: 'v1',
});

expect(response.statusCode).toBe(400);
expect(response.body.message).toBe('More than one version found. Version to create from not specified.');

const initialVersion = await getManager().findOne(AppVersion, {
where: { appId: application.id, name: 'v0' },
});

response = await request(app.getHttpServer())
.post(`/api/apps/${application.id}/versions`)
.set('Authorization', authHeaderForUser(adminUserData.user))
.send({
versionName: 'v1',
versionFromId: initialVersion.id,
});

await request(app.getHttpServer())
expect(response.statusCode).toBe(201);

response = await request(app.getHttpServer())
.post(`/api/apps/${application.id}/versions`)
.set('Authorization', authHeaderForUser(adminUserData.user))
.send({
versionName: 'v2',
versionFromId: response.body.id,
});
dataSources = await getManager().find(DataSource);
dataQueries = await getManager().find(DataQuery);

const dataSources = await getManager().find(DataSource);
const dataQueries = await getManager().find(DataQuery);

expect(dataSources).toHaveLength(2);
expect(dataQueries).toHaveLength(4);
expect(dataSources).toHaveLength(3);
expect(dataQueries).toHaveLength(6);

const credentials = await getManager().find(Credential);
expect([...new Set(credentials.map((c) => c.valueCiphertext))]).toEqual(['strongPassword']);
Expand Down

0 comments on commit 841710a

Please sign in to comment.