Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat: set image for android notification #19

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

Merged
lkaybob merged 2 commits into lkaybob:master from mgcastelberg:android-image-notification
Nov 30, 2023

Conversation

@mgcastelberg
Copy link
Contributor

@mgcastelberg mgcastelberg commented Nov 2, 2023

Goal

Send image on android notification

  • Add setImage for android notification
  • Add testSetImage unit tests and integration test

Copy link
Owner

lkaybob commented Nov 5, 2023

Guess there's something wrong with GitHub Actions. I'll look into it and make sure the action runs properly.

@lkaybob lkaybob self-assigned this Nov 5, 2023
Copy link
Contributor Author

mgcastelberg commented Nov 7, 2023 via email

OK, thank you very much for reviewing my contribution, I hope the part of the github actions is resolved soon, once you tell me that everything is in order, I will upload the second part to send images for IOS Have a great night. Atte. Ing. José Manuel Gómez Castelberg Diseñador / Programador We Play Code S. A. de C. V. Jugamos código
...
________________________________ De: Sunghyuk Kay ***@***.***> Enviado: domingo, 5 de noviembre de 2023 02:01 a. m. Para: lkaybob/php-fcm-v1 ***@***.***> Cc: Manuel Gómez ***@***.***>; Author ***@***.***> Asunto: Re: [lkaybob/php-fcm-v1] feat: set image for android notification (PR #19) Guess there's something wrong with GitHub Actions. I'll look into it and make sure the action runs properly. — Reply to this email directly, view it on GitHub<#19 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACFAT3LQF63ZTOKD2OPF5LTYC5BWBAVCNFSM6AAAAAA62GYXHWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOJTGY3DMOJWGU>. You are receiving this because you authored the thread.Message ID: ***@***.***>

Copy link
Owner

lkaybob commented Nov 12, 2023
edited
Loading

@mgcastelberg Sorry for the delay. I guess I found the issue.

In my GA manifest, there is a step that decrypts encrypted service account file(service_account.json.gpg), which requires repository secret. Github Actions does allow repository secrets to be shared with forked repository for specific event(pull_request_target), but it requires Github Actions manifest file to be updated.

So I guess there should be two options so far.

Option 1) Rebasing merge request.
I update the GA manifest file on master branch first, and then you rebase your working branch to updated master branch. This method is canonical way to do in my opinion. Although it requires few tricky local git commands on both sides, this would definitely make sure that GA works fine.

Option 2) Temporarily set secret on your side
As a workaround other than first option, I can share you the secret, and you set it on your forked repository. This one does not require git interactions, but I'm not sure this method would work, but at least we can try.

