Crud invoice - #2558
Crud invoice#2558
Conversation
| class InvoicePresenter | ||
| ISSUER_NAME = "A Window Between Worlds".freeze | ||
| ISSUER_ADDRESS_LINES = [ "1029 1/2 W 24th St", "Los Angeles, CA 90007" ].freeze | ||
| ISSUER_EMAIL = "info@awbw.org".freeze |
There was a problem hiding this comment.
we don't want to use our env var here? and why info@ vs programs@ ?
also think this should be env var. i worry about emails in code esp in botlandia.
There was a problem hiding this comment.
I'm not sure the best email? Trainings said they do invoices for materials, is that programs or something else?
This was all copied for event_invoice presenter. I probably could have extracted that one and created some shared code but I didn't want to break anything with the event specific stuff. Worth a new issue to consider some shared env's or anything else that has crossover between the dynamic invoices vs db backed.
class EventInvoice
ISSUER_NAME = "A Window Between Worlds".freeze
ISSUER_ADDRESS_LINES = [ "1029 1/2 W 24th St", "Los Angeles, CA 90007" ].freeze
ISSUER_EMAIL = "info@awbw.org".freeze
PAYABLE_TO_NOTE = "Please make checks payable to A Window Between Worlds".freeze
Thoughts? I wouldn't love calling the constant from EventInvoice in this invoice presenter but I do agree on not duplicating it.
There was a problem hiding this comment.
@jmilljr24 they've worked it out internally to have everything route to one system email and forward as appropriate. my vote/request is we stick w that bc we already said supporting multiple emails is a feature request.
There was a problem hiding this comment.
You lost me on this one. Are you saying EventInvoice is wrong and needs to be updated as well? The screenshot of christy's current invoice uses info fyi. I have no stake in this game, I just don't understand the correlation to supporting multiple emails feature request. This is just displayed on the invoice - not connected our outgoing email system in the app.
Use info or use programs? Should I ask in Christy's thread?
There was a problem hiding this comment.
my vote was keep to same as the one email we service, bc next feature request will be to email the invoice.
yes, happy to have you ask in christy's thread. or, to just go w info@.
whichever email tho, i'm trying to avoid emails/contact info being in OSS code bc it'll increase spam to them and increase hack options.
| skip_before_action :preload_current_user_associations, raise: false | ||
| skip_verify_authorized | ||
|
|
||
| def lookup |
There was a problem hiding this comment.
why not support a dropdown select rather than only allowing access to one org address?
There was a problem hiding this comment.
For context, my though process on this whole feature was to try and keep it bare bones and lower the risking of having to make revisions if we make it over complex or lock fields in before any user feedback.
For the address section of the invoice, I wanted to keep it as a text field because there is a good possibility they want to add to/adjust this info. Normally I would want a real db association but for address I can't see any time soon where we need to know the address record associated with an invoice. The person/organization is important so I kept those fields via id.
It's certainly possible and not a huge lift to 1. check the selected invoicee 2. insert a address dropdown 3. on select of address fill in text field.
It's just getting to in the weeds when they can simply just type an address if different than the primary especially if they are not doing this every single day. Or take the primary and add extra info after if if the invoicee needs it for their records. I little work on there end is ok until it becomes a big problem was my thinking.
There was a problem hiding this comment.
we already have AWBW as an org and all orgs have multiple addresses. then we get out of contact info in code or them writing text here that isn't saved in their db. but, i'm fine w us going w what you've done.
| <% content_for(:page_title, "Edit Invoice") %> | ||
| <% content_for(:page_bg_class, "admin-only bg-blue-100") %> | ||
|
|
||
| <div class="mx-auto max-w-3xl"> |
There was a problem hiding this comment.
would love for there to be a card for page content. happy to do as follow-on later.
| <% end %> | ||
| </div> | ||
|
|
||
| <table class="w-full text-sm"> |
There was a problem hiding this comment.
would love for there to be a card for table content. happy to do as follow-on later to make it look like the other indexes.
There was a problem hiding this comment.
For sure! I left ui bare bones. I want to just get the functionally so we can check the box for moving off filemaker features.
maebeale
left a comment
There was a problem hiding this comment.
ty for hopping on this one!
|
i can run a features.yml update after we merge this. |
da5044a to
49461f0
Compare
Closes [link an issue or remove this line]
What is the goal of this PR and why is this important?
Stakeholders require creating one off invoices. Adding db backed invoices allows CRUD and give the ability to import fm data if required.
How did you approach the change?
UI Testing Checklist
Anything else to add?