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

Add "Due At" Column on Admin Invoice Overview #1992

Closed
wants to merge 9 commits into from
Closed

Add "Due At" Column on Admin Invoice Overview #1992

wants to merge 9 commits into from

Conversation

gilesytheking
Copy link

Added "due at" Column onto admin invoice overview. Tested and working on my staging site - Fixes request #1976

Could you confirm the date formatting in this file is correct - date(Y-m-d) or does it need to be updated to use format_date or format_datetime? I can update this accordingly.

image

On side note, whilst testing this change, I noticed that the invoice filters do not function correctly. Also not working on my live site in 0.6.4. I will create a separate issue for this.

Added column to show invoice due at in admin invoice overview
Add Invoice Due at on admin Invoice Overview
@BelleNottelling
Copy link
Member

If I'm not mistaken, all instances of date were supposed to be converted to format_date some time ago (excluding the installer).
@admdly can confirm if that's the case or not, though

@admdly
Copy link
Contributor

admdly commented Dec 19, 2023

If I'm not mistaken, all instances of date were supposed to be converted to format_date some time ago (excluding the installer). @admdly can confirm if that's the case or not, though

That's correct, all instances should use format_date or format_datetime depending on the context. We're just waiting on an upstream bug (twigphp/Twig#3845) being fixed for the changes to actually display correctly.

@John-S4
Copy link
Member

John-S4 commented Dec 19, 2023

As a side note on wording...

I would really prefer it to be 'Due' or 'Due by' than 'Due at', similarly the paid column should be 'Paid' or 'Paid on' not 'Paid at'. Actually the same goes for Issued.

@gilesytheking
Copy link
Author

gilesytheking commented Dec 19, 2023

@John-S4 @BelleNottelling @admdly - If you are all happy I will go ahead and update the following;

Change date formatting to format_date on admin -> invoice overview.

Update the column headers in admin -> invoice overview to - Invoice Date, Due Date & Paid Date. This follows the wording as shown on the client side.

Change date formatting to format_datetime on admin -> individual invoice view

Update wording on admin -> individual invoice to - Invoice Date, Due Date & Paid Date

Add due date column to admin -> Client -> Invoices & update column headers

Add column headers & due date column to admin Dashboard -> Invoices Widget

See below images from my staging site. if happy I'll update the pull request.

image

image

image

image

image

image

@admdly
Copy link
Contributor

admdly commented Dec 19, 2023

@gilesytheking sounds good to me. 🙂

@John-S4
Copy link
Member

John-S4 commented Dec 19, 2023

sounds good to me. 🙂

and me.

@gilesytheking
Copy link
Author

Pull request has been updated. There were a couple of other locations I found and have updated too.

image

image

@BelleNottelling
Copy link
Member

Personally I think that "Paid Date" is a somewhat unusual way to word it. I'd expect it to be "Due Date" and "Date Paid" or either of the matching pairs that @John-S4 originally stated in this message: #1992 (comment) (I'd probably prefer what John had originally said).

In general, I'd honestly suggest avoiding referencing most existing stuff as the "right" way to word things as there are still many things that are unchanged from BoxBilling which was generally full of typos, spelling mistakes, and very poor grammar

@admdly
Copy link
Contributor

admdly commented Dec 19, 2023

Personally I think that "Paid Date" is a somewhat unusual way to word it. I'd expect it to be "Due Date" and "Date Paid" or either of the matching pairs that @John-S4 originally stated in this message: #1992 (comment) (I'd probably prefer what John had originally said).

In general, I'd honestly suggest avoiding referencing most existing stuff as the "right" way to word things as there are still many things that are unchanged from BoxBilling which was generally full of typos, spelling mistakes, and very poor grammar

I agree "Paid Date" isn't quite right, but I'm impartial to the "x Date" convention - perhaps "Payment Date" as a compromise?

