Fix getExpiration to be a one-way valve

[ryancwalsh]

Upon image rendering, the system should check for all activity of the mainnet account since the certificate issue date.

• If the indexer is unavailable:

  The cert could say simply "Expires: after the first 6-month period of inactivity of this mainnet account"

• If the indexer is available (which is the typical case):

  The cert calculates the expiration and if expired simply says Expired: 2021-12-03 but if still active says Expires: after the first 6-month period of inactivity of this mainnet account (2022-08-06 if no future activity)

But in the short run, since Dan hasn't had time to update designs, we could write an * and then insert a new line underneath that starts with * and then explains.

See https://discord.com/channels/828768337978195969/906115083250307103/940981535677505577 and discussion before and after https://discord.com/channels/828768337978195969/906115083250307103/943511742229667901

My next challenge (which you are welcome to explore, but might feel unfamiliar and harder) is how to change getExpiration in https://github.com/NEAR-Edu/near-certification-tools/blob/2d77d6af8594bfe0d456a9aa9c4c5a91efe382d9/web-app/pages/api/cert/%5BimageFileName%5D.ts#L82 to using Prisma. Prisma probably allows "cheating" and using raw Postresql queries. Getting that working would be a first step.

Then ideally later we'd do it in an abstract way using Prisma's style without a raw query (which would give us flexibility in case the indexer ever decides to use a DB other than Postgres).

You can see a sample Prisma query in getMostRecentActivityDateTime, which is a function that probably needs to be deleted since or at least reconceived since we now have a different definition of how we want certificates to expire.

[eadsoy]

Re: https://stackoverflow.com/questions/43872026/remove-issue-reference-in-github#:~:text=There%20is%20an,at%206%3A33 That's interesting and unfortunate that GitHub doesn't let us clean up mishaps like that. Not a big deal though.

Great, thank you. Very annoying really.

What are the .json.bak files for?

