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

Content Management API Sample #1

Merged
merged 17 commits into from Jul 18, 2019

Conversation

@jmerlan
Copy link
Contributor

commented Jul 16, 2019

[adding a few screenshots - varga]
ContentManagement_8GWiNyU0q9
ContentManagement_seVyuHDjST
ContentManagement_Hv2wBwhAZZ
ContentManagement_Omm3XxDpe6
ContentManagement_VcDM3DMDXt

@matthewcmorgan

This comment has been minimized.

Copy link
Member

commented Jul 16, 2019

As this is to be open sourced, please review with that in mind.

@matthewcmorgan matthewcmorgan requested a review from andyplatter Jul 16, 2019

Show resolved Hide resolved ContentManagement/Secrets.cs Outdated
// Pass the list of unique list items as Family Type names array
var uniqueItems = new HashSet<string>(familyTypes);
familyTypes = uniqueItems.ToList();

This comment has been minimized.

Copy link
@terrypoot

terrypoot Jul 16, 2019

This could be simplified by declaring familyTypes as a HashSet rather than declaring it a List and then copying it into a HashSet.

}

// TODO: Ensure that only one content is returned rather than hardcoding the first item in list
if (content.Count > 1) { MessageBox.Show("Warning, more than one piece of content matches this query.", "Warning"); }

This comment has been minimized.

Copy link
@terrypoot

terrypoot Jul 16, 2019

It looks like this TODO is done.

Unifi.Batch batchManufacturer =
Unifi.SetTypeParameterValue(unifiToken, selectedItem, familyTypeName, "Manufacturer", txtBxManufacturer.Text, "TEXT", 2016);
Unifi.Batch batchModel = Unifi.SetTypeParameterValue(unifiToken, selectedItem, familyTypeName, "Model", txtBxModel.Text, "TEXT", 2016);

This comment has been minimized.

Copy link
@terrypoot

terrypoot Jul 16, 2019

Seems odd to hard-code the revit year here, when the search is not limited by revit year. If they select a non-2016 file, I don't think this will do what they expected.

@Liath
Copy link
Member

left a comment

We should get you setup with Resharper, it should take care of a lot of these comments on style things for you.

Also, your JSON payloads are kind of hard to reason about. I don't come from C# land but there has to be a cleaner syntax for this.

[assembly: AssemblyConfiguration("")]
[assembly: AssemblyCompany("")]
[assembly: AssemblyProduct("ContentManagement")]
[assembly: AssemblyCopyright("Copyright © 2019")]

This comment has been minimized.

Copy link
@Liath

Liath Jul 16, 2019

Member

Should probably set values for copyright and include a permissive license so people aren't too worried about legal stuff to incorporate snippets from this project.

This comment has been minimized.

Copy link
@matthewcmorgan

matthewcmorgan Jul 16, 2019

Member

Should probably set values for copyright and include a permissive license so people aren't too worried about legal stuff to incorporate snippets from this project.

In the repository, we set it up with an MIT license, would be a good idea to include that here as well.

gridBatchMonitor.Visibility = Visibility.Hidden;

// Get all libraries from Unifi and display in the libraries combobox
List<Unifi.Library> libraries = Unifi.GetLibraries(unifiToken);

This comment has been minimized.

Copy link
@Liath

Liath Jul 16, 2019

Member

C# isn't my native language by any means but I feel like explicitly declaring the type of a variable is one of those things @scottperrineunifi got upset with me over. :)

This comment has been minimized.

Copy link
@ianrmcguire

ianrmcguire Jul 16, 2019

We've tended towards doing var libraries = Unifi.GetLibraries(unifiToken);

This comment has been minimized.

Copy link
@terrypoot

terrypoot Jul 16, 2019

Honestly, for open source code, I think having the actual types there is better. Using var infers it from the return type, but if the user doesn't know the library code, the return type isn't as easily accessible.

This comment has been minimized.

Copy link
@Liath

Liath Jul 17, 2019

Member

My javascript might be showing but does the type actually matter enough to need to be shown every time? Like, if you're just reading through the code you should be able to tell pretty quickly from how the variable is declared and used what it is. It's like reading fantasy, I don't need to know the composition of the city guards armor and the author won't bother mentioning it unless it becomes relevant to the story.

This comment has been minimized.

Copy link
@ridespirals

ridespirals Jul 17, 2019

Member

Not really. If they're using visual studio, they will have intellisense and autocomplete to give them information about the types, anyway.

// Get an access token using basic authentication (username and password)
// Optionally create a file named "Secrets.cs" and add fields called UnifiUsername and UnifiPassword and add your credentials for the below code to work.
// Don't forget to ensure that this file is added to your .gitignore file.
string unifiToken = Unifi.GetAccessToken(Secrets.UnifiUsername, Secrets.UnifiPassword);

This comment has been minimized.

Copy link
@Liath

Liath Jul 16, 2019

Member

This seems okay for a demo app but we should probably pull these from the environment like any other app we build.

This comment has been minimized.

Copy link
@scottperrineunifi

scottperrineunifi Jul 17, 2019

Storing your password in an environment variable = secure? 🤔

