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.

Sign up for GitHub

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

Jump to bottom

広告の曜日を設定できるように #10095

Merged
merged 15 commits into from
Jul 7, 2023
Merged

広告の曜日を設定できるように #10095

merged 15 commits into from
Jul 7, 2023

Conversation

nenohi
Copy link
Contributor

@nenohi nenohi commented Feb 25, 2023

What

広告に曜日の設定をできるように修正を行った

全てにチェックが入っていない場合も表示するようにしている

Why

毎週何曜日にこの広告を優先度○で出してほしい等の要望がある

Additional info (optional)

ローカル環境にてテスト済み

下記確認
・チェック後のupdateにて設定値が入っていること
・チェックを外した際にuptdateにて規定値が入っていること
・ラベルクリック時にクリックした項目の値が入っていること(保存時に確認
・チェックが入っていない場合の広告は表示されない(別曜日にてチェックが入っている場合
・チェックが入っている曜日の広告が表示される
・どの曜日にもチェックが入っていない場合は表示される

@github-actions github-actions bot added packages/backend Server side specific issue/PR packages/frontend Client side specific issue/PR labels Feb 25, 2023
@codecov
Copy link

codecov bot commented Feb 25, 2023 •
edited
Loading

Codecov Report

Merging #10095 (8082a08) into develop (209d8b4) will increase coverage by 48.64%.
The diff coverage is 86.36%.

@@             Coverage Diff              @@
##           develop   #10095       +/-   ##
============================================
+ Coverage    24.72%   73.36%   +48.64%     
============================================
  Files          705      809      +104     
  Lines        65224    77526    +12302     
  Branches      2303     5403     +3100     
============================================
+ Hits         16125    56880    +40755     
+ Misses       49099    20646    -28453
Impacted Files Coverage Δ
...ackend/src/server/api/endpoints/admin/ad/create.ts 74.54% <66.66%> (+74.54%) ⬆️
...ackend/src/server/api/endpoints/admin/ad/update.ts 75.00% <66.66%> (+75.00%) ⬆️
packages/backend/src/server/api/endpoints/meta.ts 97.99% <91.66%> (+97.99%) ⬆️
packages/backend/src/models/entities/Ad.ts 92.75% <100.00%> (+0.32%) ⬆️

... and 638 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@syuilo
Copy link
Member

syuilo commented Feb 26, 2023 •
edited
Loading

広告に曜日を紐づけたいケースがあんまり理解できていない
(男性のみに見せたいとか、若年層のみに見せたいとかはあるけど、この曜日だけに見せたい、というのはどういう意図だろう)

@argxentakato
Copy link

カレーの日を告知するとか…?

@acid-chicken
Copy link
Member

広告に曜日を紐づけたいケースがあんまり理解できていない

メディアを週刊で更新するなら更新タイミングの日や次の日などに広告を打つと効率的

@nenohi
Copy link
Contributor Author

nenohi commented Feb 26, 2023

広告に曜日を紐づけたいケースがあんまり理解できていない

例えばユーザーが多い休日のみをターゲットにするとか、毎週その曜日にyoutube等に動画がアップされるのでその日だけ告知したい等がある

(現在ioでも毎週日曜日に指定した広告を流してほしいとのこともあったので

rinsuki
rinsuki reviewed Feb 27, 2023
View reviewed changes
Copy link
Contributor

@rinsuki rinsuki left a comment •
edited
Loading

Choose a reason for hiding this comment

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

サーバーのタイムゾーン周りで不可解な挙動になりそうなのでタイムゾーン指定までさせたほうがいいかもしれない

packages/backend/src/server/api/endpoints/users/notes.ts Outdated Show resolved Hide resolved
@nenohi
Copy link
Contributor Author

nenohi commented Feb 28, 2023

サーバーのタイムゾーン周りで不可解な挙動になりそうなのでタイムゾーン指定までさせたほうがいいかもしれない

これは今の修正みたいな感じで指定するので合ってる？

@rinsuki
Copy link
Contributor

rinsuki commented Feb 28, 2023 •
edited
Loading

いや、サーバーのタイムゾーンがどこかわからない (基本 UTC な気はするが場合によっては普通に現地時間=日本だったら Asia/Tokyo だったりする) ので管理者が曜日を設定する時にタイムゾーンも設定できたほうがいいという話をしたかった

@nenohi
Copy link
Contributor Author

nenohi commented Feb 28, 2023

広告ごとにタイムゾーン決めて
になるのか、
サーバーで１つのタイムゾーンを決めて
になるとどっちになる？

@rinsuki
Copy link
Contributor

rinsuki commented Feb 28, 2023

広告ごとにタイムゾーン決めてあったほうが英語圏とかでは嬉しい気がするけどそういうこと言い出すとクエリが面倒になる気がしてきた (面倒だったらとりあえずサーバーの現在タイムゾーン表示 & 変更するには TZ 環境変数で指定せいってUIに書いておくでいい気もする)

@nenohi
Copy link
Contributor Author

nenohi commented Feb 28, 2023

クエリはもうちょっと面倒になりそう
曜日選択のところに
曜日はサーバーのタイムゾーンを元に指定されます
ぐらいの表記は必要そうかも

@nenohi
Copy link
Contributor Author

nenohi commented Feb 28, 2023

UIもうちょっといい感じ™にしたいけどわからん
image

@nenohi
Copy link
Contributor Author

nenohi commented Mar 1, 2023

こんな感じが良さそう(1つがデカくなりすぎ？
image

fix bug(広告追加後、保存を押さずに削除するとidが無いためエラーとなるバグ

@nenohi
Copy link
Contributor Author

nenohi commented Mar 3, 2023 •
edited
Loading

良さそうになってきた
image

@nenohi
Copy link
Contributor Author

nenohi commented Mar 3, 2023

どうでしょう？
localesは全部に適用する必要がある？

@syuilo
Copy link
Member

syuilo commented Mar 20, 2023

例えばユーザーが多い休日のみをターゲットにするとか

曜日を選択できるようにするということは、逆にユーザーの少ない平日のみをターゲットにしたいユースケースも存在するということ？

@nenohi
Copy link
Contributor Author

nenohi commented Mar 20, 2023

そういうこともできたりする
今希望されてるのはYoutube更新の曜日だけ掲載したいっていうのがある

@u1-liquid
Copy link
Contributor

これってまだ修正必要な部分あったりします？（なかったらマージしてもよさそう）

@syuilo syuilo merged commit 3c6175d into misskey-dev:develop Jul 7, 2023
@syuilo
Copy link
Member

syuilo commented Jul 7, 2023

👍🏻

slofp pushed a commit to Secineralyr/misskey.dream that referenced this pull request Jul 21, 2023
@nenohi @acid-chicken @slofp
広告の曜日を設定できるように (misskey-dev#10095)
70f42fe 
* 曜日選択できるように

* ラベル選択でもチェックが変更されるように

* adを参照しないといけないかも

* smallint -> integer

* 異物混入だったので取りだし

* タイムゾーン指定(Date2つ使うのなんか違和感

* 未テスト

* これにすると出てこないかも

* UIチョット変更

* UI変更 fix bug

* 畳むように修正

* dayofweek->dayOfWeek

* マイグレ時にnot null,default設定してるのでnullable:falseでよさそう

* コメントの記載

* Update packages/backend/src/server/api/endpoints/meta.ts

Co-authored-by: Acid Chicken (硫酸鶏) <[email protected]>

---------

Co-authored-by: Acid Chicken (硫酸鶏) <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@syuilo syuilo syuilo left review comments

@rinsuki rinsuki rinsuki left review comments

@acid-chicken acid-chicken acid-chicken left review comments

Assignees
No one assigned
Labels
packages/backend Server side specific issue/PR packages/frontend Client side specific issue/PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nenohi @syuilo @argxentakato @acid-chicken @rinsuki @u1-liquid