jclouds-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ignasi Barrera <notificati...@github.com>
Subject Re: [jclouds/jclouds-labs] JCLOUDS-1216 - oneandone-user-api (#336)
Date Tue, 13 Dec 2016 14:14:35 GMT
nacx requested changes on this pull request.

Thanks @alibazlamit!

> +
+@AutoValue
+public abstract class User {
+
+   public abstract String id();
+
+   public abstract String name();
+
+   @Nullable
+   public abstract String description();
+
+   @Nullable
+   public abstract String email();
+
+   @Nullable
+   public abstract String state();

Should this be of type `UserState`?

> +         public abstract Builder email(String email);
+
+         public abstract Builder password(String password);
+
+         public abstract UpdateUser build();
+      }
+   }
+
+   @AutoValue
+   public abstract static class AddUserIp {
+
+      public abstract List<String> ips();
+
+      @SerializedNames({"ips"})
+      public static AddUserIp create(List<String> ips) {
+         return new AutoValue_User_AddUserIp(ips == null ? ImmutableList.<String>of()
: ips);

Enforce an immutable list if the parameter is not null.

> +
+   @AutoValue
+   public abstract static class AddUserIp {
+
+      public abstract List<String> ips();
+
+      @SerializedNames({"ips"})
+      public static AddUserIp create(List<String> ips) {
+         return new AutoValue_User_AddUserIp(ips == null ? ImmutableList.<String>of()
: ips);
+      }
+   }
+
+   @AutoValue
+   public abstract static class Api {
+
+      public abstract Boolean active();

Prefer primitive types for non-nullable fields.

> +   }
+
+   @AutoValue
+   public abstract static class Api {
+
+      public abstract Boolean active();
+
+      @Nullable
+      public abstract String key();
+
+      @Nullable
+      public abstract List<String> allowedIps();
+
+      @SerializedNames({"active", "key", "allowed_ips"})
+      public static Api create(Boolean active, String key, List<String> allowedIps)
{
+         return new AutoValue_User_Api(active, key, allowedIps == null ? ImmutableList.<String>of()
: allowedIps);

Immutable when not null.

> +
+      @Nullable
+      public abstract String key();
+
+      @Nullable
+      public abstract List<String> allowedIps();
+
+      @SerializedNames({"active", "key", "allowed_ips"})
+      public static Api create(Boolean active, String key, List<String> allowedIps)
{
+         return new AutoValue_User_Api(active, key, allowedIps == null ? ImmutableList.<String>of()
: allowedIps);
+      }
+
+      @AutoValue
+      public abstract static class UpdateApi {
+
+         public abstract Boolean active();

Same about primitives.

> +import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import org.apache.jclouds.oneandone.rest.domain.User;
+import org.apache.jclouds.oneandone.rest.domain.options.GenericQueryOptions;
+import org.apache.jclouds.oneandone.rest.filters.AuthenticateRequest;
+import org.jclouds.Fallbacks;
+import org.jclouds.rest.annotations.BinderParam;
+import org.jclouds.rest.annotations.Fallback;
+import org.jclouds.rest.annotations.MapBinder;
+import org.jclouds.rest.annotations.RequestFilters;
+import org.jclouds.rest.annotations.SelectJson;
+import org.jclouds.rest.binders.BindToJsonPayload;
+
+@Path("/users")
+@Produces("application/json")

Remove this and leave only the method specific ones

> +   public void testGet() {
+      User result = userApi().get(currentUser.id());
+
+      assertEquals(result.id(), currentUser.id());
+   }
+
+   @Test(dependsOnMethods = "testGet")
+   public void testUpdate() throws InterruptedException {
+      //this test will fail too since its not allowed to update other users from the API
+//      String updatedDescription = "updatejclouds";
+//
+//      User updateResult = userApi().update(currentUser.id(), User.UpdateUser.builder()
+//              .description(updatedDescription)
+//              .build());
+//
+//      assertEquals(updateResult.description(), updatedDescription);

Can't we configure these tests to update the logged user? Could create a new API context for
the just created user and use that to implement these tests?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/336#pullrequestreview-12683948
Mime
View raw message