Implement txs in event, property and allocator #182

spderosso · 2018-05-11T05:29:13Z

It also updates the dvconfig.json files of these cliches in preparation for #181

maryam-a

In general:

Maybe you should have constants for repeated strings or have an object containing the various states, e.g. const STATES = { COMMIT: 'commit', ABORT: 'abort', ...}
Possibly have a new line before each case statement to increase readability
Lint the files
Any comments that I gave in a server file should be applied to all other server files

maryam-a · 2018-05-11T15:33:53Z

packages/catalog/allocator/server/server.ts

+  static async allocationExistsOrFail(id: string): Promise<void> {
+    const alloc: AllocationDoc | null = await allocations
+      .findOne({ id: id }, { projection: { _id: 1 } });
+    if (alloc === null) {


What if you used _.isNil

In this case, it is very clear that the value cannot be undefined (note that the type of alloc defined in the line above is AllocationDoc | null)

maryam-a · 2018-05-11T15:36:09Z

packages/catalog/allocator/server/server.ts

+      .findOne(
+        {id: allocationId, 'assignments.resourceId': resourceId},
+        { projection: { _id: 1 } });
+    if (alloc === null) {


maryam-a · 2018-05-11T15:38:08Z

packages/catalog/allocator/server/server.ts

+              allocationId, resourceId);
+            const updateObj = await allocations
+              .updateOne(
+                {


This query is the same as above. Maybe you can have a query object constant to reduce code repetition

maryam-a · 2018-05-11T15:44:12Z

packages/catalog/allocator/server/server.ts

+            throw new Error(CONCURRENT_UPDATE_ERROR);
+          }
+
+          return true;


return pendingUpdateObj.matchedCount === 1

it can't be 0 (because if otherwise an error is thrown) and we are doing updateOne so it can't be > 1

maryam-a · 2018-05-11T15:46:01Z

packages/catalog/allocator/server/server.ts

+              result: (gqlResp.errors) ? 'no' : 'yes',
+              payload: gqlResp
+            };
+          case 'abort':


fall through?

/* fall through */ is not necessary when you have an empty case

maryam-a · 2018-05-11T15:48:05Z

packages/catalog/event/server/server.ts

-      if (!_.isEmpty(setObject)) {
-        await events.updateOne({ id: input.id }, { $set: setObject });
+      if (_.isEmpty(setObject)) {
+        return false;


throw an error?

hmm maybe? It's not crazy to return false here as it is ok for the update to be empty, but I don't have a strong opinion about it.

maryam-a · 2018-05-11T16:00:07Z

packages/catalog/property/server/server.ts

+  [field: string]: any;
+}
+
+interface Pending {


You don't have this in the other server files. Why is it different for this one?

see ObjectDoc

maryam-a · 2018-05-11T16:01:34Z

packages/catalog/property/src/app/property/show-object/show-object.component.ts

@@ -43,10 +44,14 @@ export class ShowObjectComponent implements OnInit, OnChanges {
    if (!this.gs) {
      return;
    }
-    this.properties = await properties(
-      this.showOnly, this.showExclude, this.fetchProperties.bind(this));
+    if (!this.properties) {


Maybe use !_.isEmpty?

as it is right now you can pass [] for it to fetch nothing but I guess we don't use that. Either way, this is really not a big deal

spderosso · 2018-05-16T23:13:26Z

@maryam-a ready!

spderosso · 2018-05-16T23:16:46Z

Maybe you should have constants for repeated strings or have an object containing the various states, e.g. const STATES = { COMMIT: 'commit', ABORT: 'abort', ...}

Note that we have:

interface Context {                                                                                  
    reqType: 'vote' | 'commit' | 'abort' | undefined; 
    ...
}

so if you have a typo in e.g., commit the compiler will detect it. So the main reason to use an enum is gone because of that. The only thing you gain by having an enum is if in the future we change 'commit' to be some other string but I don't think that's ever going to happen.

maryam-a · 2018-05-18T05:14:46Z

packages/catalog/allocator/dvconfig.json

+  "startServer": true,
+  "actions": {
+    "package": {
+      "include": [ "src/app/allocator/**" ]


./src/app/allocator/** to support Windows

Are you sure it doesn't work on Windows? We are getting the cwd with process.cwd() and passing that to globs. It should do the right thing.

Adding the dot doesn't work on unix and I don't think it would work on Windows either. See isaacs/node-glob#309

Actually, you are right. Sometimes you have to include the ./ and sometimes you don't so I got confused here.

maryam-a · 2018-05-18T05:14:59Z

packages/catalog/allocator/dvconfig.json

+    },
+    "app": {
+      "names": [
+        { "for": "src/app/app.component.html", "use": "allocator-root" }


maryam-a · 2018-05-18T05:19:02Z

packages/catalog/event/dvconfig.json

+  },
+  "actions": {
+    "package": {
+      "include": [ "src/app/event/**" ]


./src/app/event/**

maryam-a · 2018-05-18T05:19:48Z

packages/catalog/event/dvconfig.json

+    },
+    "app": {
+      "names": [
+        { "for": "src/app/app.component.html", "use": "event-root" }


update as above

and all other dvconfig files

maryam-a · 2018-05-18T05:27:26Z

packages/catalog/property/src/app/property/show-object/show-object.component.ts

@@ -22,7 +22,8 @@ export class ShowObjectComponent implements OnInit, OnChanges {
  @Input() showExclude: string[];
  @Output() loadedObject = new EventEmitter<any>();

-  properties: string[];
+  // Internal input
+  @Input() properties: string[];


internal input?

The idea is that the input is not for users to use. It is for actions inside the cliche to pass the fetched properties to show-object so as to avoid doing an unnecessary request

Why not use private then?

maryam-a · 2018-05-18T18:24:43Z

packages/catalog/property/src/app/property/show-object/show-object.component.ts

@@ -22,7 +22,8 @@ export class ShowObjectComponent implements OnInit, OnChanges {
  @Input() showExclude: string[];
  @Output() loadedObject = new EventEmitter<any>();

-  properties: string[];
+  // Internal input
+  @Input() properties: string[];


Why not use private then?

spderosso requested a review from maryam-a May 11, 2018 05:29

maryam-a requested changes May 11, 2018

View reviewed changes

spderosso mentioned this pull request May 16, 2018

Extract action trees from html #181

Merged

spderosso added 10 commits May 16, 2018 14:05

update group dvconfig

d3f935c

update allocator dvconfig

619e409

update event

68904bb

update property

8c771d4

update morg

7e0ef61

implement txs in allocator

81a14de

implement txs in property

8b8bed6

implement txs in event

555f3e7

only fetch data when necessary in property

21f0a65

add no exec request action info to property

b245a0b

spderosso force-pushed the implement-txs branch from 02cae08 to b245a0b Compare May 16, 2018 18:07

spderosso added 2 commits May 16, 2018 14:17

move show-consumer template to separate file

d3b6a7a

reuse repeated queries

e3965ec

lint fixes

a82e49f

maryam-a reviewed May 18, 2018

View reviewed changes

maryam-a requested changes May 18, 2018

View reviewed changes

maryam-a approved these changes May 18, 2018

View reviewed changes

spderosso merged commit 33a3d3e into ops-tmp May 18, 2018

spderosso deleted the implement-txs branch September 7, 2018 20:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement txs in event, property and allocator #182

Implement txs in event, property and allocator #182

spderosso commented May 11, 2018

maryam-a left a comment •

edited

Loading

maryam-a May 11, 2018

spderosso May 16, 2018

maryam-a May 11, 2018

maryam-a May 11, 2018

maryam-a May 11, 2018

spderosso May 16, 2018

maryam-a May 11, 2018

spderosso May 16, 2018

maryam-a May 11, 2018

spderosso May 16, 2018

maryam-a May 11, 2018

spderosso May 16, 2018

maryam-a May 11, 2018

spderosso May 16, 2018

spderosso commented May 16, 2018

spderosso commented May 16, 2018

maryam-a May 18, 2018

spderosso May 18, 2018

maryam-a May 18, 2018

maryam-a May 18, 2018

maryam-a May 18, 2018

maryam-a May 18, 2018

maryam-a May 18, 2018

maryam-a May 18, 2018

spderosso May 18, 2018

maryam-a May 18, 2018

maryam-a May 18, 2018

Implement txs in event, property and allocator #182

Implement txs in event, property and allocator #182

Conversation

spderosso commented May 11, 2018

maryam-a left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spderosso commented May 16, 2018

spderosso commented May 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maryam-a left a comment •

edited

Loading