The .bak files are for generating random data, I left them in the directory (by adding the .bak extension so that Synth doesn't detect them) in case we decide to generate random data later on. The ones we use: receipt.json and action_receipt.json, are using data sources to generate data:

I will very likely remove Synth completely but I want to leave it a little bit longer to make sure there's no use for it.

---

After deciding that JS's Number type won't be safe to calculate issuedAtUnixNano:

1. I installed the bn.js library and implemented it.

2. I then converted the result to String( bn.js doesn't let conversion to Number if value exceeds 53 bits, which is the point)

3. I thought I'll use this string value in the query by casting it to numeric but I got a DB error stating there's an invalid sign. To be more precise, we are using issuedAtUnixNano to get results after the issue date and this was how we used it in two places in the query:

   ...
   AND R."included_in_block_timestamp" > ${issuedAtUnixNano}
   ...

   After converting it to string I changed it to:

   ...
   AND R."included_in_block_timestamp" > ${issuedAtUnixNano}::numeric
   ...

   This threw the following error :

   Invalid `prisma.queryRaw()` invocation:··
     Raw query failed. Code: `22P03`. Message: `db error: ERROR: invalid sign in external "numeric" value`]

   Then the great fun began. I manually added the value both in pgAdmin and also in the code and it worked perfectly fine even as a string. AND R."included_in_block_timestamp" > '1615368226000000000', AND R."included_in_block_timestamp" > '1615368226000000000'::numeric, AND R."included_in_block_timestamp" > '1615368226000000000'::decimal; all worked fine without errors. I also checked what the params look like in the Prisma logs, and no problem there either, issuedAtUnixNano is a string and we're just passing it into the query. It looks like it's an issue with how Prisma delivers raw queries. Some similar issues:

   * https://github.com/prisma/prisma/issues/10424
   * https://github.com/prisma/prisma/issues/5083
   * Double casting : https://github.com/prisma/prisma/issues/4647#issuecomment-939555602

   This took me a lot of time to figure out but I decided to double cast included_in_block_timestamp: to text and then to numeric, which finally worked. I tried this before actually but it didn't work without parentheses for some reason so I really struggled to find a solution but adding parentheses did the trick. It doesn't make any sense but what can I say, it worked. https://github.com/NEAR-Edu/near-certification-tools/blob/4d3fd7c349618fa0a6d5e7379e13798a043295dd/web-app/helpers/expiration-date.ts#L40

4. On a final note: I am not sure if we have to use bn.js, because using BigInt works as well. From https://www.epfl.ch/labs/dedis/wp-content/uploads/2020/01/report-2019-2-Julien-von-Felten-KyberJS-performance.pdf:

2.2 From bn.js to BigInt It was decided to change the bn.js to BigInt for the first optimization. The problem with the kyber library is that it is using huge numbers which can not be represented with a standard number in JavaScript which uses 53 bits. To thwart this problem, the bn.js is currently used but this library uses arrays to represent big numbers which is not performant in memory and computation time. Hence, it is a priority to find another method than bn.js to represent big integers. There are two different options: • The new BigInt type • Concatenate two numbers to create a big integer Fortunately, a new type came in early 2019 which is the BigInt type8 . It was a perfect timing to try this new feature from JavaScript. It is useful for representing numbers bigger than 2 53 .

So, I'm really not sure which one is better. Both are working fine. If you have a strong feeling about using BigInt rather than bn.js, I'll happily convert to BigInt.

5. Now only thing left is finally thinking about additional scenarios for test cases.

[ryancwalsh]

@eadsoy Thanks for the update! I think you could try to get a better grasp on this number safety topic and what the best practices are (i.e. how we should handle the fact that NEAR cares about nanosecond precision). It will help both of us.

But focus on the tests first since you think there is a chance that your handling of numbers and datetimes might actually already be fine (so it's lower priority to get a clear understanding / proof of it).

Thanks!

[ryancwalsh]

@eadsoy I'm still looking forward to seeing some working test cases :-) Thanks!

[eadsoy]

Test Cases

We do have working test cases at the moment, I am testing whether we are getting back the expected results. So only thinking of edge case scenarios, which don’t seem too edgy to me maybe because we had thought about some of them before. I am really trying to find any missing case but here are all the test cases we have:

1. Account that has multiple 180-days of inactivity after the issue date:

   • Expected query result: moment is the first occurence of such period
   • Expected expiration date: last activity date - (diff_to_previous_activity - 180)

2. Account that has been active, no 180-days of inactivity.

   • Expected query result: moment is the last activity date.
   • Expected expiration date: last activity date + 180

3. Account with years of activity = has multiple inactivity periods exceeding 180 days and years of activity on his account

   • Set up issue date at a time where account doesn’t have any 180-days of inactivity after issue date.
     • Expected query result: moment is the first occurence of such period
     • Expected expiration date: last activity date - (diff_to_previous_activity - 180)
   • Set up issue date at a time where account has multiple periods of 180-days of inactivity
     • Expected query result: moment is the last activity date.
     • Expected expiration date: last activity date + 180

For the account with years of activity, I have seeded 20 entries that span throughout years, but this is not enough I think. Is the amount of entries more important or that they are spread throughout years? II suppose more data would give us information on the performance of the query we are using, right? If the amount of entries are more important or also important, I was thinking to approach it like this:

Generating seed data :

Goal is to seed it with 1000 receipts + action_receipts for a specific user.

• Synth is/ was great but unfortunately they do have a bug when inserting decimal values. So using it alone is not an option.
• As an alternative I thought I could use two seeding methods together: One that uses Synth, the other one would be Prisma’s seed file.
  • Use Synth to generate 1000 acction_receipts for a user. This means only receipt_ids and account_names are needed. Both are string values, I’m not expecting an issue there.
  • In the Prisma seed file, iterate through these 1000 rows and get their receipt_id as foreign key for receipts, generate random timestamps in certain range and create receipts.

I’m not sure if this is a good way yet but this is what I’m working on right now. For generating the random timestamp I thought this might be a good start https://ethereum.stackexchange.com/a/86881

import { randomBytes } from 'crypto';
import BN from 'bn.js';

const value = randomBytes(32); // 32 bytes = 256 bits

// Value as native bigint
/* global BigInt */
const bigInt = BigInt(`0x${value.toString('hex')}`);

// Value as BN.js number
const bn = new BN(value.toString('hex'), 32);

console.log({ bigInt });
console.log({ bn });

Both generate BigInt values but I am not sure about this at this moment, trying to find a better way.

All this is because I'm thinking it might be good to seed the db with bigger amount of seed data. If there's something apparent I am missing, I will just seed the db with hard-coded data.

I also thought of how the expiration date could be displayed but I’d like to get back to it after I am done with this.

[ryancwalsh]

@eadsoy I think generally we'll want tests in our normal suite of tests to be deterministic (i.e. they don't have random inputs and undetermined outputs). Test suites that have randomness in them get extra confusing and problematic for teammates who weren't working on that feature.

I was able to run your tests in branch https://github.com/NEAR-Edu/near-certification-tools/tree/feature/prisma-test-setup

I was planning to review them more closely and see whether I think anything is missing, but then I noticed some more important problems related to minting that took my attention*. Maybe I'll be able to look at tests tomorrow.

*a bunch of annoying confusion related to nanoseconds vs milliseconds, string vs number, etc. These are major pain points when working with NEAR, in my opinion. I think we (Edu) will want to come up with reliable patterns for dealing with safe precision math, ensuring that timestamps are the correct type, etc (since Pagoda has not).

[ryancwalsh]

@eadsoy I have some constructive feedback :-)

In order for tests to be useful, they need to be trustworthy, and in order for them to be trustworthy, they need to be clear, and in order for them to be clear, each of their parts (including seed data) needs to be obvious.

I'm trying to understand the tests, and I open up https://github.com/NEAR-Edu/near-certification-tools/blob/f58765aa10d300d525201bc64c1ba90e1b3f24fd/web-app/prisma/seed.ts and it is very hard to understand.

It's a whole long series of non-human-readable long numbers (probably in nanoseconds), like 1648224546000000000.

And none of those dates are obviously clumped together with given identities / test scenarios.

Can you please reorganize this seed file such that someone (like me) can more easily verify the logic of the dates that you chose for each scenario?

