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

VSCODE-183: Edit documents from collection #239

Merged
merged 5 commits into from
Jan 18, 2021

Conversation

alenakhineika
Copy link
Contributor

@alenakhineika alenakhineika commented Jan 15, 2021

Description

Provide the ability to edit documents from the collection view so that when a user is looking at a collection they can quickly make changes to the documents without having to go through playgrounds.

  • Documents in the collection view have a code lens to access the editing functionality.
  • By clicking on the code lens, the user is taken to a new editable version of the document, displayed in EJSON.
  • When the document is saved (Cmd/Ctrl + S), the document is updated to reflect the changes that were made.
  • When the document is saved, a notification informs the user that the change has been saved in MongoDB.
  • When the update results in an error, the user is notified.
  • It's possible to make edits to a document and save it more than once.
  • Track Document Edited and Document Updated telemetry events.

https://jira.mongodb.org/browse/VSCODE-183

Checklist

  • New tests and/or benchmarks are included
  • Documentation is changed or added

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

@alenakhineika alenakhineika marked this pull request as draft January 15, 2021 22:07
@alenakhineika alenakhineika marked this pull request as ready for review January 16, 2021 16:14
@alenakhineika alenakhineika requested a review from Anemy January 16, 2021 16:30
Copy link
Member

@Anemy Anemy left a comment

Choose a reason for hiding this comment

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

Nice - tried it out, works well. If we want we could show a different message when a user tries to edit a document from a view, but I think how it is currently is fine (it says document cannot be found). Couple small questions

@@ -21,8 +21,9 @@ MongoDB Playgrounds are the most convenient way to prototype and execute CRUD op

- Prototype your queries, aggregations, and MongoDB commands with MongoDB syntax highlighting and intelligent autocomplete for MongoDB shell API, MongoDB operators, and for database, collection, and field names.
- Run your playgrounds and see the results instantly. Click the play button in the tab bar to see the output.
- Save your playgrounds in your workspace and use them to document how your application interacts with MongoDB
- Build aggregations quickly with helpful and well-commented stage snippets
- Edit documents returned by your playground.
Copy link
Member

Choose a reason for hiding this comment

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

👍

throw new Error(errorMessage);
}

const find = util.promisify(dataservice.find.bind(dataservice));
Copy link
Member

Choose a reason for hiding this comment

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

🔥

export enum DocumentSource {
DOCUMENT_SOURCE_TREEVIEW = 'treeview',
DOCUMENT_SOURCE_PLAYGROUND = 'playground',
DOCUMENT_SOURCE_COLLECTIONVIEW = 'collectionview'
Copy link
Member

Choose a reason for hiding this comment

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

I think we already have a few enums and types in the util folder. Maybe there's a better place we can have them - is the source only used by telemetry?

Copy link
Contributor Author

@alenakhineika alenakhineika Jan 18, 2021

Choose a reason for hiding this comment

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

Yes, we use it for telemetry, but in order to make it work, we need to pass it to several places in code. And I found it weird to require a telemetry model in types.ts to import this enum therefore moved it to a separate file. I also have noticed that we have types in type.js, but also separate files with types such as connectionModelType.ts or dataServiceType.ts. I think it would be nice to place all of them together. Eiter all in a single types file or each type into a separate file and place, all types files to a single types folder. But I think to do it after discussion with you as a small separate PR. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

sure sounds good

sinon.replace(vscode.workspace, 'openTextDocument', mockOpenTextDocument);

const mockShowTextDocument = sinon.fake.resolves();
const mockShowTextDocument: any = sinon.fake();
Copy link
Member

Choose a reason for hiding this comment

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

does the sinon fake type work here? we might be able to avoid some of the anys

Copy link
Contributor Author

@alenakhineika alenakhineika Jan 18, 2021

Choose a reason for hiding this comment

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

The thing with this any that when we import Sinon instead of requiring it, linter starts complaining about additional things and sinon.SinonSpy that fits here for the sinon.replace(vscode.workspace, 'showTextDocument', mockShowTextDocument); replacement starts erroring with Property 'firstArg' does not exist on type 'SinonSpy<any[], any>. We can revisit this later and try to find something better than the any type, but for now, I think it is ok to use any, since before it was without a type anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha sounds good. Will be nice to get rid of some of the anys one day. Importing things with their types is a good start. Maybe firstArg is an internal thing or something we shouldn't be using 🤔 should be fine to handle it another time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe everything is ok with firstArg, but we need to figure out how to property create mocks with typescript and Sinon. I was reading the internet about it and looks like it worked with the earlier versions of typescript and start erroring at some point.

@alenakhineika alenakhineika merged commit 64b2aba into master Jan 18, 2021
@alenakhineika alenakhineika deleted the VSCODE-183-edit-documents-from-collection branch January 18, 2021 12:56
@Anemy Anemy mentioned this pull request Jan 25, 2021
@localjo
Copy link

localjo commented Feb 19, 2021

I have a question about this feature. From the readme and the screenshot added in #241 it seems that I should be able to write a query in a playground, and then edit the result, but when I try to edit the result, I get a message saying that I can't edit in a read-only editor. I'm using version 0.4.2 of the plugin, so I'm curious what I might be doing wrong that I am not able to edit the playground result.

Screen Shot 2021-02-19 at 3 51 17 PM

@localjo
Copy link

localjo commented Feb 19, 2021

Update: the problem was that I had Code Lens disabled. Enabling Code Lens fixed the problem. 😄

Screen Shot 2021-02-19 at 4 05 06 PM

@alenakhineika
Copy link
Contributor Author

@localjo hey, just wanted to ask you to check the code lens setting, but you found it first 😀 thx for trying the extension! and feel free to reach out in the future if you have any other questions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants