topcoder-platform / admin-app

Customer support application
7 stars 25 forks source link

Billing Account Admin View #46

Closed ajefts closed 7 years ago

ajefts commented 7 years ago

We need to add a view in the admin app to manage customer records, billing accounts, and resources on billing accounts.

This will manage the following tables in the time_oltp database:

client project project_manager

The API will handle the DB interactions, and @skyhit is managing that part. For this story, we need the UI built within the admin tool. Here is what an admin user needs to be able to do:

  1. Create a new client
  2. Edit a client
  3. Create a new billing account
  4. Edit a billing account
  5. Add a resource to a billing account
  6. Remove a resource from a billing account

Client properties:

Billing account properties:

Resource properties:

In the admin tool, admins should be able to manage the above objects as well as the relationships between then.

Admin should be able to search and filter billing accounts easily by client name, start date, end date, status, billing account name, subscription number, user

huangqun commented 7 years ago

@ajefts Should we start on this after the API is ready? Or should we start now and build some mock API for testing?

ajefts commented 7 years ago

Once @skyhit has the API mocks, let's get moving on this one. If you can do any pre-work, please do. Thanks.

huangqun commented 7 years ago

@skyhit do you have any mocks I can take a look now?

skyhit commented 7 years ago

@huangqun, we only have the swagger api spec yet - https://github.com/appirio-tech/tc-billing-account-service

we are going to launch challenges to get the mock apis implemented, so you can get start for integration

huangqun commented 7 years ago

@skyhit is the api mock done yet?

skyhit commented 7 years ago

@huangqun yes, are you able to access https://software.topcoder.com/review/actions/ViewProjectDetails?pid=30056272 ?

huangqun commented 7 years ago

@skyhit no I don't have access to it, could you please grant me access?

ajefts commented 7 years ago

You should have access now.

On Tue, Jan 31, 2017 at 10:29 PM, huangqun notifications@github.com wrote:

@skyhit https://github.com/skyhit no I don't have access to it, could you please grant me access?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/appirio-tech/admin-app/issues/46#issuecomment-276563631, or mute the thread https://github.com/notifications/unsubscribe-auth/ADorrr_ZyfIdUmCTEGdnXbRdLCiiqBiLks5rX_wQgaJpZM4LpaW9 .

huangqun commented 7 years ago

@skyhit I checked the mock API and have a question: why don't the billing account users API require authentication? Does that mean anyone can create normal / admin users? Or are these APIs not supposed to be used at all (i.e. only created to test the other billing account API endpoints)?

ajefts commented 7 years ago

These should require auth, so if we missed that then let's get it updated...

Also, I think we missed the PO Number on the billing account object. We'll need to add that as well.

huangqun commented 7 years ago

I think there are some other things missing (or not clear to me):

  1. What are the endpoints to create / edit a client? Can't find this client object in the mock api either.
  2. These fields are missing from the billing account object: description, po number, payment terms, subscription number
  3. In the endpoints to add / delete user to / from billing account, there's only user id but not status, is this a mistake in the API or do we not need the status field?
ajefts commented 7 years ago

I think you're right on all points.

@skyhit Can you take a look and run an update?

skyhit commented 7 years ago

@ajefts @huangqun

from the submissions, the billing account users API is created for testing, not from the original spec

- Additionally I created an endpoint to create users.
This was not requested in challenge spec but it's required to get valid IDs and assign them to Billing Accounts.

for What are the endpoints to create / edit a client? Can't find this client object in the mock api either., we didn't manage client in this api set

These fields are missing from the billing account object: description, po number, payment terms, subscription number

yes, we can add these fields for mock and real implementation.

skyhit commented 7 years ago

@ajefts should we create a mock directory for saving the mock apis? so we can constantly update that?

skyhit commented 7 years ago

@ajefts @huangqun here is the challenge spec - http://www.topcoder.com/direct/contest/detail?projectId=30056392 anything missing? if looks good, I can launch it right away!

huangqun commented 7 years ago

@skyhit the only part I'm not sure about is this:

In the endpoints to add / delete user to / from billing account, there's only user id but not status, is this a mistake in the API or do we not need the status field?

skyhit commented 7 years ago

@ajefts @huangqun I checked project_manager table, there is only a active field - https://github.com/appirio-tech/tc-database-scripts/blob/master/time_oltp/01_time_oltp_main_schema.sql#L259,

so for deletion, we just set it inactive, and by default, set it active, so status property, seems useless.

skyhit commented 7 years ago

@ajefts @huangqun the mock service is updated and ready, please check https://github.com/appirio-tech/tc-billing-account-service/pull/10

if you need further changes, let me know.

huangqun commented 7 years ago

@ajefts looks like it's not merged yet, could you please accept the PR so I can pull the latest complete code?

Thanks!

ajefts commented 7 years ago

it's merged now @huangqun . thanks.

huangqun commented 7 years ago

@ajefts this will be done in https://www.topcoder.com/direct/contest/detail.action?projectId=30056412

ajefts commented 7 years ago

Thanks

On Tue, Feb 7, 2017 at 9:00 PM, huangqun notifications@github.com wrote:

@ajefts https://github.com/ajefts this will be done in https://www.topcoder.com/direct/contest/detail.action?projectId=30056412

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/appirio-tech/admin-app/issues/46#issuecomment-278206365, or mute the thread https://github.com/notifications/unsubscribe-auth/ADorrn2d2zdWZ0A8aQ1hz9HO48qET8Cwks5raSG-gaJpZM4LpaW9 .

huangqun commented 7 years ago

@ajefts the requirement says "we should be able to search and filter billing accounts easily by client name", but looking at the api and data objects, billing account is not associated with client in anyway, was there a mistake in the api or objects?

huangqun commented 7 years ago