You'll want to use human-readable dates (and use a function to convert them to nanoseconds for insertion into DB).

And instead of calling createMany just once per table, group the insertions into different sections (one section per scenario).

E.g. comments like this will be helpful: https://github.com/NEAR-Edu/near-certification-tools/blob/f58765aa10d300d525201bc64c1ba90e1b3f24fd/web-app/test/__tests__/expiration-date.test.ts#L20

Thanks!

[ryancwalsh]

@eadsoy Remember to pull a fresh copy of the branch locally. I figured out one of the problems you were having: https://github.com/NEAR-Edu/near-certification-tools/commit/4e0cc7921e1e8f769ee5ec4a7c4334f213480511

[ryancwalsh]

@eadsoy I didn't hear from you today.

I added this temporary warning on the main branch: https://certificates.near.university/account/rashaabdulrazzak.near

It would be nice for us to get https://github.com/NEAR-Edu/near-certification-tools/pull/40/files merged in (and delete that alert component), but first I'd like it all to look at lot clearer and easier to understand.

Thanks.

[eadsoy]

I am sorry for not providing daily updates. I was not aware that I should have. I’ll speed this up and get back to daily updates. Sorry again and hopefully this won’t take too long to finish going forward, so there won't be a need for too many updates and we finally wrap this up.

1. https://github.com/NEAR-Edu/near-certification-tools/issues/17#issuecomment-1083677450

I think generally we'll want tests in our normal suite of tests to be deterministic (i.e. they don't have random inputs and undetermined outputs). Test suites that have randomness in them get extra confusing and problematic for teammates who weren't working on that feature.

Ok that's great to hear. I was really not sure if my approach was enough, thank you.

2. I'm trying to understand the tests, and I open up https://github.com/NEAR-Edu/near-certification-tools/blob/f58765aa10d300d525201bc64c1ba90e1b3f24fd/web-app/prisma/seed.ts and it is very hard to understand.

It's a whole long series of non-human-readable long numbers (probably in nanoseconds), like 1648224546000000000.

I understand that definitely. I haven’t been descriptive while going back and forth between Synth and Prisma’s seed file and deciding what method to use to seed the db, but I surely should have. I am making the changes you suggested using the new function (convertStringDateToNanoseconds) to convert dates to timestamps, currently throwing an error but I'm working on it.

3. Remember to pull a fresh copy of the branch locally. I figured out one of the problems you were having: https://github.com/NEAR-Edu/near-certification-tools/commit/4e0cc7921e1e8f769ee5ec4a7c4334f213480511

Pulled a fresh copy. I guess you are referring to the import issue I was having. These query functions were inside the [imageFileName].ts and importing from there was causing issues. I separated the getQueryResult(now getRawQueryResult) to test/test-helpers because it is only used in the test file. I thought having it in the expiration-date.ts file might be confusing. But now I learned it’s not. Thank you.

4. I added this temporary warning on the main branch: https://certificates.near.university/account/rashaabdulrazzak.near

It would be nice for us to get https://github.com/NEAR-Edu/near-certification-tools/pull/40/files merged in (and delete that alert component), but first I'd like it all to look at lot clearer and easier to understand.

Thanks.

I'll be updating you later today (definitely after your NCD session) but just wanted to respond beforehand.

[ryancwalsh]

@eadsoy

I separated the getQueryResult(now getRawQueryResult) to test/test-helpers because it is only used in the test file. I thought having it in the expiration-date.ts file might be confusing. But now I learned it’s not. Thank you.

That's not how I would describe the change I made or why I made it.

The important part is that I deleted the entire web-app/test/test-helpers/query.ts.

A) We don't want to duplicate code B) Our tests need to be testing the actual functions that our app relies on (or else the tests aren't accomplishing the goals of tests)

Thanks.

[eadsoy]

@eadsoy

I separated the getQueryResult(now getRawQueryResult) to test/test-helpers because it is only used in the test file. I thought having it in the expiration-date.ts file might be confusing. But now I learned it’s not. Thank you.

That's not how I would describe the change I made or why I made it.

The important part is that I deleted the entire web-app/test/test-helpers/query.ts.

A) We don't want to duplicate code B) Our tests need to be testing the actual functions that our app relies on (or else the tests aren't accomplishing the goals of tests)

Thanks.

I missed your point apparently. I'm sorry and thank you.

1. I have refactored the tests in /test/tests/expiration-date.test.ts to accept milliseconds as arguments. I created a convertStringDateToMilliseconds function, which is almost the duplicate of convertStringDateToNanoseconds. I placed it in the tests/test-helpers but moved it to helpers/time.ts as you suggested in https://github.com/NEAR-Edu/near-certification-tools/pull/40#commitcomment-70286488. My commit: 29b63f0

2. https://github.com/NEAR-Edu/near-certification-tools/pull/40#commitcomment-70286289

I corrected the comment in the next commit https://github.com/NEAR-Edu/near-certification-tools/pull/40#issuecomment-1087212672

3. I think I need to re-read through the comments in the seed file carefully and make corrections
4. Understanding why issued_at is in milliseconds according to the NEP177 standard and why program_end_date is in nanoseconds in the meta data took me some time to understand. I am still not sure I understand.
5. Finally, I'd like to ask if I should write tests to see if the expiration date is rendered correctly too. This is something that confuses me.

