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

Notification Lifecycle Commands #180

Merged
merged 5 commits into from
May 6, 2024
Merged

Notification Lifecycle Commands #180

merged 5 commits into from
May 6, 2024

Conversation

amydevs
Copy link
Contributor

@amydevs amydevs commented Apr 29, 2024

Description

This PR implements inbox and outbox subcommands for the notifications command. We have never used two layers of subcommands in other domains, so my usage of such needs to be reviewed.

Issues Fixed

Tasks

  • 1. inbox read
  • 2. inbox clear
  • 3. outbox read
  • 4. outbox clear

Final checklist

  • Domain specific tests
  • Full tests
  • Updated inline-comment documentation
  • Lint fixed
  • Squash and rebased
  • Sanity check the final build

@amydevs amydevs self-assigned this Apr 29, 2024
@amydevs amydevs force-pushed the feature-outbox branch 2 times, most recently from 1f11ddb to 3f9d0bc Compare May 2, 2024 02:00
@CMCDragonkai
Copy link
Member

How come the issue was closed if this is fixing it?

@CMCDragonkai
Copy link
Member

CMCDragonkai commented May 5, 2024

@aryanjassal might have some commentary here in terms of inspecting the notifications.

Maybe if the polykey agent status could provide some information to the end user.

@amydevs amydevs force-pushed the feature-outbox branch 3 times, most recently from e07744a to 2c081c8 Compare May 6, 2024 03:04
@CMCDragonkai
Copy link
Member

Hey @amydevs do not do version bumps in a feature branch!!! It can only be done in staging!!!!

@tegefaulkes
Copy link
Contributor

Hey @amydevs do not do version bumps in a feature branch!!! It can only be done in staging!!!!

That seems to be updating a dependency's version, is that an issue? I've done this before in branches, usually when the new version has changes specific to some feature I'm working on.

@amydevs
Copy link
Contributor Author

amydevs commented May 6, 2024

@tegefaulkes i think roger was mistaken due to the CI job failure

@amydevs
Copy link
Contributor Author

amydevs commented May 6, 2024

@CryptoTotalWar do u have any suggestions? if not, i'll merge and and start working on audit

@CryptoTotalWar
Copy link

CryptoTotalWar commented May 6, 2024

@CryptoTotalWar does everything look good? if so, i'll merge and and start working on audit

What am I reviewing specifically?

Also curious, similar to how we have feature-x.dev branches for polykey.com , is there such thing for Polykey CLI on some sort of testnet? That way we can do some real user interactions on a testnet before deploying to mainnet.

Furthermore, reading the issue ticket for the PR, it helps me understand a bit more context about what this PR is aiming to solve for but without having screenshots of it being tested, it's hard to know... anyway it boils down to actually being able to test new functionality and document the user-flow from a UX perspective.

@amydevs
Copy link
Contributor Author

amydevs commented May 6, 2024

@CryptoTotalWar i mistyped sorry, i meant more so if u have any feedback from the process that you experienced in polykey regarding the notifications system throughout your usage

@CMCDragonkai
Copy link
Member

@CryptoTotalWar does everything look good? if so, i'll merge and and start working on audit

What am I reviewing specifically?

Also curious, similar to how we have feature-x.dev branches for polykey.com , is there such thing for Polykey CLI on some sort of testnet? That way we can do some real user interactions on a testnet before deploying to mainnet.

Furthermore, reading the issue ticket for the PR, it helps me understand a bit more context about what this PR is aiming to solve for but without having screenshots of it being tested, it's hard to know... anyway it boils down to actually being able to test new functionality and document the user-flow from a UX perspective.

Yes there is it's called testnet.polykey.com.

@amydevs
Copy link
Contributor Author

amydevs commented May 6, 2024

Asked @CryptoTotalWar about the new commands and he said they seem sensible, merging now.

@amydevs amydevs merged commit 412d5fd into staging May 6, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Asynchronous Notification Architecture and Notification Lifecycle Commands
4 participants