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.

Sign up for GitHub

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

Jump to bottom

Support multi status entries #40

Merged
merged 7 commits into from
Aug 12, 2024
Merged

Support multi status entries #40

merged 7 commits into from
Aug 12, 2024

Conversation

PatStLouis
Copy link
Contributor

This is to support testing the DB BitstringstatusList implementation.

This isn't transforming the original credential but creating a new variable statusEntries

@PatStLouis
fix if id statement
9142559 
Signed-off-by: Patrick <[email protected]>
aljones15
aljones15 reviewed Aug 10, 2024
View reviewed changes
tests/10-issue.js Outdated Show resolved Hide resolved
aljones15
aljones15 reviewed Aug 10, 2024
View reviewed changes
tests/10-issue.js Outdated Show resolved Hide resolved
aljones15
aljones15 reviewed Aug 10, 2024
View reviewed changes
tests/10-issue.js Outdated Show resolved Hide resolved
aljones15
aljones15 reviewed Aug 10, 2024
View reviewed changes
tests/10-issue.js Outdated Show resolved Hide resolved
aljones15
aljones15 reviewed Aug 10, 2024
View reviewed changes
tests/10-issue.js Outdated Show resolved Hide resolved
aljones15
aljones15 reviewed Aug 10, 2024
View reviewed changes
tests/10-issue.js Outdated Show resolved Hide resolved
aljones15
aljones15 suggested changes Aug 10, 2024
View reviewed changes
Copy link
Contributor

@aljones15 aljones15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, and excellent work just have a few little concerns and some javascript related comments.

@PatStLouis PatStLouis requested a review from aljones15 August 12, 2024 00:50
aljones15
aljones15 approved these changes Aug 12, 2024
View reviewed changes
Copy link
Contributor

@aljones15 aljones15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good provided the typos can be fixed.

tests/10-issue.js Outdated Show resolved Hide resolved
tests/10-issue.js Outdated Show resolved Hide resolved
tests/10-issue.js Outdated
statusEntry.should.have.own.property(
'statusMessage');
} else {
if(statusEntry === statusEntries[statusEntries.lenght - 1]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if(statusEntry === statusEntries[statusEntries.lenght - 1]) {
if(statusEntry === statusEntries[statusEntries.length - 1]) {

tests/10-issue.js Outdated
an('array').length.should.be.
equal(statusEntry.statusSize);
} else {
if(statusEntry === statusEntries[statusEntries.lenght - 1]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if(statusEntry === statusEntries[statusEntries.lenght - 1]) {
if(statusEntry === statusEntries[statusEntries.length - 1]) {

tests/10-issue.js Outdated
statusEntry.statusMessage.should.be.
an('array').length.should.be.
equal(statusEntry.statusSize,
'Expected statusMessage lenght to be equal to ' +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'Expected statusMessage lenght to be equal to ' +
'Expected statusMessage length to be equal to ' +

tests/10-issue.js Outdated
'Expected statusMessage lenght to be equal to ' +
'statusSize.');
} else {
if(statusEntry === statusEntries[statusEntries.lenght - 1]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if(statusEntry === statusEntries[statusEntries.lenght - 1]) {
if(statusEntry === statusEntries[statusEntries.length - 1]) {

tests/10-issue.js Outdated
statusEntry.should.have.own.property(
'statusMessage');
} else {
if(statusEntry === statusEntries[statusEntries.lenght - 1]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if(statusEntry === statusEntries[statusEntries.lenght - 1]) {
if(statusEntry === statusEntries[statusEntries.length - 1]) {

tests/10-issue.js Outdated
'be processed as 1.',
async function() {
this.test.link = 'https://www.w3.org/TR/vc-bitstring-status-list/#:~:text=If%20statusSize%20is%20not%20present%20as%20a%20property%20of%20the%20credentialStatus%2C%20then%20statusSize%20MUST%20be%20processed%20as%201.';
if(statusEntry === statusEntries[statusEntries.lenght - 1]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if(statusEntry === statusEntries[statusEntries.lenght - 1]) {
if(statusEntry === statusEntries[statusEntries.length - 1]) {

tests/10-issue.js Show resolved Hide resolved
tests/10-issue.js Outdated
statusListCredential.credentialSubject;
const {encodedList} = credentialSubject;
// Uncompress encodedList
const decoded = await sl.decodeList({encodedList});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might want to do something like:

let decoded;
let error;
try {
  decoded = await sl.decodedList({encodedList});
} catch(e) {
  error = e;
} 
should.not.exist(error)

You get the idea. while it would definitely be outside of the scope of this PR, we have existing assertions for multibase encoding.

BigBlueHat
BigBlueHat requested changes Aug 12, 2024
View reviewed changes
Copy link
Member

@BigBlueHat BigBlueHat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking merge until those lenght typos are fixed.

Those also highlight that those particular lines weren't run...and that you probably need a better editor/IDE which would have complained more loudly.

aljones15
aljones15 reviewed Aug 12, 2024
View reviewed changes
tests/10-issue.js
issuedVc.credentialStatus.statusMessage.should.be.
an('array').length.should.be.
equal(issuedVc.credentialStatus.statusSize,
'Expected statusMessage lenght to be equal to ' +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still got lenght here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in the removed sections!

@aljones15
Copy link
Contributor

Blocking merge until those lenght typos are fixed.

Those also highlight that those particular lines weren't run...and that you probably need a better editor/IDE which would have complained more loudly.

I believe he doesn't have any test data that makes those statements fire.

@PatStLouis
Copy link
Contributor Author

@aljones15 correct, most of these statements are in the statusSize / statusMessage which is currently at risk and has no implementations. This is not a feature I will be implementing (at least not for now). Length is one of those words I always type wrong for some reason.

@PatStLouis
Copy link
Contributor Author

Blocking merge until those lenght typos are fixed.

Those also highlight that those particular lines weren't run...and that you probably need a better editor/IDE which would have complained more loudly.

I use VSCode with the ESLint plugin from Microsoft, always happy to hear better tooling suggestions

@BigBlueHat
Copy link
Member

I use VSCode with the ESLint plugin from Microsoft, always happy to hear better tooling suggestions

It's weird that didn't catch this. No worries. Just suggestions. 😄 Sadly, we can't test the tests without stuff to test against...

@PatStLouis PatStLouis merged commit c84a866 into main Aug 12, 2024
2 checks passed
@PatStLouis PatStLouis deleted the support-multiple-statuses branch August 12, 2024 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aljones15 aljones15 aljones15 approved these changes

@BigBlueHat BigBlueHat BigBlueHat approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@PatStLouis @aljones15 @BigBlueHat