[App Search] Detail Page for Automated Curations by byronhulcher · Pull Request #113550 · elastic/kibana

byronhulcher · 2021-09-30T15:26:54Z

Summary

The Curation view has now split into the ManualCuration and AutomatedCuration views. ManualCuration is unchanged from the old Curation while AutomatedCuration is new, with the differences being:

Automated Badge
A "convert to manual" button
Buttons and other actions to modify the curation are hidden
Updated copy for when user cannot modify the curation

Screenshots

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

…ggestions route

…uration is automated

…detail

byronhulcher · 2021-10-04T16:35:47Z

retest

JasonStoltz · 2021-10-04T17:57:42Z

x-pack/plugins/translations/translations/ja-JP.json

    "xpack.enterpriseSearch.appSearch.engine.curations.manageQueryButtonLabel": "クエリを管理",
    "xpack.enterpriseSearch.appSearch.engine.curations.manageQueryDescription": "このキュレーションのクエリを編集、追加、削除します。",
    "xpack.enterpriseSearch.appSearch.engine.curations.manageQueryTitle": "クエリを管理",
-    "xpack.enterpriseSearch.appSearch.engine.curations.organicDocuments.emptyDescription": "表示するオーガニック結果はありません。上記のアクティブなクエリを追加または変更します。",


I'm pretty sure you don't have to prune these old labels, and that they will be pruned automatically by the i18n process. This is something @constancecchen mentioned before (if I recall correctly), though I'm not sure I'd be able to find the PR where she talks about that or any documentation to back that up.

I got CI failures from these unpruned ones, I'll reach out to you next time I see that happen.

Re: i18n failures - if you run node scripts/i18n_check --fix it'll handle removing stuff for you automatically without having to open the file!

More reading: https://github.com/elastic/kibana-team/blob/master/localization.md (their diagram is neat and worth expanding, + the info around how and when they manage scheduling for i18n)

Re: i18n failures - if you run node scripts/i18n_check --fix it'll handle removing stuff for you automatically without having to open the file!

👍 I used that to remove these, just highlighting that they're not pruned automatically.

JasonStoltz

This code all looks great. I just left a couple of comments. Full disclosure, it takes forever for me to pull down a new branch and rebuild to test, so I did not test this locally.

JasonStoltz · 2021-10-04T18:06:15Z

...e_search/public/applications/app_search/components/curations/curation/automated_curation.tsx

+import { PromotedDocuments, OrganicDocuments } from './documents';
+
+export const AutomatedCuration: React.FC = () => {
+  const { curationId } = useParams() as { curationId: string };


I suspect you can do something like this instead to avoid casting.

const { curationId } = useParams<{ curationId: string }>();

Thank you! I did this in 9930246

JasonStoltz · 2021-10-04T18:08:00Z

...e_search/public/applications/app_search/components/curations/curation/automated_curation.tsx

+  const { convertToManual } = useActions(CurationLogic({ curationId }));
+  const { activeQuery, dataLoading, queries } = useValues(CurationLogic({ curationId }));


Purely out of curiosity, do you know if there is any functional difference between what you have and the following? The later avoids instantiating the logic twice, but I don't know if it matters or not.

Suggested change

const { convertToManual } = useActions(CurationLogic({ curationId }));

const { activeQuery, dataLoading, queries } = useValues(CurationLogic({ curationId }));

const logic = CurationLogic({ curationId });

const { convertToManual } = useActions(logic);

const { activeQuery, dataLoading, queries } = useValues(logic);

No reason! I'll go with yours

I've done this in 9930246. I think its effectively the same, but I think instantiating it once is a better practice, in casse we use a lot of props in other logic files.

JasonStoltz · 2021-10-04T18:25:07Z

.../applications/app_search/components/curations/curation/documents/promoted_documents.test.tsx

    });