[ryancwalsh]

@eadsoy Re 4.: Yeah, it's weird. Jacob created program_start_date and program_end_date using nanoseconds because NEAR transactions have nanosecond precision, so he wanted to stay consistent. We don't know why NEP177 issued_at has only millisecond precision.

Re 5. I'm not sure what you mean. It seems like you're already testing whether getExpiration returns the correct value. E.g. await expect(getExpiration('bobwilson.testnet', convertStringDateToMilliseconds('2021-11-05T00:00:00+00:00'))).resolves.toEqual('2022-08-31');

So as long as we have tested each scenario that we can think of, that's good. I haven't sat down and tried to think of all of the different relevant data scenarios.

But in trying to read yours, I still don't feel confident in them.

E.g. ` * Sally has only one activity('2021-03-02T12:35:46+00:00') after issue date ('2021-03-02T12:36:46+00:00') which is 1 minute prior to issue date

• query should only return activities after issue date that match conditions and if existent.`

I think sometimes there must be typos ("after issue date [...] which is 1 minute prior to issue date"), and other times it's just not clear.

I think it would be a helpful exercise for us each to sit down with a blank sheet of paper and forget all of the code and the existing tests and instead just write out in plan easy-to-understand terms all of the scenarios we can think of, such as:

Steve's cert was issued_at 2021-01-05, and he had frequent mainnet activity for a couple of months (through 2021-03-15) but then no mainnet activity for 204 days and then had some more mainnet activity. So his expiration = 180 + 2021-03-15. Right? That could be one.

Another could be: Patricia's cert was issued 2022-04-01, and she has not had any mainnet activity since then. So her cert is tentatively scheduled to expire 2022-04-01 + 180.

Another could be: Steven's cert was issued 2021-08-03 and he has continued to have mainnet activity every couple of days through today [or a specific date, since we don't need the test to be dynamic]. So his cert is tentatively scheduled to expire [that end date] + 180.

getRawQuery also needs better documentation because even I am already having trouble remembering how it works and why it needs a Union. So someone new to the project would need to totally reverse engineer it to understand, which is not good. It should be describable in just 1 sentence or so.

Thanks.

[eadsoy]

It seems like you're already testing whether getExpiration returns the correct value. E.g.

This answers my question:) thank you

I think it would be a helpful exercise for us each to sit down with a blank sheet of paper and forget all of the code and the existing tests and instead just write out in plan easy-to-understand terms all of the scenarios we can think of, such as:

I am writing down the test cases in the format you suggested also trying to correct typos.

I have a question:

As I am writing the cases down, I realized something:

1. issued_at is in milliseconds, and the query checks everything after this. Even if the account had an activity 1 minute prior to issue_date, the query won’t pick that up as expected.
2. Then, in the getExpiration function while calculating the expiration date, we add or subtract days from the moment value depending on the scenario (again all in milliseconds).
3. Inside getExpiration, we use the formatDate function to only return the date string. If the expiration date is 2022-04-05T06:03:42.957Z, we pass it as ‘2022-04-05’ to the isBeforeNow function while rendering the certification, as if the expiration date is actually 2022-04-05T00:00:00.000Z.
4. The isBeforeNow function takes in this date string and checks whether it is a date before this moment and returns a boolean value; we then display the result.

So for example:

1. Query returns

result: [
    {
      moment: '2022-10-07T17:39:24.572937+00:00',
      diff_to_previous_activity: null,
      has_long_period_of_inactivity: false
    }
  ]

1. In getExpiration we take the moment from the query result and add 180 days in this case, which would be 2022-04-05T17:39:24.572Z in dayjs format.
2. Finally in getExpiration, we use the formatDate function to return the date string which is ‘2022-04-05’ in this case.
3. Then, the isBeforeNow function checks if this date is before this very moment which let’s say is 2022-04-05T06:16:45.470Z. Since the argument passed is ‘2022-04-05’ , isBeforeNow compares this moment to 2022-04-05T00:00:00.000Z and not 2022-04-05T17:39:24.572Z. There are hours for the certification to get expired, but it will display 'Expired'.

What I’m trying to say is, we are displaying a date on the certificate but it might be that if my certificate is showing today’s date, I might still see that it is expired for two reasons:

1. We are not comparing now to the actual time when the certificate expires.
2. If we ignore the actual time and just proceed with the date string, I think it's still strange to see that it is expired if I render my certificate on the expiration date the certificate is displaying. But this might be overthinking on my part.

So, shouldn't we compare now to the time as well instead of just the date?

[ryancwalsh]

@eadsoy Darn, when I got the email from GitHub about this comment of yours, it said that I could reply via email, which I did at Tue, Apr 5, 6:25 AM ET, but I now see that my response never got added here to the conversation. So I'll copy it from my sent email 7 days ago:

I don't have the code in front of me, but yes, we want the expiration to be a particular moment (not rounded to the day) even though the value displayed on the certificate will be rounded to the day. But whether the certificate shows as expired or not should be precise to the moment.

