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

feat: add Ticket api integration #115

Merged
merged 4 commits into from
Jun 24, 2024
Merged

feat: add Ticket api integration #115

merged 4 commits into from
Jun 24, 2024

Conversation

luisya22
Copy link
Contributor

Changes Made 🎉

  • feat: add supabase ticket api integration
  • feat: add placeholders for ticket image upload

Describe Changes

Integration with Supabase API to get Ticket information.
Add placeholder for image upload.

Visuals (Optional)

image

Checklist ✅

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

Copy link

vercel bot commented Apr 20, 2024

@luisya22 is attempting to deploy a commit to the aforina's projects Team on Vercel.

A member of the Team first needs to authorize it.

return;
}

await uploadTicket(providerId, ticketId, img);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@afordigital @Zyruks @jarrisondev do you think it is necessary to persist the ticket if it only downloads the image (taking into account that downloading does not require persisting)?

Copy link
Member

Choose a reason for hiding this comment

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

You mean you don't need to save it because it will always make the call to the database?

Copy link
Collaborator

@juanpablo-is juanpablo-is May 14, 2024

Choose a reason for hiding this comment

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

I mean, there are 2 buttons "Compartir" and "Descargar Ticket", the functionality to persist the image for later use as OG only serves if you want to share it on a social network (in this case in the "Compartir" button via Twitter), the action of "Descargar Ticket" is not necessary the functionality to persist and OG

In my case, I do not have Twitter, but if I want to download the ticket (to download, it is only to make a "screenshot" of the ticket and save it in the pc), in these cases it is not necessary to save in DB or anything

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed - I don't think persisting the ticket image is necessary unless it's shared via Twitter.

@afordigital
Copy link
Member

Amazing PR luis, thank you for all the time and the contribution!

uploadTicket from image dowload, change update to upsert when updating
ticket
@luisya22
Copy link
Contributor Author

The new commit resolve most of the requested changes. The only remaining concern is how are we going to implement the rate limiting for image upload.

Copy link
Contributor

@MarcosNASA MarcosNASA left a comment

Choose a reason for hiding this comment

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

This looks good to me, Luis. Great job. Regarding rate limiting, do you know whether Supabase offers some sort of control around it?

@@ -4,8 +4,9 @@ import { FC, RefObject, useRef } from 'react';
import { Variant } from '@common';
Copy link
Contributor

Choose a reason for hiding this comment

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

🟢 Nit: it would be awesome if downloadTicket and shareTwitter didn't depend on React-specifics (refs) but on the DOM elements directily.

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 means that I should use getElementById?

Copy link
Contributor

Choose a reason for hiding this comment

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

const downloadTicket = async (element: HTMLElement, ticketId: string | null, providerId: string | null) => {
  // TODO: Send Generated Image to Supabase
  if (ticketId && providerId) {
    try {
      const img = await toBlob(element);
      const dataUrl = await toPng(elementRef.current);

      if (!img) {
        console.error(); // TODO: Alert
        return;
      }

      const link = document.createElement('a');
      link.download = 'hackafor-ticket.png';
      link.href = dataUrl;
};

---
// Consuming it
if (!elementRef.current) return
downloadTicket(elementRef.current, ...)

This way, the downloadTicket function is no longer React-dependent. But again, it's a super nit, feel free to ignore!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your recommendation, I'll implement this.

return;
}

await uploadTicket(providerId, ticketId, img);
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed - I don't think persisting the ticket image is necessary unless it's shared via Twitter.


if (_event == 'INITIAL_SESSION') {
getUserTicket(session.user.id)
.then((ticket) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ I remember we mentioned we would want to persist Ticket entries (without image) as soon as someone logs-in for the first time so that logging in claims your ticket number - was this achieved in a different way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this was achieved using a trigger directly on Supabase

Copy link
Contributor

Choose a reason for hiding this comment

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

Right! Thanks, Luis!

Copy link
Contributor

@MarcosNASA MarcosNASA left a comment

Choose a reason for hiding this comment

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

I think we should be good to merge, shouldn't we?

@afordigital afordigital merged commit f0e8559 into Afordin:main Jun 24, 2024
2 of 3 checks passed
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.

4 participants