fix: Custom logo not displayed on shared page #7205

miya · 2023-01-04T05:57:02Z

Task

#112434 [GROWI][Next.js] カスタムロゴ設定した状態で、任意のページのsharelink にアクセスすると、ロゴが表示されない
└ #112474 修正

miya · 2023-01-04T06:00:29Z

packages/app/src/server/middlewares/certify-shared-file.js

+    if (isBlandLogo) {
+      req.isSharedPage = true;
+      return next();
+    }


attachmentType が BRAND_LOGO の attachment は relatedPage が null となるため、42 行目以降の処理は不要にして、shared page で表示できるようにしました。

brand logo かどうかの判定ロジックは Attachment model or service に持っていく

req.isSharedPage = true; セットするのは不自然

そもそも shared page からのアクセスだろうがそれ以外からのアクセスだろうが、brand logo は pass させたいわけだから

certifyBrandLogo middleware をつくるか、または /attachment/brand-logo route を作っちゃうか

yuki-takei · 2023-01-04T07:20:02Z

packages/app/src/server/middlewares/certify-shared-file.js

+    if (isBlandLogo) {
+      req.isSharedPage = true;
+      return next();
+    }


brand logo かどうかの判定ロジックは Attachment model or service に持っていく

req.isSharedPage = true; セットするのは不自然

そもそも shared page からのアクセスだろうがそれ以外からのアクセスだろうが、brand logo は pass させたいわけだから

certifyBrandLogo middleware をつくるか、または /attachment/brand-logo route を作っちゃうか

miya · 2023-01-04T09:50:14Z

packages/app/src/server/routes/index.js

@@ -106,6 +107,8 @@ module.exports = function(crowi, app) {
  app.post('/_api/admin/import/qiita'           , loginRequiredStrictly , adminRequired , csrfProtection, addActivity, admin.api.importDataFromQiita);
  app.post('/_api/admin/import/testQiitaAPI'    , loginRequiredStrictly , adminRequired , csrfProtection, addActivity, admin.api.testQiitaAPI);

+  app.get('/attachment/brand-logo/:id([0-9a-z]{24})' , certifyBrandLogo, loginRequired, attachment.api.get);


メンテナスモード時に考慮して API を新設しました

別 PR の FB に対応したものになります
#7204 (comment)

/attachment/brand-logo 固定でいいね

固定にしました

miya · 2023-01-04T09:53:09Z

packages/app/src/server/middlewares/certify-brand-logo.ts

+    next();
+  };
+
+};


Brand logo か判断するための express middleware

loginRequired を通過させるための変数 isBrandLogo を追加しました

id ではなく type で取得する
(BRAND_LOGO type のドキュメントは1つしか存在しないはず)

修正しました

route を分けたからここでの存在確認いらないね。
この middleware は無条件で req.isBrandLogo = true をセットしていい。

attachment が存在しなかったら attachment.api.getBrandLogo が 404 を返す

修正しました

packages/app/src/server/middlewares/login-required.js

miya · 2023-01-04T09:54:28Z

packages/app/src/server/routes/apiv3/customize-setting.js

@@ -718,7 +718,7 @@ module.exports = (crowi) => {
      try {
        attachment = await attachmentService.createAttachment(file, req.user, null, AttachmentType.BRAND_LOGO);
        const attachmentConfigParams = {
-          'customize:customizedLogoSrc': attachment.filePathProxied,
+          'customize:customizedLogoSrc': attachment.brandLogoFilePathProxied,


/attachment/brand-logo/{filePath} で保存されるように修正しました

/attachment/brand-logo 固定でいいね

固定にしたので、customize:customizedLogoSrc は廃止にしました

yuki-takei · 2023-01-04T14:16:54Z

packages/app/src/server/middlewares/certify-brand-logo.ts

+    next();
+  };
+
+};


id ではなく type で取得する
(BRAND_LOGO type のドキュメントは1つしか存在しないはず)

yuki-takei · 2023-01-04T14:17:25Z

packages/app/src/server/models/attachment.js

@@ -51,6 +51,10 @@ module.exports = function(crowi) {
    return `/attachment/${this._id}`;
  });

+  attachmentSchema.virtual('brandLogoFilePathProxied').get(function() {
+    return `/attachment/brand-logo/${this._id}`;
+  });


yuki-takei · 2023-01-04T14:17:58Z

packages/app/src/server/routes/apiv3/customize-setting.js

