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

Display all request groups names in tag editor drop down #1293

Merged
merged 1 commit into from Dec 10, 2018

Conversation

Projects
None yet
3 participants
@rickychandra
Copy link
Contributor

rickychandra commented Dec 6, 2018

Closes: #1286
Added all the folders (request groups) names as prefix in tag editor dialog drop down.

requests
Request list

dropdown
Dropdown

let name = typeof reqGroup.name === 'string' ? reqGroup.name : '';
prefix = `[${name}] ` + prefix;
requestGroupId = reqGroup.parentId;
} while (true);

This comment has been minimized.

@TeeSeal

TeeSeal Dec 6, 2018

Perhaps you can do a while (requestGroupId) {} loop instead?
In that case you could remove the if (reqGroup == null) break; entirely.

This will only work if reqGroup.parentId returns a falsy value when there is no parent.

This comment has been minimized.

@gschier

gschier Dec 6, 2018

Collaborator

There's actually a handy method to get all the ancestors of a given DB model. This seems like the perfect application for that function.

export const withAncestors = (database.withAncestors = async function(

// Sample usage from network.js
const ancestors = await db.withAncestors(request, [
    models.request.type,
    models.requestGroup.type,
    models.workspace.type
]);

This comment has been minimized.

@rickychandra

rickychandra Dec 7, 2018

Contributor

@gschier Using db.withAncestors() will return Promise and require the caller function to use async/await up until the render() function, which won't work.
If your concern is code reusability, what if we refactor it to a function getAncestorRequestGroups(requestGroupId: string, allRequestGroups: Array<RequestGroup>): Array<RequestGroup>)? This function will recursively get all request groups starting from requestGroupId, and it can be put in app/common/misc.js.
Another caveat of using db.withAncestors() in this case is that there will be many calls to the db (IO operations). Although it may still be performant, I think we could do better by reusing data already fetched (allDocs variable).
What do you think? 😄

This comment has been minimized.

@gschier

gschier Dec 10, 2018

Collaborator

Ah yes, I didn't realize it was being called from render. I don't really care much about reusability here. I'm fine with what you have 👍

P.S. the DB is in-memory so there is no I/O.

let name = typeof reqGroup.name === 'string' ? reqGroup.name : '';
prefix = `[${name}] ` + prefix;
requestGroupId = reqGroup.parentId;
} while (true);

This comment has been minimized.

@gschier

gschier Dec 6, 2018

Collaborator

There's actually a handy method to get all the ancestors of a given DB model. This seems like the perfect application for that function.

export const withAncestors = (database.withAncestors = async function(

// Sample usage from network.js
const ancestors = await db.withAncestors(request, [
    models.request.type,
    models.requestGroup.type,
    models.workspace.type
]);
@gschier
Copy link
Collaborator

gschier left a comment

Nice work! Merging this in

let name = typeof reqGroup.name === 'string' ? reqGroup.name : '';
prefix = `[${name}] ` + prefix;
requestGroupId = reqGroup.parentId;
} while (true);

This comment has been minimized.

@gschier

gschier Dec 10, 2018

Collaborator

Ah yes, I didn't realize it was being called from render. I don't really care much about reusability here. I'm fine with what you have 👍

P.S. the DB is in-memory so there is no I/O.

@gschier gschier merged commit 2b971ae into getinsomnia:develop Dec 10, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rickychandra rickychandra deleted the rickychandra:improvement/request-group-prefix branch Dec 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment