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

Add updated_at column to objects' tables #1218

Merged

Conversation

RoyiSitbon
Copy link
Contributor

@RoyiSitbon RoyiSitbon commented Feb 7, 2022

Signed-off-by: sitbubu royi.sitbon@logz.io

Is your feature request related to a problem? Please describe.
We want to see more informative data inside the saved objects' tables,
one important column that answers our needs will be the “updated at” option.
After adding this option we will gain an additional advantage, i.e sort our objects by date.

Describe the solution you'd like to
We wish to add an additional “updated at” column to our management, visualizations, and dashboards tables. e.g. for the management table, we will define:
Screen Shot 2022-02-06 at 20 04 38

and then we will be able to see an additional column, like so:
Untitled

The same solution will be implemented under the visualizations and dashboards tables, besides the fact that we will need to set our updated_at property inside the attributes prop since it uses us as the rendered object info for these tables.

Linked issue:
#1217

@RoyiSitbon RoyiSitbon requested a review from a team as a code owner February 7, 2022 08:36
Copy link
Contributor

@tmarkley tmarkley left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! I believe this field uses the ISO 8601 string format YYYY-MM-DDTHH:mm:ss.sssZ, so we should keep that consistent. And I know we can't change the field name, but when we render it I think it would look better to use a noun instead of including the at proposition (i.e. Updated Time instead of Updated at).

@kgcreative
Copy link
Member

There is an advanced setting in stack management that determines the date format of how dates should be pretty printed. We should be using that to format the date/time display.

@kavilla kavilla linked an issue Feb 8, 2022 that may be closed by this pull request
@RoyiSitbon
Copy link
Contributor Author

Thanks for the changes! I believe this field uses the ISO 8601 string format YYYY-MM-DDTHH:mm:ss.sssZ, so we should keep that consistent. And I know we can't change the field name, but when we render it I think it would look better to use a noun instead of including the at proposition (i.e. Updated Time instead of Updated at).

thanks, changed as requested

@RoyiSitbon
Copy link
Contributor Author

Thanks for the changes! I believe this field uses the ISO 8601 string format YYYY-MM-DDTHH:mm:ss.sssZ, so we should keep that consistent. And I know we can't change the field name, but when we render it I think it would look better to use a noun instead of including the at proposition (i.e. Updated Time instead of Updated at).

Hey, all changes have been made inside this pr.
Thanks

@RoyiSitbon RoyiSitbon force-pushed the AddUpdatedAtColumnToObjectsTables branch from 7e81f6c to 476d1da Compare March 2, 2022 06:46
@tmarkley tmarkley self-assigned this Mar 15, 2022
@RoyiSitbon RoyiSitbon requested a review from tmarkley April 4, 2022 08:57
Copy link
Member

@kavilla kavilla left a comment

Choose a reason for hiding this comment

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

Blocking to ensure stability for 2.0.0-rc1

@tmarkley tmarkley dismissed kavilla’s stale review May 9, 2022 16:12

No need to block for RC1 any longer.

@tmarkley
Copy link
Contributor

tmarkley commented May 9, 2022

Hi @RoyiSitbon, do you need any additional questions answered for this PR?

@RoyiSitbon
Copy link
Contributor Author

Hi @RoyiSitbon, do you need any additional questions answered for this PR?

Hey @tmarkley, thanks for checking. I replied above.

@RoyiSitbon RoyiSitbon force-pushed the AddUpdatedAtColumnToObjectsTables branch from 476d1da to 61116c6 Compare May 13, 2022 11:32
@RoyiSitbon RoyiSitbon requested a review from kavilla May 13, 2022 11:33
@RoyiSitbon RoyiSitbon force-pushed the AddUpdatedAtColumnToObjectsTables branch 2 times, most recently from 88e68c4 to 05da790 Compare May 13, 2022 14:52
@codecov-commenter
Copy link

codecov-commenter commented May 13, 2022

Codecov Report

Merging #1218 (7580cec) into main (b5d529a) will increase coverage by 0.02%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##             main    #1218      +/-   ##
==========================================
+ Coverage   66.55%   66.57%   +0.02%     
==========================================
  Files        3170     3170              
  Lines       60321    60326       +5     
  Branches     9181     9182       +1     
