Skip to content

Add VAPID keys' generator for webpush notifications #8738

Merged
andreslucena merged 6 commits intodevelopfrom
feature/vapid_keys
Feb 2, 2022
Merged

Add VAPID keys' generator for webpush notifications #8738
andreslucena merged 6 commits intodevelopfrom
feature/vapid_keys

Conversation

@quinHD
Copy link
Copy Markdown
Contributor

@quinHD quinHD commented Jan 19, 2022 •
edited
Loading

🎩 What? Why?

Please describe your pull request.

📌 Related Issues

Link your PR to an issue

  • Related to #?
  • Fixes #?

Testing

Acceptance criteria

  • As a Decidim implementer, I have documentation on how to configure push notifications in the server side
  • Given that I'm a Decidim implementer,
    When I run rails decidim:generate
    Then I have the following ouput:
 VAPID keys correctly generated.
 ************************
 VAPID private key is XXXXXXXXXXXXXX
 VAPID public key is XXXXXXXXXXXXXX
  • Given that I'm a Decidim implementer,
    When I configure the VAPID_PRIVATE_KEY and VAPID_PUBLIC_KEY env vars
    Then the push notifications service is set up

Step by step
In the root folder, run the decidim:generate rake task. Copy the the keys and add them to the env variables using the names specified above.

📋 Checklist

🚨 Please review the guidelines for contributing to this repository.

  • ❓ CONSIDER adding a unit test if your PR resolves an issue.
  • ✔️ DO check open PR's to avoid duplicates.
  • ✔️ DO keep pull requests small so they can be easily reviewed.
  • ✔️ DO build locally before pushing.
  • ✔️ DO make sure tests pass.
  • ✔️ DO make sure any new changes are documented in docs/.
  • ✔️ DO add and modify seeds if necessary.
  • ✔️ DO add CHANGELOG upgrade notes if required.
  • ✔️ DO add to GraphQL API if there are new public fields.
  • ✔️ DO add link to MetaDecidim if it's a new feature.
  • ❌AVOID breaking the continuous integration build.
  • ❌AVOID making significant changes to the overall architecture.

📷 Screenshots

image

♥️ Thank you!

@quinHD quinHD self-assigned this Jan 19, 2022
@andreslucena andreslucena added project: PWA Barcelona City Council contract module: core type: feature labels Jan 24, 2022
@quinHD quinHD linked an issue Jan 24, 2022 that may be closed by this pull request
Server setup for Push Notifications #8488
Closed
3 tasks
@quinHD quinHD changed the title Add VAPID keys generator VAPID keys generator Jan 24, 2022
@quinHD
Copy link
Copy Markdown
Contributor Author

quinHD commented Jan 25, 2022

@andreslucena , añadí test de la rake task y el coverage sigue sin pasar

@andreslucena
Copy link
Copy Markdown
Member

andreslucena commented Jan 25, 2022

@andreslucena , añadí test de la rake task y el coverage sigue sin pasar

Please, let's talk in English here, so the rest of the community can understand us 😄

As I mentioned in the meeting today, for now the Codecov patch is not working correctly. I'll remove it altogether as having a check that doesn't work most of the time, it doesn't make much sense.

@andreslucena andreslucena mentioned this pull request Jan 25, 2022
Merged
@quinHD
Copy link
Copy Markdown
Contributor Author

quinHD commented Jan 25, 2022

@andreslucena , añadí test de la rake task y el coverage sigue sin pasar

Please, let's talk in English here, so the rest of the community can understand us 😄

As I mentioned in the meeting today, for now the Codecov patch is not working correctly. I'll remove it altogether as having a check that doesn't work most of the time, it doesn't make much sense.

Yep! of course! It was a mistake 😄
Ok, so I skip this check.

@ferblape
Copy link
Copy Markdown
Contributor

ferblape commented Jan 26, 2022

@quinHD I think decidim:generate is way too generic. What about decidim:generate_vapid_keys?

@quinHD
Copy link
Copy Markdown
Contributor Author

quinHD commented Jan 26, 2022

@quinHD I think decidim:generate is way too generic. What about decidim:generate_vapid_keys?

Ok. Let's ask @andreslucena , because I took the name that's in the issue.

andreslucena
andreslucena requested changes Jan 28, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

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

A couple of doubts to solve before approving

@andreslucena
Copy link
Copy Markdown
Member

andreslucena commented Jan 28, 2022

@quinHD I think decidim:generate is way too generic. What about decidim:generate_vapid_keys?

Oh, I've just seen your comments - but it's great that we're aligned in this one, I thought exactly the same 😅

My hypothesis is that it was a placeholder, or that I was just with too many things in my mind and missed that while defining the User Stories.

I was thinking about decidim:webpush:generate_keys - but decidim:generate_vapid_keys is great for me, as adding a namespace is too premature if we're only thinking in adding one task.

@andreslucena
Copy link
Copy Markdown
Member

andreslucena commented Jan 31, 2022

Watch out @quinHD
I think the last merge messed up the git history.

@quinHD quinHD force-pushed the feature/vapid_keys branch from dbc2a5b to 010b8d8 Compare January 31, 2022 19:43
andreslucena
andreslucena reviewed Feb 1, 2022
View reviewed changes
andreslucena
andreslucena reviewed Feb 1, 2022
View reviewed changes
andreslucena
andreslucena reviewed Feb 1, 2022
View reviewed changes
andreslucena
andreslucena reviewed Feb 1, 2022
View reviewed changes
@andreslucena andreslucena changed the title VAPID keys generator Add VAPID keys' generator for webpush notifications Feb 1, 2022
@andreslucena andreslucena merged commit 899a06b into develop Feb 2, 2022
@andreslucena andreslucena deleted the feature/vapid_keys branch February 2, 2022 08:27
@andreslucena andreslucena mentioned this pull request Feb 2, 2022
Merged
62 tasks
@ferblape
Copy link
Copy Markdown
Contributor

ferblape commented Feb 15, 2022

@andreslucena I've just realized something that might be wrong in this PR. Given that the task is defined in decidim-dev module, it's not available in a production environment, which was a surprise to me. I didn't notice this because I generated my keys in the local environment

I think we should move the task to decidim-core module. What do you think?

@andreslucena
Copy link
Copy Markdown
Member

andreslucena commented Feb 15, 2022

I think we should move the task to decidim-core module. What do you think?

As we've talked in the meeting a couple of minutes ago, I agree, let's move it to decidim-core please. I didn't catch that, neither. I'll add it as a note in the project, so we can keep track of it.

eliegaboriau pushed a commit to eliegaboriau/decidim that referenced this pull request Feb 21, 2022
@quinHD @eliegaboriau
Add VAPID keys' generator for webpush notifications (decidim#8738)
69d71a7
@quinHD quinHD mentioned this pull request Mar 1, 2022
Merged
12 tasks
@alecslupu alecslupu added this to the 0.27.0 milestone Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@microstudi microstudi microstudi left review comments

@andreslucena andreslucena andreslucena approved these changes

+1 more reviewer

@ferblape ferblape ferblape approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@quinHD quinHD

Labels

module: core project: PWA Barcelona City Council contract

Projects

None yet

Milestone

0.27.0

Development

Successfully merging this pull request may close these issues.

Server setup for Push Notifications

5 participants

@quinHD @andreslucena @ferblape @microstudi @alecslupu