So if that's not how it works yet, it's an important catch, and we need to fix it.

[ryancwalsh]

@eadsoy And can you please add test cases about https://github.com/NEAR-Edu/near-certification-tools/issues/17#issuecomment-1097015748? Thanks.

[ryancwalsh]

@eadsoy The seeding and the test cases are still super confusing to me. We really need these to be clearer, or else they're not achieving the goal of giving us assurance that the code works.

First, see https://github.com/NEAR-Edu/near-certification-tools/commit/8b5cd1640db8dad535a72936de0477f5cffd0303 since it doesn't make sense to have 2 test cases named so similarly (Steven and Steve).

Also, should return expiration date for Steve as last activity date - (diff_to_previous_activity - 180) is currently failing.

So then I looked into the code to try to understand the seeding and the tests.

And I look at https://github.com/NEAR-Edu/near-certification-tools/blob/8b5cd1640db8dad535a72936de0477f5cffd0303/web-app/test/__tests__/expiration-date.test.ts#L68

1. What does Steve's cert was issued_at 2021-01-05 — issueDate mean? In VSC, it looked like a minus rather than a dash. But even now that I see that it's a dash, I'm still wondering. Are you just saying that issued_at is another way of saying issueDate? Maybe we'd say Steve's cert was issued_at 2021-01-05 (i.e. issueDate). Or maybe it's clear enough as Steve's cert was issued_at 2021-01-05.

2. Where can I find 2021-01-05 as Steve's issueDate? It took too much brainpower to realize that it was the second argument of getExpiration (and also of getRawQueryResult). Plus, It seems like there was an error, where getExpiration was (accidentally?) using a different value for issueDate. Check whether my upcoming commit (which I'll mention below) fixes it or if I'm misunderstanding something. I think it's clearer to define a const which then gets used for both.

3. It also says His cert has expired on = 2021-03-15 + 180 ( = 2021-10-06 - (204 - 180) days). The 2021-03-15 part seems wrong because the seed data shows activity on 2021-03-16 (as was also mentioned higher in the comment of expiration-date.test.ts).

4. Look at each part of https://github.com/NEAR-Edu/near-certification-tools/commit/c4714dfc3a8bae157056e88d1a97538c3d999013 carefully. Am I messing anything up? Or do you agree with my changes? Can you please make sure all of the other test cases get as clear as possible? The Steve test now passes. So do the other tests. But I haven't taken the time to investigate the other tests because this one took me so much time. If you can make sure that they're all clear, that would help. Thanks!

[eadsoy]

Quoting from your last comment https://github.com/NEAR-Edu/near-certification-tools/issues/17#issuecomment-1097088588

@eadsoy The seeding and the test cases are still super confusing to me. We really need these to be clearer, or else they're not achieving the goal of giving us assurance that the code works.

1. What does Steve's cert was issued_at 2021-01-05 — issueDate mean? In VSC, it looked like a minus rather than a dash. But even now that I see that it's a dash, I'm still wondering. Are you just saying that issued_at is another way of saying issueDate? Maybe we'd say Steve's cert was issued_at 2021-01-05 (i.e. issueDate). Or maybe it's clear enough as Steve's cert was issued_at 2021-01-05.

Yes definitely, very much aware of these. I’m sorry about the unclear comments and still hard to understand seed file. It was WIP, to be pushed with all the corrections by now. I fixed most of them I think but haven’t pushed because I was dealing with other issues I didn’t ask you about, before making sure they make sense. (I pulled your last changes and am continuing from there)

After my last commit, while correcting typos etc. and writing test cases, I had some problems with this test case in particular which I couldn’t figure out:

_Patricia's cert was issued_at 2022-04-01, She has not had any mainnet activity since then. Her cert is tentatively scheduled to expire 2022-04-01 + 180._

I checked Rasha’s account on explorer and also from the Indexer but her NFT isn’t appearing on her account as a transaction. It’s like there is no connection to her account at all besides probably showing up on her collectibles list.

Because, if minting an NFT doesn’t show up on the transaction history of the receiver account, AND if the account didn’t have any activity at all after their issue date, the query won't return any result.

If it would appear on the transaction history, the query would be able to pick up the issue date as activity and depending on whether it is over 180 days ago or not would return the desired result to calculate the expiration date.

I read your conversation with Jacob https://github.com/NEAR-Edu/near-certification-tools/issues/47 but I don’t think this is the answer to my question? I also see the certificates I minted on testnet on the collectibles list of the receiving account but they don't appear on the transaction history of the receiver account.

I would really appreciate your help with this. What am I missing? If I'm correct, would this work as a solution?:

• If the query returns nothing, check the issue date and add 180 days to it.
• Check whether the calculated expiration date is before now or not and proceed from there?
• Thinking out loud: What if the account doesn't exist? Query still wouldn't return anything. I need to think about this.

3. It also says His cert has expired on = 2021-03-15 + 180 ( = 2021-10-06 - (204 - 180) days). The 2021-03-15 part seems wrong because the seed data shows activity on 2021-03-16 (as was also mentioned higher in the comment of expiration-date.test.ts). Yes, I realized this as well. Thank you for correcting it.

First, see https://github.com/NEAR-Edu/near-certification-tools/commit/8b5cd1640db8dad535a72936de0477f5cffd0303 since it doesn't make sense to have 2 test cases named so similarly (Steven and Steve).

I took your recommended test cases from https://github.com/NEAR-Edu/near-certification-tools/issues/17#issuecomment-1087701979 and integrated them. I didn't change the names and Steve-Steven is definitely confusing. Agreed and again, thank you.

2. Where can I find 2021-01-05 as Steve's issueDate? It took too much brainpower to realize that it was the second argument of getExpiration (and also of getRawQueryResult). Plus, It seems like there was an error, where getExpiration was (accidentally?) using a different value for issueDate. Check whether my upcoming commit (which I'll mention below) fixes it or if I'm misunderstanding something. I think it's clearer to define a const which then gets used for both.

3. Look at each part of https://github.com/NEAR-Edu/near-certification-tools/commit/c4714dfc3a8bae157056e88d1a97538c3d999013 carefully. Am I messing anything up? Or do you agree with my changes? Can you please make sure all of the other test cases get as clear as possible? The Steve test now passes. So do the other tests. But I haven't taken the time to investigate the other tests because this one took me so much time. If you can make sure that they're all clear, that would help. Thanks!

The test case is passing when I expect the expiration date to be 2021-09-13, not 2021-09-12.

You did not mess up anything at all because naturally you see this as an error, not sure but I think maybe it’s about our timezones being different. I have found some issues on dayjs’ github repo, mentioning this I think but it doesn’t make sense to me at all yet. I'm still looking into it.

[eadsoy]

@eadsoy Darn, when I got the email from GitHub about this comment of yours, it said that I could reply via email, which I did at Tue, Apr 5, 6:25 AM ET, but I now see that my response never got added here to the conversation. So I'll copy it from my sent email 7 days ago:

I don't have the code in front of me, but yes, we want the expiration to be a particular moment (not rounded to the day) even though the value displayed on the certificate will be rounded to the day. But whether the certificate shows as expired or not should be precise to the moment.

So if that's not how it works yet, it's an important catch, and we need to fix it.

No worries at all and thank you. So in that case, yes, this has to be fixed.

[eadsoy]

@eadsoy And can you please add test cases about #17 (comment)? Thanks.

On it.

[eadsoy]

@ryancwalsh I’m again sorry for the silence but I seriously am just thinking. I’d really appreciate if you could give me today to figure these out and write you a proper update. I have advanced but definitely need more time. Thanks!

[ryancwalsh]

@eadsoy

1. About the Rasha and Patricia problem. Great catch. I totally missed this. This is why these test cases are so helpful. For your 3 bullet points about "If the query returns nothing", yeah I wonder if we need to add a 3rd part of the CTE to check whether the account exists (if the first part returns nothing)? I don't know.

Or maybe it would suffice to add a note to the JSDoc of getExpiration to explain that it assumes that issuedAt would only be provided as the 2nd arg if it is real. E.g. the only time our webapp ever calls getExpiration is after we've already confirmed that the tokenId is legit: https://github.com/NEAR-Edu/near-certification-tools/blob/c4714dfc3a8bae157056e88d1a97538c3d999013/web-app/pages/api/cert/%5BimageFileName%5D.ts#L43-L58

2. About 2021-09-13 vs 2021-09-12, that's weird. Even though you and I are in quite different time zones, and so it feels like it could be a time zone issue, I can't actually think of how it would be a time zone issue because we're not specifying the hour of day anyway. E.g. if I type "2021-03-16 + 180 days" into Google, I get 2021-09-12 as the result at https://convert-dates.com/days-from/180/2021/03/16#:~:text=Your%20starting%20date%20is%20March,would%20be%20September%2012%2C%202021. and https://www.wikidates.org/calculate/180-days-from-3-16-2021.html and https://whatisthedatetoday.com/180-days-from-march-16-2021.html and never see a different result.

And I think all of the seeds and tests all currently use +00:00 (UTC) too. So I'm confused about why we're getting different results.

[eadsoy]

1. https://github.com/NEAR-Edu/near-certification-tools/issues/17#issuecomment-1097015748

The expiration date is now being compared to the present moment taking the exact moment of expiration into account, instead of only the date:

• expiration is now returned in 'YYYY-MM-DDTHH:mm:ss+00:00' format https://github.com/NEAR-Edu/near-certification-tools/blob/c3c77d7b9d905a17af66c131a40106c798eba966/web-app/helpers/expiration-date.ts#L173

• It is later on formatted to 'YYYY-MM-DD' in certificate-designs.ts: https://github.com/NEAR-Edu/near-certification-tools/blob/c3c77d7b9d905a17af66c131a40106c798eba966/web-app/helpers/certificate-designs.ts#L122

• I also added two test cases for this, please check the end of this update message

2. https://github.com/NEAR-Edu/near-certification-tools/issues/17#issuecomment-1098072866

Patricia’s case is solved. If query doesn’t return any result, the expiration date will be calculated as issue date + 180 https://github.com/NEAR-Edu/near-certification-tools/blob/c3c77d7b9d905a17af66c131a40106c798eba966/web-app/helpers/expiration-date.ts#L171