==========================================
+ Hits        40144    40162      +18     
+ Misses      17984    17969      -15     
- Partials     2193     2195       +2     
Impacted Files Coverage Δ
...agement_section/objects_table/components/table.tsx 65.51% <0.00%> (-1.15%) ⬇️
...ment_section/objects_table/saved_objects_table.tsx 73.48% <ø> (ø)
...rd/public/application/listing/dashboard_listing.js 78.57% <50.00%> (-4.77%) ⬇️
...c/core/public/saved_objects/simple_saved_object.ts 56.25% <100.00%> (+2.91%) ⬆️
...objects/public/saved_object/saved_object_loader.ts 53.84% <100.00%> (+53.84%) ⬆️
...ared/static/forms/hook_form_lib/hooks/use_field.ts 66.66% <0.00%> (+0.96%) ⬆️

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

Copy link
Contributor

@tmarkley tmarkley left a comment

Choose a reason for hiding this comment

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

Thanks for these improvements @RoyiSitbon! The code is clean and maintainable, that's much appreciated.

I think the last thing that would put the cherry on top is adding some additional unit tests as well as functional tests for this feature.

@RoyiSitbon RoyiSitbon requested a review from tmarkley June 9, 2022 13:54
@RoyiSitbon RoyiSitbon force-pushed the AddUpdatedAtColumnToObjectsTables branch from 5d60051 to 46f7e13 Compare June 20, 2022 06:33
@RoyiSitbon RoyiSitbon requested a review from tmarkley June 20, 2022 06:34
@kavilla kavilla assigned tmarkley and unassigned tmarkley Jun 20, 2022
@joshuarrrr joshuarrrr force-pushed the AddUpdatedAtColumnToObjectsTables branch from 46f7e13 to d5ba653 Compare June 22, 2022 19:06
tmarkley
tmarkley previously approved these changes Jun 27, 2022
Copy link
Contributor

@tmarkley tmarkley left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for the improvements!

@kavilla kavilla added v2.3.0 and removed v2.2.0 labels Aug 9, 2022
@kavilla
Copy link
Member

kavilla commented Aug 9, 2022

Hello @RoyiSitbon,
Will pull this down! Quick question will this require a migration on existing documents?

Hey @kavilla , yes pls. not requires a migration, unless we will want to ascertain the existence of these metaFields. Nothing will go wrong even without migration attach(also validated using tests as expected).

Thanks a lot

Great then I think we can target to get this in to 2.3! Thank you so much @RoyiSitbon

@kavilla kavilla added v2.4.0 'Issues and PRs related to version v2.4.0' and removed v2.3.0 labels Sep 12, 2022
ashwin-pc
ashwin-pc previously approved these changes Sep 17, 2022
Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

I've validated this locally and the change looks good! Glad that you exposed the updated_at a global level so that every saved object type can utilize it. Sorry that this got sidetracked.

@RoyiSitbon
Copy link
Contributor Author

I've validated this locally and the change looks good! Glad that you exposed the updated_at a global level so that every saved object type can utilize it. Sorry that this got sidetracked.

🙏

Signed-off-by: sitbubu <royi.sitbon@logz.io>
Signed-off-by: sitbubu <royi.sitbon@logz.io>
Signed-off-by: sitbubu <royi.sitbon@logz.io>
Signed-off-by: sitbubu <royi.sitbon@logz.io>
Signed-off-by: sitbubu <royi.sitbon@logz.io>
Signed-off-by: sitbubu <royi.sitbon@logz.io>
Signed-off-by: sitbubu <royi.sitbon@logz.io>
Signed-off-by: sitbubu <royi.sitbon@logz.io>
Signed-off-by: sitbubu <royi.sitbon@logz.io>
@RoyiSitbon RoyiSitbon dismissed stale reviews from ashwin-pc and tmarkley via 87e49ed September 18, 2022 04:43
@RoyiSitbon RoyiSitbon force-pushed the AddUpdatedAtColumnToObjectsTables branch from 7580cec to 87e49ed Compare September 18, 2022 04:43
Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

Looks great, I'm excited for this improvement!

