search

OneZoom / OZtree

OneZoom Tree of Life Explorer
Other
90 stars 20 forks source link

Renewals page creation #363

Open jrosindell opened 3 years ago

jrosindell commented 3 years ago
  1. always ask someone to enter their e-mail before entering into the page
  2. say that people can unify / change their e-mail by contacting us
  3. always write to the original e-mail to tell people that their leaves are up for renewal and to confirm that they have been renewed
  4. give people the option on renewal to be on the donor list or not - default is yes - also get people to review the current terms and conditions and confirm they they are renewing their own original sponsored leaves - terms should add something about confirming that you're only renewing your own leaves - add date of last update to the terms and conditions.
  5. This page should also include an option for 'recover expired sponsorships'
  6. If sponsorships were 'gratis' then they should appear but but priced not at zero but at the current going price with a note to say that you can write to request gratis renewal which may be granted depending on the reason the gratis leaf was granted and your current relationship with the charity.
jrosindell commented 3 years ago

Page design.

Should be a list of all leaves by the user with that e-mail Should be a tick box for each one that needs renewal Should work like sponsor leaf page with 'accept terms and donate now' button.

jrosindell commented 3 years ago

James and Yan to draw a mockup with labelled actions on it then hope to outsource if feasible

lentinj commented 3 years ago

Notes from meeting:

lentinj commented 3 years ago

This is starting to come together, and displaying the various categories of sponsorships. Still a bit too messy to commit, but it's here if you're curious: https://github.com/lentinj/OZtree/tree/wip-363

I've created a test/fixtures/reservations.sql that will move existing entries aside, and populate with a bunch of made-up sponsorships in different states. As well as being useful for developing / testing, the fixture sponsors will probably be useful for inspecting the end-result in various modes and edge cases.

hyanwong commented 3 years ago

Great, thanks for setting up the test fixture(s) - should be really useful. Don't hesitate to reorganise anything in the tests/ dir, as I didn't really know what I was doing when setting it up.

jrosindell commented 3 years ago

I've just thought of an edge case - which is that some leaves will have price = NULL and yet are sponsored e.g. the panda.

Maybe in such cases we should take the price paid before as the renewal.

Alternatively we make a note saying this leaf must be renewed by consultation over e-mail.

hyanwong commented 3 years ago

I think these are in the "banned" list (a separate table), so we could give those leaves a price (the price paid) and I think they will still be listed as "contact us" in the sponsorship box

lentinj commented 3 years ago

Okay, presumably we should also show a hint at the bottom, before the donate button. having 1 "banned" item out of ~50 sponsorships won't be particularly obvious if you're trying to renew the lot.

hyanwong commented 3 years ago

There's only one (out of ~20) species that are "banned" that have been sponsored so far. So we shouldn't put too much effort into making this aspect look perfect.

lentinj commented 3 years ago

This is starting to come together. The fixture SQL generates some standard users for playing, so you don't have to recreate conditions, have a go with:

Some TODOs and questions:

hyanwong commented 3 years ago

So how does the fixture stuff work, if you have an existing reservations table (as I do on my local version)? If I checkout the 3.5 branch and go to 127.0.0.1:8000/sponsor_renew.html/fixture_single@example.com I get

<class 'AttributeError'> 'NoneType' object has no attribute 'fetchone'