QUICK NOTE: - Changes in Query -

I have changed moment to be the start date of whatever condition we are looking for. moment is ALWAYS the start date now. Now, expiration date is also more accurate, since it is actually adding 180 days to the exact moment in the case of long period of inactivity (previously, 180 days was subtracted from the end of the long inactivity period, but this date was not always the same in time as the start of the inactivity period if that makes sense). I should have done this from the beginning really: https://github.com/NEAR-Edu/near-certification-tools/blob/c3c77d7b9d905a17af66c131a40106c798eba966/web-app/helpers/expiration-date.ts#L90 Naturally, the returning line of getExpiration has shortened with this change. No subtraction is needed since moment is ALWAYS the start date if query returns a result, I also changed diff_to_previous_activity to diff_to_next_activity since that is the case now: https://github.com/NEAR-Edu/near-certification-tools/blob/c3c77d7b9d905a17af66c131a40106c798eba966/web-app/helpers/expiration-date.ts#L171

In addition, getExpiration only returns these fields now: https://github.com/NEAR-Edu/near-certification-tools/blob/c3c77d7b9d905a17af66c131a40106c798eba966/web-app/helpers/expiration-date.ts#L11-L16 Expiration is ALWAYS moment + 180 days since moment is the start date of inactivity period or the most recent activity. So adding 180 days to this date to get the expiration date is accurate in all cases.

3. https://github.com/NEAR-Edu/near-certification-tools/issues/17#issuecomment-1098072866

Since issuing a cert doesn't appear to be an action that displays on account's transaction history: The first CTE (the first query) now also checks the period between account's first activity after the issue date, and the issue date.

https://github.com/NEAR-Edu/near-certification-tools/blob/c3c77d7b9d905a17af66c131a40106c798eba966/web-app/helpers/expiration-date.ts#L25-L131

What I did is, going from the inside out in the first query:

1. Each account action after issue date as moment_of_activity https://github.com/NEAR-Edu/near-certification-tools/blob/c3c77d7b9d905a17af66c131a40106c798eba966/web-app/helpers/expiration-date.ts#L96
2. First action after issue date as first_activity https://github.com/NEAR-Edu/near-certification-tools/blob/c3c77d7b9d905a17af66c131a40106c798eba966/web-app/helpers/expiration-date.ts#L91-L94
3. Days between first_activity and issue date as days_between_issue_date_and_first_activity https://github.com/NEAR-Edu/near-certification-tools/blob/c3c77d7b9d905a17af66c131a40106c798eba966/web-app/helpers/expiration-date.ts#L85
4. Lastly a case expression to return
   • Primarily the issue date as moment, in case _days_between_issue_date_and_firstactivity exceeds 180
   • or diff_to_next_activity if _days_between_issue_date_and_firstactivity < 180 AND there is an inactivity period exceeding again 180 days. In this case, moment is the start date of such period. https://github.com/NEAR-Edu/near-certification-tools/blob/c3c77d7b9d905a17af66c131a40106c798eba966/web-app/helpers/expiration-date.ts#L75-L82
5. If first query doesn't return anything, the second query goes through as before and returns the most recent activity if present.

4.

About 2021-09-13 vs 2021-09-12, that's weird. Even though you and I are in quite different time zones, and so it feels like it could be a time zone issue, I can't actually think of how it would be a time zone issue because we're not specifying the hour of day anyway. E.g. if I type "2021-03-16 + 180 days" into Google, I get 2021-09-12 as the result at https://convert-dates.com/days-from/180/2021/03/16#:~:text=Your%20starting%20date%20is%20March,would%20be%20September%2012%2C%202021. and https://www.wikidates.org/calculate/180-days-from-3-16-2021.html and https://whatisthedatetoday.com/180-days-from-march-16-2021.html and never see a different result.

You have already seen this but just to have it here too: I added the utc plugin wherever we are parsing a date string with dayjs to prevent it from parsing different dates depending on time zones.

5. https://github.com/NEAR-Edu/near-certification-tools/issues/17#issuecomment-1098072866

Or maybe it would suffice to add a note to the JSDoc of getExpiration to explain that it assumes that issuedAt would only be provided as the 2nd arg if it is real. E.g. the only time our webapp ever calls getExpiration is after we've already confirmed that the tokenId is legit:

I actually missed this, you're right. The account not existing shouldn't be an issue at the stage when getExpiration is called.

Continuing on next comment...

[eadsoy]

Changes in Test Cases

1. I added two test cases to test the isBeforeNow() function. https://github.com/NEAR-Edu/near-certification-tools/blob/c3c77d7b9d905a17af66c131a40106c798eba966/web-app/test/__tests__/expiration-date.test.ts#L297-L352

In order to compare now to a future / past expiration date that would work each time tests are run, I created a test helper function to generate data for a given account that populates account's data with the given interval and timeUnit(days, minutes etc.)

https://github.com/NEAR-Edu/near-certification-tools/blob/c3c77d7b9d905a17af66c131a40106c798eba966/web-app/test/test-helpers/generate-account-activities.ts#L16-L30