@@ -718,7 +718,7 @@ module.exports = (crowi) => {
      try {
        attachment = await attachmentService.createAttachment(file, req.user, null, AttachmentType.BRAND_LOGO);
        const attachmentConfigParams = {
-          'customize:customizedLogoSrc': attachment.filePathProxied,
+          'customize:customizedLogoSrc': attachment.brandLogoFilePathProxied,


/attachment/brand-logo 固定でいいね

yuki-takei · 2023-01-04T14:18:08Z

packages/app/src/server/routes/index.js

@@ -106,6 +107,8 @@ module.exports = function(crowi, app) {
  app.post('/_api/admin/import/qiita'           , loginRequiredStrictly , adminRequired , csrfProtection, addActivity, admin.api.importDataFromQiita);
  app.post('/_api/admin/import/testQiitaAPI'    , loginRequiredStrictly , adminRequired , csrfProtection, addActivity, admin.api.testQiitaAPI);

+  app.get('/attachment/brand-logo/:id([0-9a-z]{24})' , certifyBrandLogo, loginRequired, attachment.api.get);


/attachment/brand-logo 固定でいいね

miya · 2023-01-04T18:02:48Z

packages/app/src/server/middlewares/certify-brand-logo.ts

+    next();
+  };
+
+};


修正しました

packages/app/src/stores/context.tsx

packages/app/src/components/Navbar/GrowiNavbar.tsx

packages/app/src/components/Admin/Customize/CustomizeLogoSetting.tsx

yuki-takei · 2023-01-05T11:49:18Z

packages/app/src/server/service/attachment.js

@@ -77,6 +79,11 @@ class AttachmentService {
    return;
  }

+  async isBrandLogoExist() {
+    const Attachment = this.crowi.model('Attachment');
+    return await Attachment.findOne({ attachmentType: AttachmentType.BRAND_LOGO }) != null;


count が 1 以上かどうかというクエリにしてください

修正しました

yuki-takei · 2023-01-05T11:53:09Z

packages/app/src/pages/_app.page.tsx

@@ -63,7 +63,8 @@ function GrowiApp({ Component, pageProps }: GrowiAppProps): JSX.Element {
  useSiteUrl(commonPageProps.siteUrl);
  useConfidential(commonPageProps.confidential);
  useGrowiVersion(commonPageProps.growiVersion);
-  useCustomizedLogoSrc(commonPageProps.customizedLogoSrc);
+  useIsDefaultLogo(commonPageProps.isDefaultLogo);
+  useIsCustomizedLogoUploaded(commonPageProps.isCustomizedLogoUploaded);


ここは元のままでいい

「デフォルトロゴが選択されているかどうか」と「カスタムロゴがアップロードされているかどうか」は別問題なのでこのままにしました。useCustomizedLogoSrc (カスタムロゴのソース) と useIsCustomizedLogoUploaded (カスタムロゴがアップロードされているかどうかの boolean ) の使われ方はほぼ同じで、フロントエンドで src を使う必要がないので後者を残しました。

yuki-takei · 2023-01-05T11:53:19Z

packages/app/src/components/Navbar/GrowiNavbar.tsx


  return (
    <nav id="grw-navbar" className={`navbar grw-navbar ${styles['grw-navbar']} navbar-expand navbar-dark sticky-top mb-0 px-0`}>
      {/* Brand Logo  */}
      <div className="navbar-brand mr-0">
        <Link href="/" prefetch={false}>
          <a className="grw-logo d-block">
-            <GrowiNavbarLogo logoSrc={customizedLogoSrc}/>
+            <GrowiNavbarLogo isDefaultLogo={isDefaultLogo} isCustomizedLogoUploaded={isCustomizedLogoUploaded}/>


ここは元のままでいい

isDefaultLogo だけで判断できるようにしました

yuki-takei · 2023-01-05T11:55:30Z

packages/app/src/pages/utils/commons.ts

    currentUser,
+    isDefaultLogo,
+    isCustomizedLogoUploaded,


customizedLogoSrc props は元に戻し、DB から取得した src を入れる

packages/app/src/pages/utils/commons.ts

yuki-takei · 2023-01-06T09:57:17Z

packages/app/src/server/middlewares/certify-brand-logo.ts

+    next();
+  };
+
+};


route を分けたからここでの存在確認いらないね。
この middleware は無条件で req.isBrandLogo = true をセットしていい。

attachment が存在しなかったら attachment.api.getBrandLogo が 404 を返す

reg-suit · 2023-01-06T11:17:50Z

reg-suit detected visual differences.

Check this report, and review them.

🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴

🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵

What do the circles mean?

The number of circles represent the number of changed images.
🔴 : Changed items, ⚪ : New items, ⚫ : Deleted items, and 🔵 Passed items

How can I change the check status?

If reviewers approve this PR, the reg context status will be green automatically.

Allow attachmentType to allow brand logo attachments on shared pages

a4c609c

miya requested a review from yuki-takei January 4, 2023 05:57

miya self-assigned this Jan 4, 2023

github-actions bot added the type/bug label Jan 4, 2023

miya commented Jan 4, 2023

View reviewed changes

miya temporarily deployed to VRT January 4, 2023 06:16 — with GitHub Actions Inactive

yuki-takei requested changes Jan 4, 2023

View reviewed changes

miya added 3 commits January 4, 2023 09:13

Create an api that returns the brand logo

8933306

Create express middleware to determine if it is a brand logo

c77f8f2

Change path name to save

9df34a0

miya temporarily deployed to VRT January 4, 2023 10:06 — with GitHub Actions Inactive

cleanup

6263af9

miya commented Jan 4, 2023

View reviewed changes

miya requested a review from yuki-takei January 4, 2023 10:23

miya temporarily deployed to VRT January 4, 2023 10:36 — with GitHub Actions Inactive

yuki-takei requested changes Jan 4, 2023

View reviewed changes

miya added 6 commits January 4, 2023 14:54

fix middleware

45d78a6

fix api

d729c0a

create isBrandLogoExist method

478d8e5

Move logic to AttachmentService

75c904b

add await

75c6e9a

imprv

47619d6

miya temporarily deployed to VRT January 4, 2023 17:38 — with GitHub Actions Inactive

miya added 2 commits January 4, 2023 18:01

fix state name

700dfc4

remove unnecessary diff

565da9a

miya commented Jan 4, 2023

View reviewed changes

miya temporarily deployed to VRT January 4, 2023 18:29 — with GitHub Actions Inactive

yuki-takei requested changes Jan 5, 2023

View reviewed changes

change logic

88a9b57

miya temporarily deployed to VRT January 6, 2023 08:06 — with GitHub Actions Inactive

Whether isDefaultLogo is determined by getServerSideCommonProps

ea806eb

miya commented Jan 6, 2023

View reviewed changes

packages/app/src/pages/utils/commons.ts Show resolved Hide resolved

miya temporarily deployed to VRT January 6, 2023 09:14 — with GitHub Actions Inactive

miya requested a review from yuki-takei January 6, 2023 09:14

yuki-takei requested changes Jan 6, 2023

View reviewed changes

miya added 4 commits January 6, 2023 10:28

fix api

59ba5c0

uncommon

6d2e512

Show only brand logo

4604093

remove console.log

a68c09a

miya requested a review from yuki-takei January 6, 2023 10:49

remove unnecessary code

01ec770

miya temporarily deployed to VRT January 6, 2023 11:13 — with GitHub Actions Inactive

yuki-takei approved these changes Jan 6, 2023

View reviewed changes

yuki-takei merged commit 8ffc924 into master Jan 6, 2023

yuki-takei deleted the fix/112474-custom-logo-not-displayed-on-shared-page branch January 6, 2023 11:19

github-actions bot mentioned this pull request Jan 6, 2023

Release v6.0.1 #7186

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Custom logo not displayed on shared page #7205

fix: Custom logo not displayed on shared page #7205

miya commented Jan 4, 2023

miya Jan 4, 2023

yuki-takei Jan 4, 2023

yuki-takei Jan 4, 2023

miya Jan 4, 2023 •

edited

yuki-takei Jan 4, 2023

miya Jan 4, 2023

miya Jan 4, 2023

yuki-takei Jan 4, 2023

miya Jan 4, 2023

yuki-takei Jan 6, 2023

miya Jan 6, 2023

miya Jan 4, 2023

yuki-takei Jan 4, 2023

miya Jan 4, 2023

yuki-takei Jan 4, 2023

yuki-takei Jan 4, 2023

yuki-takei Jan 4, 2023

yuki-takei Jan 4, 2023

miya Jan 4, 2023

yuki-takei Jan 5, 2023

miya Jan 6, 2023

yuki-takei Jan 5, 2023

miya Jan 6, 2023 •

edited

yuki-takei Jan 5, 2023

miya Jan 6, 2023

yuki-takei Jan 5, 2023

yuki-takei Jan 6, 2023

reg-suit bot commented Jan 6, 2023

+                  next();
+                };
+              };

fix: Custom logo not displayed on shared page #7205

fix: Custom logo not displayed on shared page #7205

Conversation

miya commented Jan 4, 2023

Task

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miya Jan 4, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miya Jan 6, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reg-suit bot commented Jan 6, 2023

miya Jan 4, 2023 •

edited

miya Jan 6, 2023 •

edited