Version web2py™ Version 2.21.1-stable+timestamp.2020.11.27.18.21.43 Python Python 3.9.4: /Users/yan/opt/miniconda3/bin/python3 (prefix: /Users/yan/opt/miniconda3) Traceback (most recent call last): File "/Users/yan/Documents/Research/OneZoom/web2py/gluon/restricted.py", line 219, in restricted exec(ccode, environment) File "/Users/yan/Documents/Research/OneZoom/web2py/applications/OZtree/controllers/default.py", line 1968, in File "/Users/yan/Documents/Research/OneZoom/web2py/gluon/globals.py", line 430, in self._caller = lambda f: f() File "/Users/yan/Documents/Research/OneZoom/web2py/applications/OZtree/controllers/default.py", line 837, in sponsor_renew for r in db(db.reservations.e_mail == user_email).iterselect( File "/Users/yan/Documents/Research/OneZoom/web2py/gluon/packages/dal/pydal/objects.py", line 3563, in iter row = next(self) File "/Users/yan/Documents/Research/OneZoom/web2py/gluon/packages/dal/pydal/objects.py", line 3535, in next db_row = self.cursor.fetchone()

But it may be that I have a too-old version of pymysql or something

lentinj commented 3 years ago

So how does the fixture stuff work

It's not fancy, it's just a SQL script to run that moves the table content to one side and populates with known values: https://github.com/OneZoom/OZtree/blob/3.5/tests/fixtures/reservations.sql

Possibly upgrading pymysql would help, or could try replacing iterselect() with select(), looks like the former is exercising codepaths the MySQL driver can't use.

hyanwong commented 3 years ago

So how does the fixture stuff work

It's not fancy, it's just a SQL script to run that moves the table content to one side and populates with known values: https://github.com/OneZoom/OZtree/blob/3.5/tests/fixtures/reservations.sql

Right, but when does it actually run? Is it triggered by something specific in the test suite? I'm assuming visiting a url outside of the test suite won't work (as the correct info isn't in the DB)

Possibly upgrading pymysql would help, or could try replacing iterselect() with select(), looks like the former is exercising codepaths the MySQL driver can't use.

I seem to have v 1.0.2 of pymysql installed, which is, I think, the latest. So I presume it's not that. And I seem to be running the latest web2py (and mysql 8.0). So I'm not sure what the issue is.

lentinj commented 3 years ago

Right, but when does it actually run? Is it triggered by something specific in the test suite? I'm assuming visiting a url outside of the test suite won't work (as the correct info isn't in the DB)

The idea is that you can run it yourself with your preferred mysql client for visual checks outside a test suite, or a test suite would execute it as part of setUpClass().

So I'm not sure what the issue is.

Did replacing iterselect() make the issue go away?

hyanwong commented 3 years ago

Possibly upgrading pymysql would help, or could try replacing iterselect() with select(), looks like the former is exercising codepaths the MySQL driver can't use.

If I use "select" instead, I get:

Traceback (most recent call last): File "/Users/yan/Documents/Research/OneZoom/web2py/gluon/restricted.py", line 219, in restricted exec(ccode, environment) File "/Users/yan/Documents/Research/OneZoom/web2py/applications/OZtree/views/default/sponsor_renew.html", line 209, in AttributeError: 'NoneType' object has no attribute 'e_mail'

I assume this is an issue with looking for a non-existent email in the DB (since I didn't run the fixture code first).

lentinj commented 3 years ago

Yup, non-existent e-mail. The above stops using iterselect and patches things to the point an empty page can render, although presumably we should have a more obvious "You haven't sponsored anything, here's how" message in this case.

hyanwong commented 3 years ago

I think iterselect() rather than select() is fine as long as the edge case of no returned items is covered.

jrosindell commented 3 years ago

A few comments after reviewing the page as it stands now....

jrosindell commented 3 years ago

For the message about not gift-aiding. I'd suggest....

We do not have a gift aid declaration from you. If you are a UK tax payer please consider making a Gift Aid declaration which will increase the value of your donation by 25% at no cost to you. If you sponsor any new leaf you will be able to make a gift aid declaration as part of that process and this declaration would also apply to any donation you make here to renew existing leaves. Alternatively, if you want to make a new gift aid declaration without sponsoring any new leaves please write to us.

lentinj commented 3 years ago

If people add to their donation amount - how will this be posted.

The other option would be to record the extra donation separately, which could be useful if you then accept donations without sposorship, but probably isn't worth the faff. I was assuming it would be divided amongst the sponsored items arbitrarily, as you say.

lentinj commented 3 years ago

I've changed the behavior (or will once it's pushed) of the donation amount box, before it was making sure that the amount was at least the required amount, now if I deselect a renewal, the amount will go down too. I think this is more intuitive, but thoughts welcome.

We could try and work out if there was an extra amount added on deselect and preserve it, but I don't think it's worth it. If we still think this is confusing, I think we separate the price out into (x) for leaves + (y) extra donation.

jrosindell commented 3 years ago

What about a separate box or radio button to add 5,10,20, some other amount to your sponsorship? To keep things simple I think the extra could be added to whichever is first (according to some ordering) to save the hassle of another table / system to record extra donations.

lentinj commented 3 years ago

Okay, lets keep it separate in the UI.

In the DB if it's added to whichever is first that makes it easier to reconstitute the extra donation too.

jrosindell commented 3 years ago

Yes I think that's best - because which donation the extra is added to doesn't really matter as long as it sums to the correct amount

lentinj commented 3 years ago

I think all your comments from before are now sorted.

should we have a link to each leaf on the tree that opens as a new tab? I think yes we should.

We already did (on the image), but I've added "leafout" styling from the homepage, which is what it should have had. Or do you mean the text also?

For the 'donate' button...

I've turned this into a pill for now too, to make it work I've moved the terms/conditions link to a paragraph above. Seem reasonable? I think this looks better than the paypal

jrosindell commented 3 years ago

I think the pill choice and leaflet styling sound like the ideal solutions here.

lentinj commented 3 years ago

Note the above commit restructures how the reservations fixture is formed, they're now agemixture@fixture.example.com in line with unit tests.

hyanwong commented 3 years ago

I've just been trying the unit tests in applications/OZtree/tests/unit/test_modules_sponsorship.py and I forgot that a new installation won't have prices set on any of the leaves, so perhaps it's worth having a test that checks for at least one non-null price in ordered_leaves and if not, reports an error that says "You need to set prices first by going to http://127.0.0.1:8000/manage/SET_PRICES" (or whatever the URL is for the local server)

hyanwong commented 3 years ago

@lentinj - I think it would be helpful to add a README.md file to ./tests/ which gives brief instructions for how to run the tests (and maybe a (very short) explanation for how they work - e.g. "tests in unit/test_modules_sponsorship.py add a few rows to the existing reservations table, and delete them at the end of the tests". What do you think?

jrosindell commented 3 years ago

When visiting the sponsor_renew.html page, and locating an item where user_donor_show = NULL, currently the page treats this as equivalent to 0, and unticks the "show on donor page" checkbox. However we want it to be equivalent to 1 instead. The rationale for this is that 0 means the user has already before been given the option to show and decided not to, whereas NULL means they have never been asked. Yan and I think if they haven't been asked, it's OK to tick the checkbox by default, because basically the whole thing is about sponsorship, which means making public your donation.

hyanwong commented 3 years ago

If I try to look at a renewals page with 

[sponsorship]
allow_sponsorship = manager

in my appconfig.ini file, I get the following error message:

Screenshot 2021-06-06 at 22 11 37

What should happen is that allow_sponsorship should be able to be set to 0, 1, or a group name. If a group name, then if the user is logged in and the user belongs to the group specified, then it acts as a 0, otherwise it acts as a 0. This allows us to test on beta.onezoom.org, where we set allow_sponsorship = manager so that we can test all the sponsoring framework, but a causal visitor who accidentally ends up on the beta site cannot see anything to do with sponsorship.

Another (very minor) issue is that I think the text that currently says "Now sponsored by Fixture Single", but I think it should say "Sorry, this leaf has now been sponsored by someone else".

lentinj commented 3 years ago

I think it would be helpful to add a README.md

Yes, I was going to bring up this as part of the testing ticket, and explain the fixtures as well. We could also do with a test runner wrapping this and any others in future, probably an extra task in the Gruntfile since that's what's already there.

lentinj commented 3 years ago

Another (very minor) issue is that I think the text that currently says "Now sponsored by Fixture Single", but I think it should say "Sorry, this leaf has now been sponsored by someone else".

@jrosindell I think the wording came from your mockups, any comments? Changing it simplifies the code, since it doesn't need to dig out the current sponsor any more.

lentinj commented 3 years ago

When visiting the sponsor_renew.html page, and locating an item where user_donor_show = NULL, currently the page treats this as equivalent to 0, and unticks the "show on donor page" checkbox. However we want it to be equivalent to 1 instead.

The web2py DAL seems to be doing this for us, boolean fields seem to assume NULL == False, so it's not as simple as adding another check in the view. Worth digging deeper to sort out?

hyanwong commented 3 years ago

We could shift the field name to user_donor_hide, and negate all the existing 0/1 values?

lentinj commented 3 years ago

I'm assuming there's a "don't do that" option in the DAL that's going to be quicker to find than recreating the field. But yes, user_donor_hide is probably the more natural way around.

hyanwong commented 3 years ago

We're going to be fiddling with (a copy of) the main reservations table to get it into the correct format anyway, so I think a small change from 0<->1 plus a rename would be OK, if it makes the whole thing more sane.

lentinj commented 3 years ago

Amassing remaining TODOs:

(added by Yan)

lentinj commented 3 years ago

For the token-renewal, web2py has signed URLs, which is pretty much what we're after. There's no built-in concept of link expiry though, should we add one and if so how long should they last?

hyanwong commented 3 years ago

Why would we want the link to expire? I think the donor would expect this to be a permanent link, wouldn't they?

hyanwong commented 3 years ago

Why would we want the link to expire? I think the donor would expect this to be a permanent link, wouldn't they?

but this wouldn't apply to the "public" donor page (#364), would it? I think we decided that for that, we would create a permanent link out of the reservations.username field. I can't recall how we get people to populate that field, though (or if we do so automatically from the verified_donor_name somehow. Do you remember @jrosindell ?

hyanwong commented 3 years ago

For the token-renewal, web2py has signed URLs, which is pretty much what we're after. There's no built-in concept of link expiry though, should we add one and if so how long should they last?

I think that @jrosindell opened a specific issue about tokens at https://github.com/OneZoom/OZtree/issues/393, so maybe this is a good discussion to move there?

Re the public page, I've just opened a discussion of usernames at #394

lentinj commented 3 years ago

For user_donor_hide, what would the sponsor form text get replaced with?

hyanwong commented 3 years ago

Do you mean what should we change it to in sponsor_leaf now that they will also get a "donor" page, or what should the (mouseover?) text be in the sponsor_renew page?

hyanwong commented 3 years ago

If the former, perhaps something like this:

{{=XML(T('We would love to be able to thank you by name:  if this box is ticked we’ll add your name to our public %s and create a page for your sponsorships that you can show to others, if you wish.') % (A(T('list of donors'), _href='/donor_list', _target="blank"),))}}
lentinj commented 3 years ago

I mean if user_donor_show becomes user_donor_hide, and the linked checkbox's action inverts, what would the label become?

jrosindell commented 3 years ago

I'd suggest this....

Your sponsorship includes a public acknowledgement of your donation. We now acknowledge donors on our tree of life explorer, on our public list of donors, and if you wish you can create a personal page of your sponsorships that you can show to others. Tick this box if you would prefer this sponsorship to be acknowledged on the tree only.

lentinj commented 3 years ago

@hyanwong https://github.com/OneZoom/OZtree/commit/5d158910e6755e8dd12430c18b8a70b833c78870 switches to user_donor_hide, but there's nothing to do the migration itself, presumably you'll add it to your list somewhere?

hyanwong commented 3 years ago

Yep, we'll need to make a list of everything that needs to be done for the one-off migration

jrosindell commented 2 years ago

As discussed in #304 I've so far had problems accessing this page in the deployment.