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

remove billing confirmation #2404

Merged
merged 7 commits into from
Jul 5, 2023
Merged

Conversation

Steve-Mcl
Copy link
Contributor

@Steve-Mcl Steve-Mcl commented Jul 4, 2023

Description

  • Removes the confirmation Check box from instances AND devices
  • Moves additional costs to the costs table
  • Several bug fixes
    • cost table not hidden when no project type selected
    • sub text under the section headers were randomly shown/not shown
    • Use the formatCurreny mixin for devices - same as instances

Before

before-remove-confirmation.mp4

After

Create Application

removes-confirmation-check.mp4

Create Instance

removes-confirmation-check-instance.mp4

Add Device

image

Related Issue(s)

#2206

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on flowforge/helm to update ConfigMap Template
    • Issue/PR raised on flowforge/CloudProject to update values for Staging/Production

Labels

  • Backport needed? -> add the backport label
  • Includes a DB migration? -> add the area:migration label

@Steve-Mcl Steve-Mcl requested a review from joepavitt July 4, 2023 15:31
@Steve-Mcl
Copy link
Contributor Author

@joepavitt - I am currently looking in to test failure but could you give a visual review of the changes please?

@MarianRaphael - you may wish to comment on the re-arrangement too?

@joepavitt
Copy link
Contributor

Why in the "Create Instance" example is there a "Payable Now" row that isn't in "Create Application". Not a fan of that being a row in that table as it seems like another item that you're paying for.

I think a little acknowledgement as per the existing description of the checkbox, where the existing checkbox is, suffices. Otherwise, all good.

@Steve-Mcl
Copy link
Contributor Author

Steve-Mcl commented Jul 4, 2023

@joepavitt

Why in the "Create Instance" example is there a "Payable Now" row that isn't in "Create Application".

It is only there when there is an amount to pay now

i.e. If you have $5 credit and monthly cost is $15, it tells you that payable now is $10
i.e. If you have $25 credit and monthly cost is $15, it doesnt show the "Payable Now" row

In essence, Create Application and Create Instance behave exactly the same (it is the same form)

PS: later in the video I switch between Small and Medium - you see the extra row appear when there is a differing "payable now" to the regular monthly cost.

PS2: I fudged the numbers in the 1st demo so that the cost of an Small instance was less than the credit available (so you could see what it looks like when the user has credit that will cover the additional cost)


I think a little acknowledgement as per the existing description of the checkbox, where the existing checkbox is, suffices.

Not sure what you mean exactly Joe?

@Steve-Mcl Steve-Mcl linked an issue Jul 4, 2023 that may be closed by this pull request
@joepavitt
Copy link
Contributor

Not sure what you mean exactly Joe?

Was thinking something like:

Screenshot 2023-07-04 at 17 48 28

Not convinced by the "pay now" row unfortunately. It's natural to read those rows and just mentally add up all items in that far column, which isn't reflective of the amount they're due to pay.

Alternatively, if we want to stick with the table format, we go with:

Screenshot 2023-07-04 at 17 54 06

maybe even without the "due now":

Screenshot 2023-07-04 at 17 55 19

@Steve-Mcl
Copy link
Contributor Author

Steve-Mcl commented Jul 5, 2023

Updated to ...

image

Hoping this is a good compromise?

@joepavitt if you can respond great - otherwise I will push the review to a.n.other.

Ta.

PS - I will update the spacing around the / to match table format

@joepavitt
Copy link
Contributor

Works for me, thanks Steve

@Steve-Mcl Steve-Mcl marked this pull request as draft July 5, 2023 10:24
@Steve-Mcl
Copy link
Contributor Author

Steve-Mcl commented Jul 5, 2023

Temporarily in draft due to bug found - shows odd value when credit is positive (i.e. user owes money)

NVM - this was a red herring 🐟 - see #2416

@MarianRaphael
Copy link
Contributor

I would suggest changing the "Additional charges: ..." label to bold. Furthermore, consider renaming it to simply "Price: ...". When creating an Instance, it's abundantly clear that this step incurs charges. I want to ensure that the price label for Devices cannot be overlooked.
Other than that, it looks good.

@Steve-Mcl
Copy link
Contributor Author

Steve-Mcl commented Jul 5, 2023

@MarianRaphael this would have been good information much earlier than now :(

Can you provide a mock up please - save me re-doing this again?

@Steve-Mcl
Copy link
Contributor Author

PS @MarianRaphael

I would suggest changing the "Additional charges: ..." label to bold. Furthermore, consider renaming it to simply "Price: ...". When creating an Instance,

The "Additional Charges" is original text on the Device Creation only.

The Instance / Application & Instance now looks like this (as of the PR status right now)

image

Please confirm if this is OK or if you want further changes.

@MarianRaphael
Copy link
Contributor

@Steve-Mcl Sorry, my comment was solely about the "devices," not the "instances." The purchasing process for instances seems great!

….vue

Co-authored-by: Marian <73583313+MarianRaphael@users.noreply.github.com>
@Steve-Mcl Steve-Mcl marked this pull request as ready for review July 5, 2023 11:23
@MarianRaphael MarianRaphael merged commit 6e04421 into main Jul 5, 2023
2 checks passed
@MarianRaphael MarianRaphael deleted the 2206-remove-billing-confirmation branch July 5, 2023 11:45
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.

Remove Confirmation Button for Additional Charges
3 participants