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

フィードバックの投稿を組織メールで通知 #146

Merged
merged 9 commits into from
May 27, 2024

Conversation

keita-yasuda
Copy link
Contributor

Issues:101

・リソースコメント投稿・利活用方法投稿・利活用方法コメント投稿時に対象組織のAdminユーザに通知メールを送信
・送信メールのテンプレートファイルを独自に定義

@keita-yasuda keita-yasuda force-pushed the feature/notice-function branch 2 times, most recently from 4ea720e to eafbcb5 Compare April 12, 2024 09:16
@Sheile
Copy link
Contributor

Sheile commented Apr 15, 2024

ブランチの元コミット

後述するテストの問題はありますが、PRを切るときは対象ブランチの最新から切るべきです。
4月9日にmainブランチが更新されているため、mainブランチを最新化した上でrebaseをお願いします。

ユニットテスト

mainブランチを最新化するとckan 2.10への対応が含まれるため、ユニットテストが動かなくなっているようです。
今回の修正とは関係ないですが、これは早めに直しておきたいですね。

それはそれとして、今回対応した箇所でもユニットテストが落ちているので、対応しておきたいです。
(修正箇所などは別途コメントします)
https://github.com/c-3lab/ckanext-feedback/actions/runs/8659812951/job/23746449259?pr=146#step:10:130

@keita-yasuda keita-yasuda force-pushed the feature/notice-function branch 2 times, most recently from 2963a28 to 130f8f6 Compare April 26, 2024 01:43
@keita-yasuda keita-yasuda marked this pull request as ready for review April 26, 2024 01:47
* Utilizationの新規投稿通知に使用する件名

```ini
ckan.feedback.notice.email.subject_util = Post a Utilization
Copy link
Contributor

Choose a reason for hiding this comment

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

utilだとutilityと捉えられるので省略せずにutilizationでお願いしたいです。
79行目も同様

CKAN_SMTP_STARTTLS=True
CKAN_SMTP_USER=user
CKAN_SMTP_PASSWORD=pass
```
Copy link
Contributor

Choose a reason for hiding this comment

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

SMTPの設定はiniからでも可能である旨を記載ください。


```bash
ckan -c ckan.ini jobs worker
```
Copy link
Contributor

Choose a reason for hiding this comment

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

ローカルのdokcer-composeで実行する際の手順も欲しいです。必要があればdocker-composeに追記をお願いします。

@@ -3,6 +3,10 @@ docker exec -it ckan-docker-ckan-dev-1 bash -c "chmod 777 /srv/app/src/ckan/ckan
# pip install ckanext-feedback
docker exec -it ckan-docker-ckan-dev-1 bash -c "pip install /srv/app/src_extensions/ckanext-feedback"
# add ckanext-feedback to ckan.plugins
docker exec -it ckan-docker-ckan-dev-1 bash -c "sed -i '168s/$/ feedback/' /srv/app/ckan.ini"
docker exec -it ckan-docker-ckan-dev-1 bash -c "sed -i 's/\(ckan\.plugins = .*\)/\1 feedback/' /srv/app/ckan.ini"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: ckan.pluginsを行数ではなく正規表現で指定する方法に修正。

Comment on lines +14 to +31
context: ckan/
dockerfile: Dockerfile.dev
args:
- TZ=${TZ}
env_file:
- .env
depends_on:
db:
condition: service_healthy
solr:
condition: service_healthy
redis:
condition: service_healthy
volumes:
- ../../../:/srv/app/src_extensions/ckanext-feedback
- /srv/app/src_extensions/ckanext-feedback/development/
restart: unless-stopped
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: やりたい事は、ckan-dev: と同じ作りのコンテナでワーカーの起動。
参考元:https://github.com/ckan/ckan-docker/blob/cc095f1323a6dc81d49ae76c5c24bc1d9a1c0172/docker-compose.dev.yml#L11-L30

  • dockerfile:はこのファイルのckan-dev:が使用するファイルを指定している。
  • ワーカーへのアクセスを必要とするモジュールが存在しないので、参考元には存在するports:の定義はこのコンテナでは不要。
  • volumes:はこのファイルのckan-dev:の設定を踏襲。

docker exec -it ckan-docker-ckan-dev-1 bash -c "ckan feedback init"
# Copy ckan.datapusher.api_token setting value from ckan-dev to ckan-worker in ckan.ini
docker exec ckan-docker-ckan-dev-1 sh -c "grep '^ckan.datapusher.api_token' /srv/app/ckan.ini" | xargs -I {} docker exec ckan-docker-ckan-worker-1 sh -c "sed -i '/^\[app:main\]/a {}' /srv/app/ckan.ini"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: ワーカーがckan-devと連携する為には、ckan-devコンテナのckan.inickan.datapusher.api_tokenの設定値を、ワーカーコンテナ内のckan.iniに追記する必要があったため、該当項目の設定のコピーを行っている。

@ryo-ma
Copy link
Contributor

ryo-ma commented May 23, 2024

コンフリクトをしているのでリベースの対応もお願いします。

Enable configuration of notification settings from ckan.ini and feedback_config.json.
Preferences in feedback_config.json take precedence.

Settings include:
- Enabling/disabling the feature
- Directory for storing email templates
- Email template names
- Subject lines
* Accept email template, subject, and content as arguments
* Define available templates and subject in the configuration file
* It is necessary to start a worker inside the container using 'ckan -c ckan.ini jobs worker'.
@ryo-ma ryo-ma merged commit 909f97f into main May 27, 2024
0 of 2 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.

None yet

3 participants