Comment on lines +66 to +67
// eslint-disable-next-line @typescript-eslint/naming-convention
updated_at,
Copy link
Member

Choose a reason for hiding this comment

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

Why does it need to be updated_at instead of updatedAt?

Copy link
Member

Choose a reason for hiding this comment

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

Thats the OpenSearch response. Don't think we can change that.

@ashwin-pc ashwin-pc merged commit 8874afd into opensearch-project:main Sep 20, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 20, 2022
* Add updated_at columnb to objects' tables

Signed-off-by: sitbubu <royi.sitbon@logz.io>

* Grammer and iso usage

Signed-off-by: sitbubu <royi.sitbon@logz.io>

* Set updated at field from advanced settings

Signed-off-by: sitbubu <royi.sitbon@logz.io>

* Use dateFormat instead of  dateFormat:scaled

Signed-off-by: sitbubu <royi.sitbon@logz.io>

* snapshot

Signed-off-by: sitbubu <royi.sitbon@logz.io>

* Add updated_at to additional comment

Signed-off-by: sitbubu <royi.sitbon@logz.io>

* Add unit-tests for updated_at as null undefined or unknown

Signed-off-by: sitbubu <royi.sitbon@logz.io>

* Simplify test file

Signed-off-by: sitbubu <royi.sitbon@logz.io>

* Simplified header and import from src

Signed-off-by: sitbubu <royi.sitbon@logz.io>

Signed-off-by: sitbubu <royi.sitbon@logz.io>
(cherry picked from commit 8874afd)
joshuarrrr pushed a commit that referenced this pull request Sep 22, 2022
* Add updated_at columnb to objects' tables

Signed-off-by: sitbubu <royi.sitbon@logz.io>

* Grammer and iso usage

Signed-off-by: sitbubu <royi.sitbon@logz.io>

* Set updated at field from advanced settings

Signed-off-by: sitbubu <royi.sitbon@logz.io>

* Use dateFormat instead of  dateFormat:scaled

Signed-off-by: sitbubu <royi.sitbon@logz.io>

* snapshot

Signed-off-by: sitbubu <royi.sitbon@logz.io>

* Add updated_at to additional comment

Signed-off-by: sitbubu <royi.sitbon@logz.io>

* Add unit-tests for updated_at as null undefined or unknown

Signed-off-by: sitbubu <royi.sitbon@logz.io>

* Simplify test file

Signed-off-by: sitbubu <royi.sitbon@logz.io>

* Simplified header and import from src

Signed-off-by: sitbubu <royi.sitbon@logz.io>

Signed-off-by: sitbubu <royi.sitbon@logz.io>
(cherry picked from commit 8874afd)

Co-authored-by: Royi Sitbon <royi.sitbon@logz.io>
@seanneumann
Copy link
Contributor

Nice!

@AMoo-Miki AMoo-Miki added the enhancement New feature or request label Nov 5, 2022
sipopo pushed a commit to sipopo/OpenSearch-Dashboards that referenced this pull request Dec 16, 2022
* Add updated_at columnb to objects' tables

Signed-off-by: sitbubu <royi.sitbon@logz.io>

* Grammer and iso usage

Signed-off-by: sitbubu <royi.sitbon@logz.io>

* Set updated at field from advanced settings

Signed-off-by: sitbubu <royi.sitbon@logz.io>

* Use dateFormat instead of  dateFormat:scaled

Signed-off-by: sitbubu <royi.sitbon@logz.io>

* snapshot

Signed-off-by: sitbubu <royi.sitbon@logz.io>

* Add updated_at to additional comment

Signed-off-by: sitbubu <royi.sitbon@logz.io>

* Add unit-tests for updated_at as null undefined or unknown

Signed-off-by: sitbubu <royi.sitbon@logz.io>

* Simplify test file

Signed-off-by: sitbubu <royi.sitbon@logz.io>

* Simplified header and import from src

Signed-off-by: sitbubu <royi.sitbon@logz.io>

Signed-off-by: sitbubu <royi.sitbon@logz.io>
Signed-off-by: Sergey V. Osipov <sipopo@yandex.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x enhancement New feature or request v2.4.0 'Issues and PRs related to version v2.4.0'
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Updated at column in saved objects' tables
10 participants