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

On /identities I have expired domains that don't seem expired #924

Closed
1 task
fricoben opened this issue Nov 25, 2024 · 18 comments · Fixed by #930
Closed
1 task

On /identities I have expired domains that don't seem expired #924

fricoben opened this issue Nov 25, 2024 · 18 comments · Fixed by #930
Assignees
Labels
Good first issue Good for newcomers OD Hack Issue reserved for the OD Hack Open for contribution An issue that is available for an Only Dust contribution

Comments

@fricoben
Copy link
Contributor

Description 📹

On /identities I have expired domains that don't seem expired, when I click on them I can see clearly they are 👇

Capture d’écran 2024-11-25 à 11 18 35

We need to add a warning label in red as we do with the subscription when a domain is expired to inform the user directly on /identity

Proposed Actions 🛠️

Here's a checklist of actions to follow for resolving this issue:

  1. Fork and Create Branch:
    Fork the repository and create a new branch using the issue number:

    git checkout -b fix-[issue-number]
  2. Implement Changes:

    • Update the warning condition in IdentitiesGalleryV1 component:
    // ... existing code ...
    {needAutoRenewal?.includes(identity.domain) && 
     (isIdentityExpired(identity) || isIdentityExpiringSoon(identity)) ? (
      <div className={styles.expiryWarning}>
        <Tooltip
          title={
            isIdentityExpired(identity)
              ? "This domain has expired! Renew it now to keep using it."
              : `This domain will expire on ${timestampToReadableDate(identity?.domain_expiry ?? 0)}`
          }
          arrow
        >
          <RebewalIcon color={isIdentityExpired(identity) ? "error" : "warning"} />
        </Tooltip>
      </div>
    ) : null}
    // ... existing code ...

This code exists right now but it doesn't work so please find out why (timestamp conversion problem or something else), moreover transform the warning (yellow) color in error (red) if it's expired).

  1. Run Tests and Commit Changes:
    Make sure your changes don't break existing functionality and commit with a clear message:
    git commit -m "Fix: Add distinct warning for expired domains"

Required 📋

To keep our workflow smooth, please make sure you follow these guidelines:

  • Assignment: Don't create a pull request if you weren't assigned to this issue.
  • Timeframe: Complete the task within 3 business days.
  • Closing the Issue: In your PR description, close the issue by writing Close #[issue_id].
  • Review Process:
    • Once you've submitted your PR, change the label to "ready for review".
    • If changes are requested, address them and then update the label back to "ready for review" once done.
  • Testing: Test your PR locally before pushing, and verify that tests and build are working after pushing.

Thank you for your contribution 🙏

⚠️ WARNING: Failure to follow the requirements above may result in being added to the OnlyDust blacklist, affecting your ability to receive future rewards.

@fricoben fricoben added Good first issue Good for newcomers OD Hack Issue reserved for the OD Hack Open for contribution An issue that is available for an Only Dust contribution labels Nov 25, 2024
@OWK50GA
Copy link

OWK50GA commented Nov 25, 2024

Hey, @fricoben
I can handle this issue, as the steps are stated clearly
I will implement the check to see whether or not the domain is expired or is expiring soon.
If these return true, I will implement the warning depending on which condition returned true, as shown above.

I will like to be assigned this issue

@tasneemtoolba
Copy link

tasneemtoolba commented Nov 25, 2024

Can I attempt this issue? read the proposed actions checklist and requirements, things are clear and straightforward

@PedroCo3lho
Copy link

Can I tackle this one?

@hoangkianh
Copy link

Would love to take this on. 💪

@Birdmannn
Copy link

Hi @fricoben
Can I hope on this?

@Emmanex01
Copy link

Let me handle this issue!

@JosueBrenes
Copy link

Hi, I'm Josué Brenes and I'll be working on issue.
I'm Dojo Coding member. ⛩️

I estimate this will take 1 day to complete.

My Plan for Adding a Warning Label for Expired Domains

1. Create a Branch

I will fork the repo and create a branch for the issue:

git checkout -b fix-[issue-number]

2. Debug and Implement Changes

Debug the Existing Code

I will identify why the current condition is not working, focusing on potential timestamp conversion issues. I will verify the following:

  • Ensure identity?.domain_expiry is returning the correct timestamp.
  • Confirm that the timestampToReadableDate() function is correctly converting the timestamp.

Implement the Fix

I will modify the IdentitiesGalleryV1 component to update the warning condition and fix the timestamp issue. Additionally, I will ensure the warning appears in red if the domain is expired. The corrected code will look like this:

{needAutoRenewal?.includes(identity.domain) &&
 (isIdentityExpired(identity) || isIdentityExpiringSoon(identity)) ? (
  <div className={styles.expiryWarning}>
    <Tooltip
      title={
        isIdentityExpired(identity)
          ? "This domain has expired! Renew it now to keep using it."
          : `This domain will expire on ${timestampToReadableDate(identity?.domain_expiry ?? 0)}`
      }
      arrow
    >
      <RebewalIcon color="error" />
    </Tooltip>
  </div>
) : null}

Key Fixes

  1. Ensure isIdentityExpired() and isIdentityExpiringSoon() return accurate results based on identity?.domain_expiry.
  2. Update the RebewalIcon color to "error" when the domain is expired.

3. Test Locally

I will ensure my changes work as expected and do not break existing functionality:

npm run build
npm run start

I will verify:

  • Expired domains display a red warning with the correct expiration message.
  • Domains close to expiration show a warning with the expected date.

4. Commit and Push

I will commit the changes with a clear message:

git add components/IdentitiesGalleryV1.tsx
git commit -m "Fix: Add red warning for expired domains in identities"
git push origin fix-[issue-number]

5. Submit PR

I will create a pull request referencing the issue:

Close #[issue_id]

6. Review and Finalize

I will label the PR as "ready for review," address feedback, and confirm all tests pass.

@Benjtalkshow
Copy link

Could I try solving this?

@MrRoudyk
Copy link

Can I take this issue?

@emarc99
Copy link
Contributor

emarc99 commented Nov 25, 2024

Is it okay if I tackle this? I will send screen as proof things work as expected.

@CEOliam
Copy link

CEOliam commented Nov 25, 2024

May I pick this up?

@pheobeayo
Copy link

Could I take over this issue?

@Kingsuite
Copy link

Could I grab this task?

@NueloSE
Copy link

NueloSE commented Nov 25, 2024

Would love to tackle this!

@mimisavage
Copy link

Could I be assigned to this?

@fricoben
Copy link
Contributor Author

assigned @emarc99

@emarc99
Copy link
Contributor

emarc99 commented Nov 26, 2024

Thanks, just seeing this. Getting to work.

@emarc99
Copy link
Contributor

emarc99 commented Nov 28, 2024

@fricoben I sent a DM on Telegram.

emarc99 added a commit to emarc99/app.starknet.id that referenced this issue Dec 2, 2024
emarc99 added a commit to emarc99/app.starknet.id that referenced this issue Dec 16, 2024
emarc99 added a commit to emarc99/app.starknet.id that referenced this issue Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good first issue Good for newcomers OD Hack Issue reserved for the OD Hack Open for contribution An issue that is available for an Only Dust contribution
Projects
None yet
Development

Successfully merging a pull request may close this issue.