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

New fields for acquisitions #50

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

veggiematts
Copy link
Contributor

Currently, the enhanced acquisitions in the resources module lacks the ability to specify a tax rate for acquisitions.

This patch adds a tax excluded, tax rate, tax included and fund code field.

Price Tax Included and Payment Amount will be automatically prefilled according to the Price Tax Excluded and Tax Rate values.

acq1

acq2

 - Add new fields to the enhanced acquisitions mode:
   - Fund Code
   - Price Tax Excluded
   - Tax Rate
   - Price Tax Included

 Price Tax Included and Payment Amount will be automatically prefilled accoring
 to the Price Tax Excluded and Tax Rate values.
@benheet
Copy link

benheet commented Jun 24, 2015

Where did the request for these new fields come from? We've discussed not having any new pull requests come in for things like this without first having discussion around the requested new features.

@benheet
Copy link

benheet commented Jun 24, 2015

I'd have to go back and read up on the discussions we had with Fenway Libraries in order to see what their thoughts were about the use of tax and fund codes. There is already a fund field, i don't know if we need one called fund code.

@veggiematts
Copy link
Contributor Author

Hi, sorry about that. This patch was made quite some times ago, probably before the discussion you've mentionned. I made it while the enhanced acquisition branch was being developped by @nkuitse .

At the time, I made a pull request directly to the dev branch : fenway-library-organization#1 , which was not commented.

Now that the new enhanced aquisition branch has been merged to master, I just rebased my patch and created a new pull request.

We can discuss the usefulness of these new fields, of course.

Being able to specify a tax rate (which may be different from one acquisition to another) seems very relevant to me, for instance.

@benheet
Copy link

benheet commented Jun 29, 2015

Thank you @veggiematts The timing makes sense now.

@benheet
Copy link

benheet commented Sep 17, 2015

@veggiematts Could you comment a bit please on the tax rate, tax included and tax excluded fields? Would you describe in a bit more detail how they work and what they are used for?

@benheet
Copy link

benheet commented Sep 17, 2015

@veggiematts An initial reaction is that the fund code field may not be necessary. both fund and fund code allow a free text use allowing folks to use either names or codes. I'm not sure that we need the ability to record both in separate fields within coral. Can you comment please on how strongly you feel that both fields are needed?

@veggiematts
Copy link
Contributor Author

As tax rate may vary between acquisitions, it was mandatory to be able to specify it on a per-order basis, and not globally for all acquisitions.

Once the user has filled the tax excluded and tax rate fields, the tax included amount is automatically computed and prefilled.

@veggiematts
Copy link
Contributor Author

About the fund code field, our customer has several funds grouped in categories. They use the fund code to specify which fund group the acquisition is about.

Let me know if you think that there is a more relevant way to keep this kind of information.

@PaulPoulain
Copy link
Contributor

Discussion during steering committee:

  • split fund & tax things in 2 different pull requests
  • move forward with taxes, everybody is OK
  • improve display / user interface : there are already too many columns, we must improve to keep readability

@PaulPoulain
Copy link
Contributor

Matthias: is this PR still needed ? don't you have to create another one with the "split fund" stuff ?

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.

3 participants