-    describe('draggging', () => {
+    describe('dragging', () => {


kibanamachine · 2021-10-04T21:57:16Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`enterpriseSearch`	1503	1505	+2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`enterpriseSearch`	1.3MB	1.3MB	+2.4KB

History

💚 Build #157656 succeeded b8e4af0
💔 Build #157554 failed b8e4af0
💔 Build #157148 failed 07defaa
💚 Build #157174 succeeded 0746093
💔 Build #157143 failed 8f5948e

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

kibanamachine · 2021-10-04T22:00:40Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

Co-authored-by: Byron Hulcher <byronhulcher@gmail.com>

This reverts commit 4d2a9c5.

brianseeders · 2021-10-05T01:05:33Z

Apologies @byronhulcher, this needed to be reverted. It was causing type checks to fail, see here: https://buildkite.com/elastic/kibana-hourly/builds/906#ccd9944c-5587-4b24-8502-a73bd37d4145

It likely conflicted with something else that was merged in since the last time you merged upstream

byronhulcher added auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes v7.16.0 Feature:Plugins labels Sep 30, 2021

byronhulcher marked this pull request as ready for review September 30, 2021 16:37

byronhulcher requested a review from a team September 30, 2021 16:37

byronhulcher added 19 commits September 30, 2021 14:05

Add optional suggestion field to Curation type

b829eb9

Make Curation view tests suggestion status aware

153a7d6

Added 'Automated' badge to automated curations

74bf26c

Added new PUT /internal/app_search/engines/{name}/search_relevance_su…

45f2a04

…ggestions route

Add convertToManual action to CurationLogic

4afc3e3

Added new Convert to Manual action for automated curations

263c33f

Display query in title for automated curations

9449d8d

Hide ActiveQuerySelect and ManageQueriesModal on Curation view when c…

39dae4a

…uration is automated

Implement isAutomated selector in CurationLogic

1ce3681

Disable promoted documents UX when curation is automated

8115c14

Disable AddResultButton when curation is automated

fba7976

Split Curation into AutomatedCuration and ManualCuration components

df1f8bb

Disable organic documents UX when curation is automated

6b94e3d

Clean-up after rebase

20070b4

Get rid of getShallowPageTitle

1cfd06a

Fix types after rebase

91aa2ee

Fix import

4c4dec0

Hide UX instead of disabling

fed6a08

Remove translations

1d29af1

byronhulcher force-pushed the automated-curation-detail branch from e0711cb to 1d29af1 Compare September 30, 2021 21:12

byronhulcher added 3 commits October 1, 2021 11:36

Merge remote-tracking branch 'origin/master' into automated-curation-…

8f5948e

…detail

Merge remote-tracking branch 'origin/master' into automated-curation-…

07defaa

…detail

Merge remote-tracking branch 'origin/master' into automated-curation-…

0746093

…detail

tylersmalley mentioned this pull request Oct 1, 2021

chore: add modifications to staging automatically after eslint fix #113443

Merged

byronhulcher requested a review from JasonStoltz October 1, 2021 19:00

Merge remote-tracking branch 'origin/master' into automated-curation-…

b8e4af0

…detail

JasonStoltz reviewed Oct 4, 2021

View reviewed changes

JasonStoltz approved these changes Oct 4, 2021

View reviewed changes

Clean-up AutomatedCuration

9930246

byronhulcher enabled auto-merge (squash) October 4, 2021 19:52

byronhulcher merged commit 4d2a9c5 into elastic:master Oct 4, 2021

kibanamachine mentioned this pull request Oct 4, 2021

[7.x] [App Search] Detail Page for Automated Curations (#113550) #113861

Merged

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 4, 2021

[App Search] Detail Page for Automated Curations (elastic#113550)

cf8d5b3

kibanamachine added a commit that referenced this pull request Oct 5, 2021

[App Search] Detail Page for Automated Curations (#113550) (#113861)

85ef620

Co-authored-by: Byron Hulcher <byronhulcher@gmail.com>

brianseeders added a commit that referenced this pull request Oct 5, 2021

Revert "[App Search] Detail Page for Automated Curations (#113550)"

7ad30ed

This reverts commit 4d2a9c5.

brianseeders added reverted v8.0.0 labels Oct 5, 2021

byronhulcher mentioned this pull request Oct 5, 2021

[App Search] Detail Page for Automated Curations #113885

Merged

6 tasks

		const { convertToManual } = useActions(CurationLogic({ curationId }));
		const { activeQuery, dataLoading, queries } = useValues(CurationLogic({ curationId }));

-  const { convertToManual } = useActions(CurationLogic({ curationId }));
-  const { activeQuery, dataLoading, queries } = useValues(CurationLogic({ curationId }));
+  const logic = CurationLogic({ curationId });
+  const { convertToManual } = useActions(logic);
+  const { activeQuery, dataLoading, queries } = useValues(logic);

Conversation

byronhulcher commented Sep 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Screenshots

Checklist

Uh oh!

byronhulcher commented Oct 4, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JasonStoltz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Oct 4, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

Uh oh!

kibanamachine commented Oct 4, 2021

💚 Backport successful

Uh oh!

brianseeders commented Oct 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

byronhulcher commented Sep 30, 2021 •

edited

Loading