I then used this function wherever account data was needed between a certain period. I figured this made the seed file easier to read and made it easier to focus on the account data already set. For example:

https://github.com/NEAR-Edu/near-certification-tools/blob/c3c77d7b9d905a17af66c131a40106c798eba966/web-app/prisma/seed.ts#L77-L99

With the isBeforeNow test cases it also helped to generate dynamic data for the account. I used it to generate activity data 1 hour prior to and 1 hour after issue date:

https://github.com/NEAR-Edu/near-certification-tools/blob/c3c77d7b9d905a17af66c131a40106c798eba966/web-app/prisma/seed.ts#L349-L359

https://github.com/NEAR-Edu/near-certification-tools/blob/c3c77d7b9d905a17af66c131a40106c798eba966/web-app/prisma/seed.ts#L396-L406

[eadsoy]

I believe all the issues we saw are fixed now.

I will lastly add some more comments to the seed file and fix typos if I see any.

This took a long time really and I hope it will be used in production too but of course I am not attached to the code. I am also afraid some other surprises will pop up so I wouldn't be surprised if you decide to ditch it. It was really tiring but fun to learn along the way though. Thank you so much for your guidance and incredible patience. I really hope I am leaving everything as clear and well written as possible.

[ryancwalsh]

@eadsoy Thanks so much, Esin! I've read through this and haven't found any problems yet. I'm going to ask another new fellow to review it too, and then we can probably merge into develop and main.

[eadsoy]

@ryancwalsh I can't describe how good it feels to hear that, thank you!

[ryancwalsh]

@eadsoy This is merged to main and deployed. We'll be sending you a new cert soon as part of the first batch. :-)

[eadsoy]

That is amazing to hear, thanks for letting me know @ryancwalsh! I also saw the suggestion to use Prisma ORM code instead just now. I will check it out out of curiosity, but yeah, that's great news:)

[ryancwalsh]

I don't have the code in front of me, but yes, we want the expiration to be a particular moment (not rounded to the day) even though the value displayed on the certificate will be rounded to the day. But whether the certificate shows as expired or not should be precise to the moment.

So if that's not how it works yet, it's an important catch, and we need to fix it.

On Tue, Apr 5, 2022, 9:50 AM Esin Adsoy @.***> wrote:

It seems like you're already testing whether getExpiration returns the correct value. E.g.

This answers my question:) thank you

I think it would be a helpful exercise for us each to sit down with a blank sheet of paper and forget all of the code and the existing tests and instead just write out in plan easy-to-understand terms all of the scenarios we can think of, such as:

I am writing down the test cases in the format you suggested also trying to correct typos.

I have a question:

As I am writing the cases down, I realized something:

1. issued_at is in milliseconds, and the query checks everything after this. Even if the account had an activity 1 minute prior to issue_date, the query won’t pick that up as expected.
2. Then, in the getExpiration function while calculating the expiration date, we add or subtract days from the moment value depending on the scenario (again all in milliseconds).
3. Inside getExpiration, we use the formatDate function to only return the date string. If the expiration date is 2022-04-05T06:03:42.957Z, we pass it as ‘2022-04-05’ to the isBeforeNow function while rendering the certification, as if the expiration date is actually 2022-04-05T00:00:00.000Z.
4. The isBeforeNow function takes in this date string and checks whether it is a date before this moment and returns a boolean value; we then display the result.

So for example:

1. Query returns

result: [

{

  moment: '2022-10-07T17:39:24.572937+00:00',

  diff_to_previous_activity: null,

  has_long_period_of_inactivity: false

}

]

1. In getExpiration we take the moment from the query result and add 180 days in this case, which would be 2022-04-05T17:39:24.572Z in dayjs format.
2. Finally in getExpiration, we use the formatDate function to return the date string which is ‘2022-04-05’ in this case.
3. Then, the isBeforeNow function checks if this date is before this very moment which let’s say is 2022-04-05T06:16:45.470Z. Since the argument passed is ‘2022-04-05’ , isBeforeNow compares this moment to 2022-04-05T00:00:00.000Z and not 2022-04-05T17:39:24.572Z. There are hours for the certification to get expired, but it will display 'Expired'.

What I’m trying to say is, we are displaying a date on the certificate but it might be that if my certificate is showing today’s date, I might still see that it is expired for two reasons:

1. We are not comparing now to the actual time when the certificate expires.
2. If we ignore the actual time and just proceed with the date string, I think it's still strange to see that it is expired if I render my certificate on the expiration date the certificate is displaying. But this might be overthinking on my part.

So, shouldn't we compare now to the time as well instead of just the date?

— Reply to this email directly, view it on GitHub https://github.com/NEAR-Edu/near-certification-tools/issues/17#issuecomment-1088329617, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAP5MXNM2IET4TNS23WMXGTVDPPEVANCNFSM5OV4QKDA . You are receiving this because you were mentioned.Message ID: @.***>

[ryancwalsh]

This is bizarre. I definitely did not send this email at 05:02 ET when I was sleeping. It looks related to https://github.com/NEAR-Edu/near-certification-tools/issues/17#issuecomment-1097015748 in April 12