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

New import export json action test #4225

Merged
merged 26 commits into from
Dec 27, 2021
Merged

New import export json action test #4225

merged 26 commits into from
Dec 27, 2021

Conversation

kobe1104
Copy link
Contributor

@kobe1104 kobe1104 commented Sep 17, 2021 •
edited by nikhilmandlik
Loading

Resolves:

resolves: #4558

All Submissions:

Author Checklist

  • Changes address original issue?
  • Unit tests included and/or updated with changes?
  • Command line build passes?
  • Has this been smoke tested?
  • Testing instructions included in associated issue?

@kobe1104 kobe1104 mentioned this pull request Sep 17, 2021
Merged
7 tasks
@kobe1104 kobe1104 self-assigned this Sep 17, 2021
@kobe1104 kobe1104 changed the title New import export json test New import export json action test Sep 17, 2021
@akhenry akhenry marked this pull request as ready for review December 6, 2021 21:50
@akhenry akhenry requested a review from nikhilmandlik December 6, 2021 21:51
@nikhilmandlik
Copy link
Contributor

@kobe1104 some conflicts

nikhilmandlik and others added 2 commits December 6, 2021 15:00
@nikhilmandlik @jvigliotta @charlesh88
form refactor (#3816)
91b359c 
* New form API and associated form controls
* Actions updated to use new form API.

Co-authored-by: Jamie V <[email protected]>
Co-authored-by: charlesh88 <[email protected]>
@kobe1104 @nikhilmandlik
Reimplementation import export json (#4171)
1c7f38f
Base automatically changed from topic-form-refactor to master December 7, 2021 20:27
@codecov
Copy link

codecov bot commented Dec 7, 2021 •
edited
Loading

Codecov Report

Merging #4225 (18a262d) into master (d84808a) will increase coverage by 0.17%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4225      +/-   ##
==========================================
+ Coverage   57.06%   57.23%   +0.17%     
==========================================
  Files         718      718              
  Lines       22771    22771              
  Branches     1653     1653              
==========================================
+ Hits        12994    13034      +40     
+ Misses       9360     9320      -40     
  Partials      417      417
Impacted Files Coverage Δ
...ugins/importFromJSONAction/ImportFromJSONAction.js 28.00% <ø> (ø)
src/plugins/plot/plugin.js 100.00% <ø> (ø)
...c/plugins/exportAsJSONAction/ExportAsJSONAction.js 92.30% <100.00%> (+67.69%) ⬆️
src/tools/url.js 69.23% <0.00%> (-30.77%) ⬇️
src/adapter/services/LegacyObjectAPIInterceptor.js 71.64% <0.00%> (-4.48%) ⬇️
...c/plugins/persistence/couch/CouchObjectProvider.js 81.85% <0.00%> (-0.45%) ⬇️
src/api/objects/ObjectAPI.js 91.44% <0.00%> (+2.13%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d84808a...18a262d. Read the comment docs.

@nikhilmandlik nikhilmandlik mentioned this pull request Dec 10, 2021
Closed
@nikhilmandlik nikhilmandlik force-pushed the new-import-export-json-test branch from c4fd88c to fde9c5b Compare December 22, 2021 09:06
davetsay
davetsay requested changes Dec 23, 2021
View reviewed changes
Copy link
Contributor

@davetsay davetsay left a comment

Choose a reason for hiding this comment

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

Half of the tests are arrow functions and half are regular ole functions. Can we pick one for all?

A comment, not a request. I'm not sure I understand what the difference between these tests are, ExportAsJSONAction exports object from tree, can export self-containing objects.

Some inline comments. Looking good.

@nikhilmandlik nikhilmandlik force-pushed the new-import-export-json-test branch from 4eef0fc to 8e22a72 Compare December 23, 2021 20:42
@nikhilmandlik
Copy link
Contributor

Half of the tests are arrow functions and half are regular ole functions. Can we pick one for all?

A comment, not a request. I'm not sure I understand what the difference between these tests are, ExportAsJSONAction exports object from tree, can export self-containing objects.

Some inline comments. Looking good.

yeah updated, can export self-containing objects has infinite Parent and child relation, parent contains child and child contains parent.
eg:
Screen Shot 2021-12-23 at 4 54 05 PM

davetsay
davetsay approved these changes Dec 27, 2021
View reviewed changes
Copy link
Contributor

@davetsay davetsay left a comment

Choose a reason for hiding this comment

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

LGTM

Reviewer Checklist
Changes appear to address issue? Y
Appropriate unit tests included? Y
Code style and in-line documentation are appropriate? Y
Commit messages meet standards? Y
Has associated issue been labelled unverified? Y

@davetsay davetsay merged commit 6f9241c into master Dec 27, 2021
@davetsay davetsay deleted the new-import-export-json-test branch December 27, 2021 20:30
@mariuszr mariuszr mentioned this pull request Dec 28, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davetsay davetsay davetsay approved these changes

@shefalijoshi shefalijoshi Awaiting requested review from shefalijoshi

@jvigliotta jvigliotta Awaiting requested review from jvigliotta

@nikhilmandlik nikhilmandlik Awaiting requested review from nikhilmandlik

Assignees

@nikhilmandlik nikhilmandlik

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Many ExportAsJSONAction spec tests are X'd out.
5 participants
@kobe1104 @nikhilmandlik @davetsay @akhenry @unlikelyzero