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.

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

Jan development #431

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

Pookie0717
Copy link

Integrated API to get data of tenants

Copy link
Contributor

@abouolia abouolia left a comment

Choose a reason for hiding this comment

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

First of all thanks for contribution, since I'm the main maintainer of Bigcapital, wanted to give some early notes just to keep the feedback frequency early to maintain the code consistency and be able to merge the PR later.

@@ -0,0 +1,433 @@
/*! normalize.css v8.0.1 | MIT License | github.com/necolas/normalize.css */
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid adding these kind of files in the PR since not related to it.

@@ -526,6 +526,10 @@ html[lang^=en] body {
border-top: 3px double #666;
font-weight: bold;
}
.invoice__table-total table tbody tr.total td {
Copy link
Contributor

Choose a reason for hiding this comment

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

That kind of change would be better if it was in another PR.

exports.up = function (knex) {
return knex.schema.createTable('user_tenants', (table) => {
table.increments();
table.integer('user_id');
Copy link
Contributor

Choose a reason for hiding this comment

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

No database relation to users and tenants table.

const tenant = await Tenant.query()
.findById(user.tenantId)
.withGraphFetched('metadata');
const tenant = await UserTenants.query()
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of exposing all the tenants data in sign-in response would be better to add another endpoint retrieves all associated tenants to the current authenticated user.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants