bank-green/bankgreen-django

Last pre-launch checks

Closed this issue · 7 comments

RogerTangos commented

AS A Bank.Green staff member
I WANT the devs to check off a bunch of things before switching the API to data.bank.green
SO THAT users aren't negatively impacted by the switch

Things to check for

  • "Top three ethical" commentary button results in banks being recommended on the sustainable banks page
  • "Recommended Order" results in appropriate changes to the bank recommendation order on the sustainable banks page (feature was removed because unused – Simon)
  • Edits of "headline" and other fields are reflected on individual bank pages
  • Only banks currently shown on the live site are shown on the test.bank.green site

Acceptance criteria

  • Each of the boxes are investigated
  • For each thing not working, the issue is either fixed or a ticket is created to fix it
RogerTangos commented

Changes in progress in the pre-launch-checks branch

RogerTangos commented

Hey @shuesken

  • data.bank.green is currently running the pre-launch-checks branch
  • PR #51's recent changes to the backend broke frontend's query, which is making it hard to test the remaining checkboxes in the front end. Would you please fix that?
  • It seems like the frontend query, as designed, is hitting 3 different tables. Would it be enough to make one query just to get the bank names by country and then load the bank info later on, or does the frontend need all of that data initially for SPA stuff?

Would you please be able to fix this?

shuesken commented

@RogerTangos
I've updated https://test.bank.green with the correct query.

I'm not sure I understand your point about hitting multiple models – the query includes two variables you might have overlooked, withCommentary and withFeatures, that make the inline directive for those properties conditional. They're only set when requesting detail information for banks or when looking at the top banks. The graphql backend should correctly parse those variables and not return information from the Commentary and Features model unless requested, and as far as I can tell, that works:
image

  • 👍1
shuesken commented

@RogerTangos I've made a few more fixes for things as I discovered them in testing. As far as I'm concerned, things look good. I do not know how to properly test the last requirement – as far as I'm aware, we show all banks that exist in the airtable, and only those we import. Maybe you have a better intuition on what spots to check here? I do think the PR has grown quite large by now though and could use another review.

RogerTangos commented

The last checkbox I tested just by counting the number of li elements in the list of banks from the united states. In the test site,

document.getElementById('foolist').getElementsByTagName("li").length  // test site
>> 100

document.getElementById('foolist').getElementsByTagName("li").length  // live site
>> 140

It seems that graphene limits results to 100, and I haven't figured out how to remove this limitation yet. Currently reviewing the rest of the PR, @shuesken

shuesken commented

@RogerTangos I fixed the limit in settings.py with GRAPHENE = {"RELAY_CONNECTION_MAX_LIMIT": 10000}

RogerTangos commented

Thank you.