Does customerNumber in the BillingAccount object refer to client name? In this case, should we assume client name is unique?

huangqun commented 7 years ago

@ajefts any comments on my previous questions?

ajefts commented 7 years ago

@huangqun Sorry, I missed these.

  1. There should be a relationship between clients and billing accounts. This relationship is defined by the client_project table. For example, https://github.com/appirio-tech/tc-api/blob/master/queries/get_active_billing_accounts lists all the active billing accounts and includes the client for each billing account.

  2. customer_number is different than client_name. Think of customer_number like an external reference. Currently, it stores a code that identifies the customer in our salesforce org. customer_number should be unique.

huangqun commented 7 years ago

@ajefts @skyhit in this case I think we need to update the mock billing service to fix such issues, these were not addressed in the mock api.

skyhit commented 7 years ago

maybe we should use the real apis,it addresses the problem

huangqun commented 7 years ago

@skyhit is there a test environment that the members can use for testing?

skyhit commented 7 years ago

@huangqun it is ready in dev, but I am not sure if we can expose to members.

for local setups, you can check https://github.com/topcoder-platform/tc-billing-account-service

ajefts commented 7 years ago

@huangqun How are we doing on this? Are we blocked?

huangqun commented 7 years ago

@ajefts this part is done but it looks like the billing service is not deployed on dev server yet? I'm able to test the UI with local mock api but not able to test it when connected to dev server, or is there some special url for the billing account service?

ajefts commented 7 years ago

@huangqun ok, I see the dev issue. I need to update nginx routes since we rename the /v3/billing-accounts. I'm doing that now, so dev should work in about 10 minutes...

ajefts commented 7 years ago

it's working in dev now. For example, https://api.topcoder-dev.com/v3/billing-accounts/1

huangqun commented 7 years ago

@ajefts is the endpoint for clients api this?

https://api.topcoder-dev.com/v3/clients

ajefts commented 7 years ago

Yes

On Thu, Feb 23, 2017 at 7:41 AM, huangqun notifications@github.com wrote:

@ajefts https://github.com/ajefts is the endpoint for clients api this?

https://api.topcoder-dev.com/v3/clients

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/appirio-tech/admin-app/issues/46#issuecomment-281981768, or mute the thread https://github.com/notifications/unsubscribe-auth/ADorrvGulPRxxzlQX4NXQiSiEjdHu9Mrks5rfX57gaJpZM4LpaW9 .

huangqun commented 7 years ago

@ajefts @skyhit it looks like the billing account api is not working properly, when we try to call it with valid request, I get this error in the response:

image

huangqun commented 7 years ago

BTW, this is the "POST /clients" endpoint.

huangqun commented 7 years ago

@ajefts @skyhit seeing more issues with the billing account api as we do more testing, not sure if it's deployment issue or bugs in the code:

  1. The sequence error stated above
  2. Got the following error when trying to filter billing accounts using the api, this happens when trying to filter by customer id: image
  3. api is not saving payment terms when we update a billing account. It's actually very strange how it's handled in the api right now, in the billing account entity it seems to be a number but it doesn't have a foreign key constraint even though there's a payment terms table.

There might be more, I'll keep you updated.

ajefts commented 7 years ago

@skyhit Can you take a look please? I see some functions in that sql that might not be available in our informix instance....

huangqun commented 7 years ago

@skyhit any updates on the API issues? @ajefts sent another PR with some fixes, like loading animation, saving updates without entering code name, etc.

skyhit commented 7 years ago

@ajefts what informix version are we using? is it related to DBINFO and RTRIM functions?

like DBINFO and TRIM functions are only available after informix 11.50 - https://www.ibm.com/support/knowledgecenter/en/SSGU8G_11.50.0/com.ibm.sqls.doc/ids_sqs_1484.htm

skyhit commented 7 years ago

@huangqun can you provide what request are you sending and how you send it?

and this problem is only related to customer filtering?

huangqun commented 7 years ago

@skyhit which issue were you referring to? There were multiple issues.

Also perhaps it's easier if you just run the dev branch of admin-app repo locally to try it, just run 'npm install' -> 'bower install' -> 'gulp build' -> 'gulp serve' and it will launch.

Then try the Clients / Billing Accounts page.

skyhit commented 7 years ago

@huangqun I want to see what is the actually url and parameters sent out to the tc-billing-account api, so I can minic the same request and identify the problem.

by the way, for current implementation customer filtering is not supported, but it will simply ignored if passed.

ajefts commented 7 years ago

Hi @huangqun,

I deployed the current version to prod to test out the performance. We definitely need to add the paging since we have a pretty big billing account list and it takes a while to load the page. We can also probably default to load only active billing accounts in filter. I'll create another ticket...

thanks.

ajefts commented 7 years ago

@skyhit

"...what informix version are we using? is it related to DBINFO and RTRIM functions?

like DBINFO and TRIM functions are only available after informix 11.50 - https://www.ibm.com/support/knowledgecenter/en/SSGU8G_11.50.0/com.ibm.sqls.doc/ids_sqs_1484.htm"

I think it's just the DBINFO function that we don't have. Can we rewrite that part of the query?

For the sequence error, what is the name of the sequence it's trying to use?

skyhit commented 7 years ago

@ajefts we used BillingAccount.class.getName() in code, looks like there is no such existing sequence in db. or we should create a sequence for billing_account.

skyhit commented 7 years ago

@ajefts check this - https://github.com/topcoder-platform/tc-billing-account-service/blob/dev/doc/test_data.sql

ajefts commented 7 years ago

@skyhit We should use the existing sequences that are already being used. It looks like they are:

com.topcoder.timetracker.ClientManager com.topcoder.timetracker.ProjectManager com.topcoder.timetracker.user.User