openfoodfoundation / openfoodnetwork

Connect suppliers, distributors and consumers to trade local produce.
https://www.openfoodnetwork.org
GNU Affero General Public License v3.0
1.12k stars 724 forks source link

As a super admin, I should see a field I can use to link enterprise with my invoicing software #12942

Open RachL opened 1 month ago

RachL commented 1 month ago

⚠️ when working on this use #12942 Invoicing ID clockify code

Context : an OFN instance often needs to invoice one entity for several hubs. It's not simple to make accounting software holding that info.

This issue gives the ability to instance manager to store the ID their accounting software uses to invoice enterprise on the OFN enterprise profile.

Description

- As a: super admin - On page: /admin/enterprises/XXXX/edit and /admin/reports/revenues_by_hub - I want to be able to do:

As a super admin, I want to see a new field where I can put an ID. This ID can be a short combination of text (lower or upper caps)+number. What's important is that spaces are not allowed. E.g. : CU2410-00190.

Any other white-space characters too (eg tabs), using this handy tip.

Several enterprise can have the same ID.

This ID is only visible by super admins: enterprise owner / managers do not see them.

This ID should be used in the report that is only available for super admin called /admin/reports/revenues_by_hub

That means that super admin can use it in the report, the enterprises table in the database or the V0 API of this report.

Enterprise profile :

image

what's this? text proposal:

You can use this field to add the ID of your invoicing software, making it easier to calculate turnover per entity you are invoicing.

Report columns:

image

Acceptance Criteria & Tests

  1. Check you can add an ID on an enterprise profile
  2. This ID can be found on the revenue by hub report
  3. This ID is available on v0 API / report section, as well as the enterprise database
  4. Super admin can change the ID as many times as they want
  5. Super admin can't add spaces to this ID
  6. Several enterprises can have the same ID
  7. Enterprise owner / managers don't see this field
BethanOFN commented 1 month ago

Can invoicing id field also be added to the enterprises table in the database, for access via sql query?

RachL commented 1 month ago

yes!

pacodelaluna commented 3 weeks ago

@RachL I can take a look at this issue, when it is ready for development.

dacook commented 3 weeks ago

This new field is only visible to certain permission level (super admin), so I suggest we should display it separately from other 'normal' fields. The clearest way to do this would be to add it to a new LHS tab (something like "Admin settings"), which only appears for super admins. But there are already so many tabs, so maybe we don't want to clutter the screen so much! So another option would be to add a new section in the Primary Details tab (with red text and grey divider line). That should be quick and easy. What do you think Rachel?

As for the format validation, I assume this is to avoid input errors? I think that's a good idea, and would avoid being too restrictive, in case there is a need for special characters. So the suggestion of limiting spaces (and allowing everything else) seems most sensible 👍. I would avoid any other white-space characters too (eg tabs), using this handy tip.

RachL commented 3 weeks ago

This new field is only visible to certain permission level (super admin), so I suggest we should display it separately from other 'normal' fields. [...] So another option would be to add a new section in the Primary Details tab (with red text and grey divider line). That should be quick and easy. What do you think Rachel?

@dacook why not, I don't have a strong opinion. Note that currently this tab already displays info for super admin only (the radio buttons for "sells" and "visible in search ?") are shown only to super admin.

As for the format validation, I assume this is to avoid input errors?

yes I will rephrase to say that we want especially to not allow spaces.

dacook commented 3 weeks ago

Note that currently this tab already displays info for super admin only (the radio buttons for "sells" and "visible in search ?") are shown only to super admin.

I didn't realise that! Actually I just checked, and I think it's only the "Sells" field that is limited to super admin.

In this case, I suggest creating a new section at the bottom of the Primary Details tab, named something like "Admin Only". We can move "Sells", and this new "Client ID" under it. If you agree, I think we are ready for development!

(As a side note, it's a bit confusing that we don't have clearer nomenclature to differentiate between an Enterprise Admin and Instance Admin...)

pacodelaluna commented 3 weeks ago

@dacook If you want, I can go on with the development. Your inputs are pretty clear. Maybe the last thing that we need to agree on is the name of this new field, client_id is good, but I was thinking it could be mismatched for a foreign key, no? I would suggest external_billing_uuid, but it may not be consistent with other namings, it is just an idea, better to be clear about it now.

About the steps to proceed:

  1. Add the field on the table in a new migration
  2. Add a validation on enterprise model as you precised it (Everything but spaces), with Rspec tests
  3. Place the field on the admin form for enterprises as you mentioned it (The renaming of Sells tab to Admin Only, with the new field)
  4. Add the field on the RevenueByHub report, with Rspec tests

Please tell me what you think about it, it is just a proposition. Or anyway, tell me how I can help you on this topic.

dacook commented 2 weeks ago

Thanks François. Regarding the field name, I'm not sure of the best approach. I see what you mean about it looking like a foreign key. But I think uuid is not quite correct: generally UUIDs are very large, designed to avoid conflicts, but this field is intended to hold any kind of IDs from any system. So I think adding external is enough to indicate it's from an external system. How about external_billing_id?

Regarding all the other steps, yes that all looks good to me. I would just add: it would be good to include the new client/billing ID in a system (browser) spec also, just confirming that the field can be saved. I think it could be added to the test "editing an existing enterprise".

dacook commented 2 weeks ago

FYI @RachL I don't see a Clockify timer for this project in AU Clockify, so I've just filed a small amount of time under General Dev (with the ID and name of this Issue).

RachL commented 2 weeks ago

@dacook it seems then that I don't have the rights to add it to the AU clockify account :( can you duplicate the code I've added on the global account?

dacook commented 2 weeks ago

It seems I can, yes! Have done now.