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.

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

added requested feature to query allHehes. Resolved linting error fro… #261

Closed
wants to merge 1 commit into from

Conversation

Muncherkin
Copy link
Contributor

…m post.api.test.ts.

Issue #241: Nu finns en query, allHehes (med optional sortOrder), för att hämta alla hehes. All funktionalitet fanns redan så inga nya tester eller så, bara en rad till i graph:en och resolvern (hehe.graphql och hehe.resolver.ts).

@Muncherkin Muncherkin linked an issue Apr 19, 2023 that may be closed by this pull request
@github-actions
Copy link

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements 67.47% 1350/2001
🔴 Branches 44.49% 218/490
🔴 Functions 57.55% 240/417
🟡 Lines 67.24% 1281/1905

Test suite run failed

Failed tests: 2/237. Failed suites: 1/19.
  ● forget a user

    thrown: "Exceeded timeout of 5000 ms for a test.
    Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."

      317 | });
      318 |
    > 319 | test('forget a user', async () => {
          | ^
      320 |   const couldForget = await api.forgetUser(mockNewUser0.username); // How could you forget me?
      321 |   expect(couldForget).toBeTruthy();
      322 |

      at Object.<anonymous> (test/unit/user.api.test.ts:319:1)

  ● reset password properly

    thrown: "Exceeded timeout of 5000 ms for a test.
    Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."

      329 | });
      330 |
    > 331 | test('reset password properly', async () => {
          | ^
      332 |   await api.createUser(mockNewUser1);
      333 |   const token = await api.requestPasswordReset(mockNewUser1.username);
      334 |   const newPassword = 'Detta test skrevs på valmötet 2021';

      at Object.<anonymous> (test/unit/user.api.test.ts:331:1)

Report generated by 🧪jest coverage report action from 2887780

@github-actions
Copy link

I have deployed this PR to feat-allhehes-ekorre.review.esek.se 🚀

@ginger51011
Copy link
Member

Är inte detta samma sak som latestHehes, där limit är null?

@Muncherkin
Copy link
Contributor Author

Är inte detta samma sak som latestHehes, där limit är null?

Jo men Froborg tyckte det var fult. Låg kvar som en "issue" så jag la till det.

Copy link
Member

@ginger51011 ginger51011 left a comment

Choose a reason for hiding this comment

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

LGTM

(Men lägg gärna till dig i contributors i package.json om du inte är där)

Copy link
Member

@afroborg afroborg left a comment

Choose a reason for hiding this comment

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

Before we merge this we really should start looking into paginating our all-x-reponses. This is not an issue now, but once we start having a few months of stuff it's really gonna impact load time.

I suggest we add some more fields:

  • page_size / limit: the max number of items returned
  • page: the page we want

The response is going to be more complex, because we need to return information about nextPage, totalCount etc. @blennster has done this in the past on other projects.

@afroborg
Copy link
Member

Also - with this resolver I don't really see the need for the latestHehes resolver anymore. Removing it won't break anything that I know of, don't think we use it anywhere.

@Muncherkin
Copy link
Contributor Author

I'd be happy to remove the latestHehe query. I'll look into the paging thing. Feels a bit like where solving a non-issue though just cause we weren't a fan of the label latest lol

@Muncherkin
Copy link
Contributor Author

Same thing as latestHehes(null), closing PR.

@Muncherkin Muncherkin closed this Nov 20, 2023
@Muncherkin Muncherkin deleted the feat/allHehes branch November 20, 2023 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Get all hehes"
3 participants