If option 2 works, I will update encrypted service account file with different passphrase and GA manifest file, so that you can work on iOS part. (Of course, if option 2 does not work, we should stick to first options :( )

Please let me know which do you prefer at first.

Copy link
Contributor Author

mgcastelberg commented Nov 12, 2023 via email

Hello lkaybob, if you like, we will try the easiest way, which is option 2, and if this definitely does not work, we will use the other alternative.Likewise, if you consider that option 1 is safer and guarantees greater certainty, we can try it, I would just need your help to see what steps I should take.Thank you in advance for following up on my participation, I am very happy to be learning new things with Git and collaborating with you. Atte. Ing. José Manuel Gómez Castelberg Diseñador / Programador We Play Code S. A. de C. V. Jugamos código
...
________________________________ De: Sunghyuk Kay ***@***.***> Enviado: domingo, 12 de noviembre de 2023 03:01 a. m. Para: lkaybob/php-fcm-v1 ***@***.***> Cc: Manuel Gómez ***@***.***>; Mention ***@***.***> Asunto: Re: [lkaybob/php-fcm-v1] feat: set image for android notification (PR #19) @mgcastelberg<https://github.com/mgcastelberg> Sorry for the delay. I guess I found the issue. In Github Actions, there is a step that decrypts encrypted service account file(service_account.json.gpg), which requires repository secret. Github Actions does allow repository secrets to be shared with forked repository for specific event(pull_request_target), but it requires Github Actions manifest file<https://github.com/lkaybob/php-fcm-v1/blob/master/.github/workflows/main.yml> to be updated. So I guess there should be two options so far. Option 1) Rebasing merge request. I update the GA manifest file on master branch first, and then you rebase your working branch to updated master branch. This method is canonical way to do in my opinion. Although it requires few tricky local git commands on both sides, this would definitely make sure that GA works fine. Option 2) Temporarily set secret on your side As a workaround other than first option, I can share you the secret, and you set it on your forked repository. This one does not require git interactions, but I'm not sure this method would work, but at least we can try. If option 2 works, I will update encrypted service account file with different passphrase and GA manifest file, so that you can work on iOS part. (Of course, if option 2 does not work, we should stick to first options :( ) Please let me know which do you prefer. — Reply to this email directly, view it on GitHub<#19 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACFAT3LCTH7VCXWWVGSXVD3YECGANAVCNFSM6AAAAAA62GYXHWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBXGA3DENJSGA>. You are receiving this because you were mentioned.Message ID: ***@***.***>

Copy link
Owner

lkaybob commented Nov 14, 2023
edited
Loading

@mgcastelberg Thank you for your understanding. Let's try second option first.

Could you send me an email to hello@lkaybob.pe.kr? (Since I can't see email address on your Github profile)

After you sending me an email, I will reply with passphrase and a guide to set secret on forked reository.

Copy link
Contributor Author

mgcastelberg commented Nov 14, 2023 via email

Hello Sunghyuk Kay I already sent you my email from my personal account Greetings Atte. Ing. José Manuel Gómez Castelberg Diseñador / Programador We Play Code S. A. de C. V. Jugamos código
...
________________________________ De: Sunghyuk Kay ***@***.***> Enviado: lunes, 13 de noviembre de 2023 07:09 p. m. Para: lkaybob/php-fcm-v1 ***@***.***> Cc: Manuel Gómez ***@***.***>; Mention ***@***.***> Asunto: Re: [lkaybob/php-fcm-v1] feat: set image for android notification (PR #19) Thank you for your understanding. Let's try second option first. Could you send me an email to ***@***.******@***.***>? (Since I can't see email address on your Github profile) After you sending me an email, I will reply with passphrase and a guide to set secret on forked reository. — Reply to this email directly, view it on GitHub<#19 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACFAT3KNKOMLMGTLYHTKLHLYELADJAVCNFSM6AAAAAA62GYXHWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBZGM4DCNBSGY>. You are receiving this because you were mentioned.

Copy link
Owner

lkaybob commented Nov 22, 2023

@mgcastelberg Sorry for the delay. I was busy for a while, and I've done some research on first option that I suggested, since second option didn't go well.

It seems Github Actions does not work as I understood, so restoring GA workflow would need additional administrative actions. (ex. Changing mechanism to test library with actual service account.)

I will assume your code runs well, so let's merge after my review.

Copy link
Owner

@lkaybob lkaybob left a comment

Choose a reason for hiding this comment

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

Feel free to leave your opinion on the review.

Copy link
Contributor Author

@mgcastelberg mgcastelberg left a comment

Choose a reason for hiding this comment

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

Once approved, I will perform a couple of tests from my project to verify that everything is in order.

lkaybob reacted with thumbs up emoji
@lkaybob lkaybob merged commit c015baa into lkaybob:master Nov 30, 2023
Copy link
Owner

lkaybob commented Nov 30, 2023

Thank you for your contribution. @mgcastelberg

I'm planning to create a release after your next contribution, since you are planning to add image send support for iOS. If you want to test the merged code so far, I guess you have to specify your project's composer to get package from master branch.

FYI. Tests ran successfully after the merge.
https://github.com/lkaybob/php-fcm-v1/actions/runs/7043311400/job/19168864687

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lkaybob lkaybob lkaybob requested changes

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /