feat(content-sidebar): add archived date to content preview sidebar by michalkowalczyk-box · Pull Request #3625 · box/box-ui-elements

michalkowalczyk-box · 2024-08-29T18:24:47Z

Adding archive date to details sidebar. When feature is enabled it's fetched along rest of the file information from global metadata template archivedItemTemplate

greg-in-a-box · 2024-08-31T01:37:18Z


        const { accessStats, accessStatsError, file, fileError, isLoadingAccessStats }: State = this.state;

+        const shouldShowArchivedAt = isFeatureEnabled(features, 'details.archivedAt.enabled');


I feel like this should be moved all the way to down the bottom where the archivedAt is actually used, if you put a tenary at this level to decide if it should have the archivedAt or not because you wont know if the archivedAt is actually present but the split is off or its undefined when its retrieved from the server or if the split is on and the archivedAt is undefined

Seems better. Just to make sure - you mean move features all the way down to ItemProperties.js or should I just move the shouldShowArchivedAt as a prop?

greg-in-a-box · 2024-09-04T18:24:08Z

+            archivedAt: null,
+            features: {
+                details: {
+                    archivedAt: {
+                        enabled: false,
+                    },
+                },
+            },
+        });
+
+        expect(wrapper).toMatchSnapshot();
+    });
+
+    test('should not render archived date when feature is not set', () => {
+        const wrapper = getWrapper({
+            archivedAt: null,


I think we should actually supply a archivedAt here so we are certain that the tests are passing because the feature is off or undefined. from reading the test itself, we dont know if its not rendering because archivedAt is null or feature is null/undefined.

greg-in-a-box · 2024-09-04T18:29:17Z


 ItemProperties.propTypes = {
+    /** the datetime this item was archived, accepts any value that can be passed to the Date() constructor */
+    archivedAt: PropTypes.oneOfType([PropTypes.number, PropTypes.string]),


can it actually be a number?

Changed to string

Changed again since we're probably are going to be using unix epoch timestamps

greg-in-a-box

lgtm

jstoffan · 2024-09-09T15:18:15Z

                        {hasVersions && <SidebarVersions file={file} onVersionHistoryClick={onVersionHistoryClick} />}
                        <SidebarFileProperties
+                            archivedAt={archivedAt}
                            file={file}


It seems more appropriate for archived_at to be a field on the File model.

jstoffan · 2024-09-09T15:19:06Z


-export default ItemProperties;
+export { ItemProperties as ItemPropertiesComponent };
+export default withFeatureConsumer(ItemProperties);


Do we have a hook? If so, let's use it; if not, let's create one.

jstoffan · 2024-09-09T15:19:41Z

    url,
 }) => {
    const descriptionId = uniqueid('description_');
+    const shouldShowArchivedAt = isFeatureEnabled(features, 'details.archivedAt.enabled');


Can we update the endpoint to not return the value if the feature isn't enabled yet, instead?

michalkowalczyk-box · 2024-09-20T12:35:27Z

        <ItemProperties
+            archivedAt={Number(getProp(file, FIELD_METADATA_ARCHIVE)?.archiveDate) * 1000}


Right now we plan to store unix epoch timestamps as strings in our template, but we're discussing changing their type to number.

Just in case we don't change it - is this conversion okay if we decide to store them as strings?

Also I will need to make sure whether they will be stored in seconds or milliseconds

Our linter may complain about using a static built-in method like this. If so, use parseInt(value, 10) instead.

That said, we should handle formatting/display within ItemProperties rather than here, as well as handling non-number values better.

Seems good with linter, I will wait with fixes for after we finalize type and format in which we store archiveDate in our metadata template.

But also - do we need something better than js Date() for handling this?

If so, I can try implementing something here, if you tell me which cases you need handled.

But if not, shouldn't passing correct date format be responsibility of parent component using ItemProperties?

I slightly refactored conversion here, now it passes undefined to prop when we don't have it metadata. My team finalized discussions and we'll have unix epoch timestamp in seconds stored as a string, so this should be correct conversion

michalkowalczyk-box · 2024-09-20T13:40:30Z

-const SIDEBAR_FIELDS_TO_FETCH = [
+const SIDEBAR_FIELDS_TO_FETCH: Array<string> = [


I had to add type because flow was showing error

I could also add same type to other field arrays in this file to be consistent or move concat to functions in which we fetch

Hm. I'm not seeing this error locally. Flow is pretty terrible, but it should be able to infer one string array from another. Are you using the local Flow binary installed with the project? If so, maybe try removing the type annotations to see if it was a temporary issue?

Removed type annotation from SIDEBAR_FIELDS_TO_FETCH, but left one on SIDEBAR_FIELDS_TO_FETCH_ARCHIVE, without this one the flow error was still showing

jstoffan

Approach LGTM. Left some feedback on a few other touch-ups that are needed.

jstoffan · 2024-09-20T14:46:31Z

 import type { BoxItem } from '../../common/types/core';
+import type { FeatureConfig } from '../common/feature-checking';
 import './DetailsSidebar.scss';
+import { isFeatureEnabled, withFeatureConsumer } from '../common/feature-checking';


Let's move this line above the SCSS and type imports.

jstoffan · 2024-09-20T14:50:05Z

      className="loading-indicator-wrapper "
    >
      <ItemProperties
+        archivedAt={NaN}


We should handle cases where the value isn't a number by hiding the line item.

Fixed after conversion change

jstoffan · 2024-09-20T14:50:35Z

 jest.mock('../SidebarClassification', () => 'SidebarClassification');
 jest.mock('../SidebarContentInsights', () => 'SidebarContentInsights');
+jest.mock('../../common/feature-checking', () => ({
+    ...jest.requireActual('../../common/feature-checking'),


Do we need the rest of the module or can we just mock the whole thing?

jest.mock('../../common/feature-checking');

It was not needed, fixed here and in ContentSidebar.test.js

jstoffan · 2024-09-20T14:54:11Z

        <ItemProperties
+            archivedAt={Number(getProp(file, FIELD_METADATA_ARCHIVE)?.archiveDate) * 1000}


Our linter may complain about using a static built-in method like this. If so, use parseInt(value, 10) instead.

That said, we should handle formatting/display within ItemProperties rather than here, as well as handling non-number values better.

jstoffan

Thanks for working to refine the solution!

jstoffan · 2024-09-20T19:09:48Z

-const SIDEBAR_FIELDS_TO_FETCH = [
+const SIDEBAR_FIELDS_TO_FETCH: Array<string> = [


Hm. I'm not seeing this error locally. Flow is pretty terrible, but it should be able to infer one string array from another. Are you using the local Flow binary installed with the project? If so, maybe try removing the type annotations to see if it was a temporary issue?

This reverts commit 11d68f5.

jstoffan · 2024-09-23T14:58:54Z

 exports[`elements/content-sidebar/SidebarFileProperties render() should render ItemProperties for anonymous uploaders 1`] = `
 <LoadingIndicatorWrapper>
  <ItemProperties
+    archivedAt={1726832355000}


Ideally, all our endpoints should use the same format for dates. We can address that as a separate change, though, if needed.

michalkowalczyk-box requested review from a team as code owners August 29, 2024 18:24

michalkowalczyk-box changed the title ~~Add archived date for content sidebar~~ feat(content-sidebar): add archived date to content preview sidebar Aug 29, 2024

greg-in-a-box reviewed Aug 31, 2024

View reviewed changes

michalkowalczyk-box force-pushed the add-archived-date-for-content-sidebar branch from e172d3b to 8079044 Compare September 4, 2024 14:11

greg-in-a-box requested changes Sep 4, 2024

View reviewed changes

michalkowalczyk-box requested a review from greg-in-a-box September 4, 2024 20:18

greg-in-a-box previously approved these changes Sep 5, 2024

View reviewed changes

jstoffan suggested changes Sep 9, 2024

View reviewed changes

michalkowalczyk-box dismissed greg-in-a-box’s stale review via 2457413 September 20, 2024 12:22

michalkowalczyk-box force-pushed the add-archived-date-for-content-sidebar branch from b271c87 to 2457413 Compare September 20, 2024 12:22

michalkowalczyk-box requested a review from a team as a code owner September 20, 2024 12:22

michalkowalczyk-box commented Sep 20, 2024

View reviewed changes

michalkowalczyk-box requested review from greg-in-a-box and jstoffan September 20, 2024 13:59

jstoffan reviewed Sep 20, 2024

View reviewed changes

jstoffan previously approved these changes Sep 20, 2024

View reviewed changes

michalkowalczyk-box dismissed jstoffan’s stale review via 19243d0 September 23, 2024 10:16

michalkowalczyk-box added 11 commits September 23, 2024 13:00

feat(content-sidebar): add archived date to content preview sidebar

3bbfd65

feat(content-sidebar): add feature flag to details sidebar

321526b

feat(content-sidebar): fix archived field

bf8c71c

feat(content-sidebar): add tests and update snapshots for DetailsSidebar

b3f576a

feat(content-sidebar): add tests and update snapshots for ItemProperties

ad3b49e

feat(content-sidebar): update test and snapshots SidebarFileProperties

aa0748d

feat(content-sidebar): move feature to ItemProperties

58d5943

feat(content-sidebar): revert tests for DetailsSidebar

ff7743b

This reverts commit 11d68f5.

feat(content-sidebar): update snapshots and tests

1c06a46

feat(content-sidebar): update test and prop types

b0f321c

feat(content-sidebar): update sidebar to fetch archive date

a27a9b4

michalkowalczyk-box added 10 commits September 23, 2024 13:00

feat(content-sidebar): revert tests to master

56c0854

feat(content-sidebar): update tests and snapshots

0d2e31e

feat(content-sidebar): remove unused prop

461bee8

feat(content-sidebar): remove unused field

f0fdb14

feat(content-sidebar): refactor fields to fetch

b017645

feat(content-sidebar): added comment to fetched fields

d254f62

feat(content-sidebar): import reorder and mock fix

eb27d22

feat(content-sidebar): remove type annotations

d116bc2

feat(content-sidebar): add type annotation

0a38648

feat(content-sidebar): changed archiveDate conversion

0aac903

michalkowalczyk-box force-pushed the add-archived-date-for-content-sidebar branch from 9505cdb to 0aac903 Compare September 23, 2024 11:02

michalkowalczyk-box requested a review from jstoffan September 23, 2024 11:48

jstoffan approved these changes Sep 23, 2024

View reviewed changes

greg-in-a-box approved these changes Sep 25, 2024

View reviewed changes

Merge branch 'master' into add-archived-date-for-content-sidebar

238cf88

michalkowalczyk-box added the ready-to-merge label Sep 26, 2024

mergify Bot merged commit 10e68f3 into box:master Sep 26, 2024

michalkowalczyk-box mentioned this pull request Oct 2, 2024

feat(content-sidebar): disable version modifications for archive file #3637

Merged


		const { accessStats, accessStatsError, file, fileError, isLoadingAccessStats }: State = this.state;

		const shouldShowArchivedAt = isFeatureEnabled(features, 'details.archivedAt.enabled');

		<ItemProperties
		archivedAt={Number(getProp(file, FIELD_METADATA_ARCHIVE)?.archiveDate) * 1000}

		const SIDEBAR_FIELDS_TO_FETCH = [
		const SIDEBAR_FIELDS_TO_FETCH: Array<string> = [

Conversation

michalkowalczyk-box commented Aug 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michalkowalczyk-box Sep 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

michalkowalczyk-box Sep 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg-in-a-box 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!

michalkowalczyk-box Sep 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

jstoffan 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!

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!

michalkowalczyk-box Sep 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jstoffan 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!

Reviewers

michalkowalczyk-box commented Aug 29, 2024 •

edited

Loading

michalkowalczyk-box Sep 2, 2024 •

edited

Loading

michalkowalczyk-box Sep 20, 2024 •

edited

Loading

michalkowalczyk-box Sep 20, 2024 •

edited

Loading

michalkowalczyk-box Sep 20, 2024 •

edited

Loading