/// <param name="password">UNIFI password</param>
/// <returns></returns>
public static string GetAccessToken(string username, string password) {
var client = new RestClient("https://api.unifilabs.com/login");

This comment has been minimized.

Copy link
@Liath

Liath Jul 16, 2019

Member

I thought we deprecated user/password access?

This comment has been minimized.

Copy link
@shammils

shammils Jul 16, 2019

The API, namely the jwt authorizer allows api keys and tokens still. We decided to not forced required API keys in the integration response for this reason.

This comment has been minimized.

Copy link
@matthewcmorgan

matthewcmorgan Jul 16, 2019

Member

Do we intend to allow user/password access for the foreseeable future? If not, we should refactor this to use the method we do intend to allow...

var request = new RestRequest(Method.POST);
request.AddHeader("cache-control", "no-cache");
request.AddHeader("Content-Type", "application/json");
request.AddParameter("undefined", "{\n\t\"username\": \"" + username + "\",\n\t\"password\": \"" + password + "\"\n}\n", ParameterType.RequestBody);

This comment has been minimized.

Copy link
@Liath

Liath Jul 16, 2019

Member
Suggested change
request.AddParameter("undefined", "{\n\t\"username\": \"" + username + "\",\n\t\"password\": \"" + password + "\"\n}\n", ParameterType.RequestBody);
request.AddParameter("undefined", "{ \"username\": \"" + username + "\", \"password\": \"" + password + "\" }", ParameterType.RequestBody);
Show resolved Hide resolved ContentManagement/Unifi.cs Outdated
Show resolved Hide resolved ContentManagement/Unifi.cs Outdated
Show resolved Hide resolved ContentManagement/Unifi.cs Outdated
/// <param name="input">The string to convert</param>
/// <returns></returns>
private static string ToLiteral(string input) {
using (var writer = new StringWriter()) {

This comment has been minimized.

Copy link
@Liath
Show resolved Hide resolved ContentManagement/Unifi.cs Outdated
}
}

public partial class Content {

This comment has been minimized.

Copy link
@ianrmcguire

ianrmcguire Jul 16, 2019

I don't think these need to be partial classes

@ianrmcguire

This comment has been minimized.

Copy link

commented Jul 16, 2019

We should get you setup with Resharper, it should take care of a lot of these comments on style things for you.

Also, your JSON payloads are kind of hard to reason about. I don't come from C# land but there has to be a cleaner syntax for this.

C# 6 introduced string interpolation for a cleaner string generation with variables: https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/tokens/interpolated

Show resolved Hide resolved ContentManagement/Unifi.cs Outdated
/// <param name="content">The UNIFI Content object to get parameters from.</param>
/// <returns></returns>
public static List<string> GetFamilyTypes(Content content) {
List<string> familyTypes = new List<string>();

This comment has been minimized.

Copy link
@ridespirals

ridespirals Jul 16, 2019

Member

I'm not sure how interested we are in making this sample project adhere to our own internal company coding standards, but if we so we probably want to do another resharper pass to replace all the type references to var, as is our standard.

This comment has been minimized.

Copy link
@ianrmcguire

ianrmcguire Jul 16, 2019

I liked Terry's comment that for open source where they aren't as familiar with the types we pass around, declare the type as return type isn't as easily accessible

Show resolved Hide resolved ContentManagement/Unifi.cs Outdated
@ridespirals

This comment has been minimized.

Copy link
Member

commented Jul 16, 2019

One general note, technically speaking UNIFI should always be written in all caps. We don't have the documented anywhere, I've tried finding the original email where this style guide was originally written but yeah, just for consistency throughout our platform, I try to make sure that any "UNIFI" text visible to customers is always written in all caps.

@ridespirals

This comment has been minimized.

Copy link
Member

commented Jul 16, 2019

Just for fun I added our UNIFI logo icon.

@helenperrineunifi

This comment has been minimized.

Copy link

commented Jul 16, 2019

We should consider renaming the solution/project to fit our UNIFI naming standards as well. And update the assembly info.

@ianrmcguire

This comment has been minimized.

Copy link

commented Jul 16, 2019

Do we want a read.me for this repo?

@matthewcmorgan

This comment has been minimized.

Copy link
Member

commented Jul 16, 2019

Do we want a read.me for this repo?

Yes, we definitely should make sure there is a README.md for this repo.

jmerlan and others added some commits Jul 16, 2019

@ridespirals

This comment has been minimized.

Copy link
Member

commented Jul 17, 2019

We should consider renaming the solution/project to fit our UNIFI naming standards as well. And update the assembly info.

Maybe we can make the namespace UnifiLabs.Samples.ContentManagement

@ridespirals

This comment has been minimized.

Copy link
Member

commented Jul 17, 2019

Just pushed commits that rename the project/solution/directories to match our convention, and updated assemblyInfo.

ridespirals added some commits Jul 18, 2019

use a listBox for batch statuses
looks a little nicer. will also render a Loading element that's replaced with the result after it comes back
refactor batchStatus to be async
prevents UI from locking up while waiting. also allows the ListBox to render the Loading... item, which gets replaced after the response comes back
prevent adding rows, make dataGrid readonly
also removed some weird RenderTransform stuff from the combo

@ridespirals ridespirals merged commit 5f739ad into master Jul 18, 2019

@ridespirals ridespirals deleted the initial branch Jul 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.