That said, I'm ambivalent about including the payment date in the tables at all. The paid status should be sufficient with the date included on the invoice details page; this appears to be how most others do it. Though, I fear this is edging into the realm of being out of scope and I'd rather not turn this rather simple PR into an entire review of how invoices/payments are displayed - so I will raise this separately if needed.

@BelleNottelling
Copy link
Member

BelleNottelling commented Dec 19, 2023

I agree "Paid Date" isn't quite right, but I'm impartial to the "x Date" convention - perhaps "Payment Date" as a compromise?

Yeah I don't love it either, that's why I said I'd prefer what John had originally suggested by using either "Due" and "Paid" or "Due by" and "Paid on"

That said, I'm ambivalent about including the payment date in the tables at all.

I'd probably be in favor of either remove the paid on column or condensing the two into one that displays either when an invoice is due or when it was paid depending on it's status. Neither date will be relevant at the same time and the less columns we have to display the better these pages will scale to smaller screens / devices.

If merging the two into one column, the status on the right would give the correct context to understand if the date displayed is when the invoice is due or when it was paid. If eliminating the paid on date entirely, the status still informs someone if it was paid and they can always get the specific date by going to the invoice itself rather than the overall list.

@gilesytheking
Copy link
Author

I agree with @admdly I think Payment Date would be a better fit. The X Date convention is commonly used in most Billing/Accounting type software.

I have reviewed a couple of other billing platforms. They don't seem to include the Payment date column in the overview and just use the payment status instead. The payment date column could be omitted completely, as the status shows if the invoice is paid or not.

image

However, if you want to keep the dates and combine them, as @BelleNottelling suggested, then possibly adding the date below the status is an option. This retains the visibility of the dates and keeps the column's down to a minimum.

I have used format_date here, so the time won't be shown once the upstream bug is fixed.

image

Let me know your thoughts so I can finalise the changes and update the pull request.

@BelleNottelling
Copy link
Member

I like both options with a slight preference for the 2nd one, personally. We get to display a little bit more info while also using up less horizontal screen space.
@gilesytheking

@admdly
Copy link
Contributor

admdly commented Dec 23, 2023

I agree with @admdly I think Payment Date would be a better fit. The X Date convention is commonly used in most Billing/Accounting type software.

I have reviewed a couple of other billing platforms. They don't seem to include the Payment date column in the overview and just use the payment status instead. The payment date column could be omitted completely, as the status shows if the invoice is paid or not.

image

However, if you want to keep the dates and combine them, as @BelleNottelling suggested, then possibly adding the date below the status is an option. This retains the visibility of the dates and keeps the column's down to a minimum.

I have used format_date here, so the time won't be shown once the upstream bug is fixed.

image

Let me know your thoughts so I can finalise the changes and update the pull request.

I'm happy with both options, like @BelleNottelling, however with a slight preference toward the first 😛 - for the simple reason that it enables sorting by due date and seems to be a common UI among billing systems.

@BelleNottelling
Copy link
Member

for the simple reason that it enables sorting by due date and seems to be a common UI among billing systems.

This is a good point, but because of pagination this really should be done via the DB query. I don't think this is currently implemented so I'd rather go for the first option for the time-being, but later down the line once the ability to sort it via the DB is added we could then switch to the 2nd option for the reduced horizontal size

Anuril added a commit to Anuril/FOSSBilling that referenced this pull request Jan 17, 2024
…l.twig and mod_invoice_invoice.html.twig
@gilesytheking gilesytheking closed this by deleting the head repository Jan 17, 2024
Anuril added a commit to Anuril/FOSSBilling that referenced this pull request Jan 17, 2024
…l.twig and mod_invoice_invoice.html.twig
@Anuril Anuril mentioned this pull request Jan 17, 2024
3 tasks
Anuril added a commit to Anuril/FOSSBilling that referenced this pull request Jun 20, 2024
…l.twig and mod_invoice_invoice.html.twig
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants