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

Wr/data bugs #266

Merged
merged 10 commits into from
Feb 7, 2022
Merged

Wr/data bugs #266

merged 10 commits into from
Feb 7, 2022

Conversation

WillieRuemmele
Copy link
Contributor

@WillieRuemmele WillieRuemmele commented Feb 1, 2022

What does this PR do?

fixes various bugs with data
lots of the QA steps will be found in the associated WI

What issues does this PR fix or reference?

@W-10290520@ forcedotcom/cli#1318
@W-10290524@ forcedotcom/cli#72
@W-5380834@

@WillieRuemmele WillieRuemmele requested a review from a team February 1, 2022 17:13
Copy link
Contributor

@mshanemc mshanemc left a comment

Choose a reason for hiding this comment

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

I like having these new exemplars for the edge cases that keep popping up.

@@ -182,7 +182,11 @@ export abstract class DataCommand extends SfdxCommand {
throw new Error(messages.getMessage('TextUtilMalformedKeyValuePair', [pair]));
} else {
const key = pair.substr(0, eqPosition);
constructedObject[key] = this.convertToBooleanIfApplicable(pair.substr(eqPosition + 1));
if (pair.includes('{') && pair.includes('}')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I could see this being true but not being parseable JSON (ex: a big textArea that's referring to some code like a Case).

So it's probably a fine solution but we'll need a try/catch on the JSON.parse because it might not be, and then we'd want to return the original value?

@@ -247,4 +247,20 @@ describe('data:record commands', () => {
expect(result).to.have.property('Bool__c', false);
});
});

it('will parse JSON correctly for update', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's have a test to match the above comment--something that isn't valid json but contains curly brackets.

}

protected stringToDictionary(str: string): Dictionary<string | boolean> {
protected stringToDictionary(str: string): Dictionary<string | boolean | Record<string, unknown>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

For the sake of readability you might consider making a new type for this.

@mshanemc
Copy link
Contributor

mshanemc commented Feb 7, 2022

QA notes

force:data:soql:query -t -q "select id, Metadata from RemoteProxy"
✅ has valid output
same, but with the description field being invalid json "description": "Can I nest some {invalid JSON} here"
✅ has valid output (treats the invalid json as string)
same, but the description field is actual json "{"foo":"bar"}"
✅ escapes it as JSON "description": "\"{\"foo\":\"bar\"}\""
same, but with nested JSON and no outer quotes {"foo":"bar", "nested": {"a": 15}}
✅ fully escaped "description": "{\"foo\":\"bar\", \"nested\": {\"a\": 15}}"

@mshanemc
Copy link
Contributor

mshanemc commented Feb 7, 2022

force:data:soql:query -q "SELECT Amount, Id, Name, (SELECT Quantity, ListPrice,PriceBookEntry.UnitPrice, PricebookEntry.Name,PricebookEntry.product2.Family FROM OpportunityLineItems) FROM Opportunity" with setup per WI
✅ 👎🏻 that's an awful looking table, BUT it does have all the values

Screen Shot 2022-02-07 at 12 29 05 PM

@mshanemc
Copy link
Contributor

mshanemc commented Feb 7, 2022

QA notes:
✅ works!

➜  dataBugs ../../plugin-data/bin/run force:data:record:update \                                                                                                                                                                              
    --sobjecttype RemoteProxy \
    --sobjectid 0rp2D000000CZjTQAW \
    --usetoolingapi \
    -v "Metadata='{\"disableProtocolSecurity\": false,\"isActive\": true,\"url\": \"https://www.example.com\",\"urls\": null,\"description\": null}'"
Successfully updated record: 0rp2D000000CZjTQAW.
Updating Record... Success

@mshanemc mshanemc merged commit 801a7aa into main Feb 7, 2022
@mshanemc mshanemc deleted the wr/dataBugs branch February 7, 2022 19:00
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