search

membermatters / MemberMatters

An open source membership, access and payments portal for makerspaces and community groups.
https://membermatters.org
MIT License
41 stars 23 forks source link

Plan/Tier card interval string error #126

Closed kodaxx closed 3 years ago

kodaxx commented 3 years ago

Description

The plan card/tier card components seems to have an error displaying the interval string correctly.

Instead of displaying From $30.00 USD every 1 months, it displays From $30.00 USD every 1 paymentPlans.interval.Months

Affected components

It seems to be these components/lines

Example

example1

kodaxx commented 3 years ago

I believe I can fix this issue, but you'll have to deal with me on my git skills in regards to contributing. It will be good practice for me, as I usually just use git with my own projects.

jabelone commented 3 years ago

Thanks @kodaxx, no problems at all. :)

kodaxx commented 3 years ago

Okay, after digging into this it seems that when you create a payment plan, the interval is saved in the DB in its plural form (Months, for example) https://github.com/membermatters/MemberMatters/blob/d0883d032b8c4b91e19a14c2d92888c2f1050a57/memberportal/api_admin_tools/models.py#L36

This means that the minPrice.interval computed function always returns the plural form (Months, for example) (https://github.com/membermatters/MemberMatters/blob/main/frontend/src/components/Billing/TierCard.vue#L19)

Since the interval string's key in i18n is the singular, these two things are conflicting https://github.com/membermatters/MemberMatters/blob/main/frontend/src/i18n/en-AU/index.ts#L403

There's a few ways to go about this I think:

  1. Change the key in the i18n file to the plural version - this seems to be the most correct while being the least disruptive
  2. Change the way it is stored in the database - this could potentially mess with existing installations
  3. I could slice the last letter off of the value that minPrice returns, since they all end in s - this feels hacky to me, but would work of course

Which of these solutions is preferred to you? Do you think my findings are correct?

kodaxx commented 3 years ago

The issue is the same in both components, btw

jabelone commented 3 years ago

There are plural and non plural versions in the i18n file. It looks like the db values are returning as capitalised strings so it's probably easiest to just call toLowerCase() on the returned string (TierCard.vue#L19) to make sure it's all lowercase, then the existing i18n definitions should work.

Screen Shot 2021-05-08 at 1 08 28 pm
kodaxx commented 3 years ago

There are plural and non plural versions in the i18n file. It looks like the db values are returning as capitalised strings so it's probably easiest to just call toLowerCase() on the returned string (TierCard.vue#L19) to make sure it's all lowercase, then the existing i18n definitions should work.

Screen Shot 2021-05-08 at 1 08 28 pm

I'm not sure this is the issue, as I see both plural and singular forms.

Either paymentPlans.intervalPlurals.Months is the key that's being computed or paymentPlans.interval.Months is being computed.

Neither are correct, even in lowercase, because the key should be paymentPlans.intervalPlurals.month, for example.

It's the 3rd level deep that is incorrect

kodaxx commented 3 years ago

The quick solution, and the one that causes the least possible disruption imo is to change the key to the plural form in the i18n file.

At that point we would do the toLowerCase() in the component and it would be correct

jabelone commented 3 years ago

Ahh sorry I missed that. Yeah it looks we need both a change to the i18n definitions and the returned value to be lowercased.

jabelone commented 3 years ago

This whole section is probably due for a refactor anyway, as it's a little messy. ;)

kodaxx commented 3 years ago

Awesome! Okay that's what I thought, I just wanted to make sure we agree on the solution - will make that change and (attempt to) submit a pull request.

Side note: I don't see a dev branch, should I PR to main?

jabelone commented 3 years ago

Yep that's correct. Each release is tagged when I'm happy with the state of main. If/once more people start contributing this will probably change slightly, but that's how it is for now. :)

jabelone commented 3 years ago

Fixed in #127

jabelone commented 3 years ago

Ah I worked out why I didn't pick this up, I had faulty data in my dev database that means I didn't notice this :)

kodaxx commented 3 years ago

Ah I worked out why I didn't pick this up, I had faulty data in my dev database that means I didn't notice this :)

Make sense lol

I'll probably stick to some of the low hanging fruit until I'm more familiar with the codebase. Thanks for the merge!

jabelone commented 3 years ago

Low hanging fruit is still fruit that needs to be picked so still appreciated ;) Cheers!