Inter store requisitions current #456

andreievg · 2017-08-11T05:02:26Z

No description provided.

Should be done via setting !! Will amend

# Conflicts: # src/database/UIDatabase.js # src/localization/modalStrings.js

# Conflicts: # src/dataMigration.js # src/pages/RequisitionsPage.js # src/sync/SyncQueue.js # src/sync/Synchroniser.js # src/sync/syncTranslators.js

Chris-Petty · 2017-08-13T23:13:25Z

package.json

@@ -4,7 +4,7 @@
    "email": "info@sussol.net"
  },
  "name": "mSupplyMobile",
-  "version": "1.1.6",
+  "version": "2.0.0",
  "private": true,
  "license": "proprietary",


I suppose private should be false, and license should be MIT?

Chris-Petty · 2017-08-13T23:15:55Z

src/dataMigration.js

+        requisitions.forEach(requisition => {
+          if (!requisition.isRequest) return;
+          // The reason for database.update is we don't want to update finalized transactions
+          // and incase database.save is replaced by automatic way of invoking db event listeners


Chris-Petty · 2017-08-13T23:44:08Z

src/dataMigration.js

+
+      database.write(() => {
+        requisitions.forEach(requisition => {
+          if (!requisition.isRequest) return;


Given that it is a new getter on Requisition objects, does myRequisition.isRequest work before the migration?

As there are 4 types of Requisition it'd be a bit safer to make a getter isResponse and make that the condition for a block surrounding this code, so it isn't ever run for the other types.

I am pretty sure realm doesn't store getters + functions from datatype in app data. It will construct Requisition data type as it load realm records. So yeah it will have the getter without migration.

In V3 mSupply api four types of requisition, finalised(fn) and suggested(sg) for both isReponse and isRequest, so either isResponse or isRequest have same 'safeness' level

I'm happy with first answer, but second answer is a bit hazy. They types are // imprest, forecast, request or response, that doesn't have anything to do with statuses. If a store somehow has a requisition of type 'imprest' or 'forecast', it'll run this migration code on it.

I think it should be fine to add a otherStoreName as mainSupplyingStoreName for any requisition prior to v3. As they will only be requisitions for main supplying store

Just to clarify the first point in this thread, no worries about myRequisition.isRequest working, because this migration code runs after the realm database is all reinstated and its own internal data migration code has already run. So at this point, all of the requisitions have already been given type: 'request' through the default

Chris-Petty · 2017-08-13T23:45:12Z

src/dataMigration.js

+          if (!requisition.isRequest) return;
+          // The reason for database.update is we don't want to update finalized transactions
+          // and incase database.save is replaced by automatic way of invoking db event listeners
+          // simply assigning property may cause changes to finalized transaction to be synced.


Not 100% sure why you'd not want to sync the change. If you ever had to reinitialise the store (e.g. replaced tablet), you'd want the records that you pull to have the updated values for otherStoreName.

Also: finalised

Should not re sync finalised requisitions as it will cause v2, v3 to replicate requisition. And as for otherStoreName (i.e name_ID), mSupply has migration code that already assigns correct name_ID.

Should add note of why we don't want them being synced, as you just described.

// The reason for database.update is we don't want to update finalized transactions // and incase database.save is replaced by automatic way of invoking db event listeners // simply assigning property may cause changes to finalized transaction to be synced.

That should be descriptive enough

What's going on here?

Should not re sync finalised requisitions as it will cause v2, v3 to replicate requisition.

Surely not?

I should adjust my comment and say, possible will cause v2, v3 to replicate requisition. But I do think that finalised requisition should not be updated

Chris-Petty · 2017-08-13T23:53:43Z

src/database/DataTypes/Requisition.js

@@ -47,6 +51,10 @@ export class Requisition extends Realm.Object {
    this.daysToSupply = months * 30;
  }

+  get isLinkedToRequisition() {
+    return typeof this.linkedRequisition === 'object';


I know we've used it elsewhere, but typeof is considered dangerous in the JS community because it returns 'object' for multiple counterintuitive things. Including, tragically enough, null.

return !!this.linkedRequisition should behave correctly in return turning True for not null. It can only be a Transaction as defined in the schema, or null/undefined.

Good point, I am pretty sure realm will store 'undefined' in object key that has no value. I will change this check to keep with consistency.

Chris-Petty · 2017-08-14T01:26:47Z

src/pages/SupplyRequisitionPage.js

+            sortable: true,
+          },
+          {
+            key: 'ourStockOnHand', // TODO: should this column name be consistent with other pages


Should it? Unless we have excess column width then it'll be good to keep this as is, I wouldn't bother changing the other pages to conform.

Chris-Petty · 2017-08-14T01:28:41Z

src/pages/SupplyRequisitionPage.js

+            alignText: 'right',
+          },
+          {
+            key: 'stockOnHand',


theirStockOnHand

DataType: RequisitionItem.stockOnHand.

Was about to make the same comment as Chris, but valid response

Maybe?

get therStockOnHand() { // check requisition is response // return 0 or even throw and error }

This also bugged me, stockOnHand not very representative

I'm wondering if it is a good practice to throw errors rather than return null with such checks else where. Would make bugsnag logs be more informative!

Might be able to have a universal error modal that handles it all at the root of the app, too.

As for your idea with the get, that along with a get ourStockOnHand could be a nice tweak and minor refactor. I'm happy not bothering though given current time constraints.

I am glad you mentioned this actually. Request requisition.stockOnHand should be 'dynamic' until finalised and update (alongside suggestedQuantity), not set in createRecord. I have other thought on this subject, will try to update by mid Tomorrow otherwise will make an issue.

Chris-Petty · 2017-08-14T01:30:06Z

src/pages/index.js

@@ -17,6 +17,9 @@ import { RequisitionsPage } from './RequisitionsPage';
 import { RequisitionPage,
         checkForFinaliseError as checkForRequisitionFinaliseError,
       } from './RequisitionPage';
+import { SupplyRequisitionPage,
+         checkForFinaliseError as checkForSupplierRequisitionError
+       } from './SupplyRequisitionPage'; // TODO finalise checking


Indeed TODO.

I don't see any reason to do before finalise checking for SupplyRequisition.

Chris-Petty · 2017-08-14T01:31:55Z

src/sync/incomingSyncUtils.js

@@ -274,8 +277,9 @@ export function createOrUpdateRecord(database, settings, recordType, record) {
        serialNumber: record.serial_number,
        requesterReference: record.requester_reference,
        comment: record.comment,
-        enteredBy: getObject(database, 'User', record.user_ID),
+        enteredBy: getObject(database, 'User', record.user_ID), // TODO: should be..


Should be what?

Wasn't sure if it there is a need to change this, look like there is no need.

Chris-Petty · 2017-08-14T01:35:10Z

src/sync/outgoingSyncUtils.js

@@ -118,10 +116,10 @@ function generateSyncData(settings, recordType, record) {
        ID: record.id,
        date_entered: getDateString(record.entryDate),
        user_ID: record.enteredById,
-        name_ID: settings.get(THIS_STORE_NAME_ID),
+        name_ID: record.otherStoreName.id,  // TODO: check if there is a case when outgoing req might not have otherStoreName


Was checked?

I can't think of a case, but will leave this to adjust last (in case there is a case)

edmofro · 2017-08-14T06:39:19Z

src/dataMigration.js

+      const mainSupplyingStoreName = nameResults[0];
+
+      const requisitions = database.objects('Requisition')
+                                   .filtered('otherStoreName = Null').snapshot();


Really capital Null?

Also you should filter here on type = 'request' rather than doing the early return check below

Thanks, nice catch, updated.
As per capital Null, I didn't want to go deep into research, just found the following (since latest realm js docs don't have filtered by undefined/null)
realm/realm-js#162 , then pr file changes https://github.com/realm/realm-js/pull/386/files there an enum .. I didn't know if case sensitive
enum class Type { None, Number, String, KeyPath, Argument, True, False, Null } type; , will check with = null and == null tomorrow

edmofro · 2017-08-14T06:49:32Z

src/dataMigration.js

+
+      database.write(() => {
+        requisitions.forEach(requisition => {
+          if (!requisition.isRequest) return;


Just to clarify the first point in this thread, no worries about myRequisition.isRequest working, because this migration code runs after the realm database is all reinstated and its own internal data migration code has already run. So at this point, all of the requisitions have already been given type: 'request' through the default

edmofro · 2017-08-14T06:55:04Z

src/dataMigration.js

+          database.update('Requisition', {
+            id: requisition.id,
+            otherStoreName: mainSupplyingStoreName,
+          }, 'sync');


I don't like the use of 'sync' here manually. This migration code uses the UIDatabase, which means it isn't causedBy sync, and this is just a hack to stop it from syncing back to the server, which actually seems entirely unnecessary. Yes it might save a bit of bandwidth, but from a logic perspective there is no reason not to sync back to server. But bandwidth is probably not significant...can't see any site having made more than 100 request type requisitions yet.

Sure I see now, database in this context is UIDatabase, thanks. Updated..
I thought finalised was finalised, and most requisitions at migration stage will be finalised, so was meaning to avoid modifying finalised transaction.. If the server api assumed finalised transaction to be 'final', it could have a check on update record, if finalised ignore and log. I think we should assume that finalised transaction/requisition's cannot be edited after v3.

edmofro · 2017-08-14T06:55:53Z

src/database/DataTypes/Requisition.js

@@ -61,6 +65,26 @@ export class Requisition extends Realm.Object {
    this.addItem(requisitionItem);
  }

+  createCustomerInvoice(database, user) {
+    if (this.isRequest || this.isFinalised) {
+      throw new Error('Cannot create invoice from Finalised or Request Requistion ');


edmofro · 2017-08-14T06:57:50Z

src/database/DataTypes/Requisition.js

+      throw new Error('Cannot create invoice from Finalised or Request Requistion ');
+    }
+    if (database.objects('Transaction').
+            filtered('linkedRequisition.id = $0', this.id).length > 0) return;


Tempted to throw an error here...shouldn't call createCustomerInvoice if there's already one created

This is for sync, in case of resync and CustomerInvoice arrived before Requisition. And if CustomerInvoice arrived after Requisition.. this is why this should only be done in postSyncProcess

edmofro · 2017-08-14T07:52:24Z

src/pages/SupplyRequisitionPage.js

+                                                 transactionItem.availableQuantity);
+      requisitionItem.suppliedQuantity = transactionItem.totalQuantity;
+      database.save('TransactionItem', transactionItem);
+      database.save('RequisitionItem', requisitionItem);


Should only need the save for requisition item

but suppliedQuantity has changed.. suppliedQuantity -> record.actualQuan, and should be synced

Basically the rule is:
At the point you use dot notation to change a data object, you must call .save
If you just call a method that manipulates the data object, you don't call .save. You can feel save in the knowledge that if the method uses dot notation to change it, it is the methods responsibility to call .save - so long as writer of the method followed the rules, you are fine (perhaps a good idea to check inside the method if you want to be really sure)

As per comment above where setTotalQuantity is used in CustomerInvoicePage -> before I refactored.

My original comment wasn't very good ( I misread your comment requisition item -> transaction item ). And yes, will leave 'sync' out of save and update talk and use them all the time dot notation assignment is used.

edmofro · 2017-08-14T07:54:22Z

src/pages/SupplyRequisitionPage.js

+        columns={infoColumns}
+        isEditingDisabled={requisition.isFinalised}
+      />
+    );


So much of this class is shared by the regular requisition page

Ahh refactor septemeber/october

edmofro · 2017-08-14T07:57:49Z

src/pages/SupplyRequisitionPage.js

+    }
+  }
+
+  renderButtons() {


Rename to reflect the single button that is being rendered

edmofro · 2017-08-14T07:59:29Z

src/pages/SupplyRequisitionPage.js

+            alignText: 'right',
+          },
+          {
+            key: 'stockOnHand',


Was about to make the same comment as Chris, but valid response

edmofro · 2017-08-14T08:00:18Z

src/pages/SupplyRequisitionPage.js

+
+export function checkForFinaliseError() {
+  return null;
+}


...this seems unnecessary. Or are you going to update with an actual check?

it's gone.. will check

Chris-Petty · 2017-08-15T00:43:00Z

src/pages/RequisitionsPage.js

@@ -31,7 +31,8 @@ export class RequisitionsPage extends React.Component {
      isRequestView: true,
      isCreatingRequisition: false,
    };
-    this.requisitions = props.database.objects('Requisition');
+    this.requisitions = props.database.objects('Requisition')
+                                      .filtered('serialNumber != "-1"');


@edmofro If you're wondering why we're back to doing this in the Requisition page:

SyncPostProcessor uses the UIDatabase, so when trying to find records with serialNumber '-1' it'd never find any because they were already filtered out.

I suppose a different solution would be make another switch option in UIDatabase for 'RawRequisitions' or something that doesn't do any filtering.

Or another function objectsDirect(type) that just passes straight to the realm.objects function without any filters.

Yeah I worked that out after a bit...sorry for sending you astray. I don't think you should at this stage, but if you were going to add to the UIDatabase, I'd go in the other direction...keep Requisition as the 'raw' type, and add a new type for the filtered requisitions.

P.S. listening carefully to this code, you might hear a hint that the postSyncProcessor shouldn't be using the UIDatabase ;-)

Yea, that would be another quick fix (Your P.S.)

Quick and correct IMO. Code smells include:

A bug happened (start sniffing...)

The 'post sync' processor, which has nothing to do with UI, is using something called a 'UIDatabase' (there it is!)

yes I agree, but, this doesn't help in deciphering differences in db wrappers:

const database = new Database(schema); this.database = new UIDatabase(database); this.settings = new MobileAppSettings(this.database); migrateDataToVersion(this.database, this.settings); this.userAuthenticator = new UserAuthenticator(this.database, this.settings); const syncAuthenticator = new SyncAuthenticator(this.settings); this.synchroniser = new Synchroniser(database, syncAuthenticator, this.settings); this.postSyncProcessor = new PostSyncProcessor(this.database, this.settings);

A bit too messy in my opinion, this.database, database

Fix bug qith sync error to push new records

andreievg and others added 29 commits August 1, 2017 17:01

add toggle buttons

3edbcb1

Able to select and display supplying/requesting store

454a916

Migrate database, set un-finalised requisition supplyingStore

683e8aa

Adjust. migration

40c9e08

Add store names and move getOtherStoreName logic to data type

22602ab

Sync logic

f915fb3

Data migration further refinements

bb3bace

Strings

1f1c91c

Update synchroniser to v3

aaf787e

Should be done via setting !! Will amend

Update from #332-fix-lines-si-between-mobile

b8f8523

Fix incoming name bug

4f02a7d

Adding new requisition page

80a8fe7

isRequest check

ec5c654

Extra field added to requisition item

fb95722

Update strings

43858c3

A TODO for unreachable return

69c3790

Adjust table to fit suggestedQuantity

e89120b

Merge branch 'master' into inter-store-requisitions-current

b9d5355

# Conflicts: # src/database/UIDatabase.js # src/localization/modalStrings.js

Link Requisition to Customer invoice

36b229d

Make TransactionItem.setTotalQuantity more generic

89fea40

Migration and otherStoreName added to createRecord

54a423f

Add supply requisition page

cd72f86

Use other store name

3e0333d

Out and In Sync utils

2e46415

Adjust supply requisition page

8062f09

Merge branch 'master' into inter-store-requisitions-current

1b72fdf

# Conflicts: # src/dataMigration.js # src/pages/RequisitionsPage.js # src/sync/SyncQueue.js # src/sync/Synchroniser.js # src/sync/syncTranslators.js

Make customer invoice creation happen in PostSyncProcessing

e65eb14

2.0.0

2cf0333

Fixed missed conflict

28ea011

Chris-Petty requested changes Aug 14, 2017

View reviewed changes

andreievg added 3 commits August 14, 2017 16:13

Fixed bug + trailing commas

2f67768

First check for finalised, before figuring out quantity

a3c0203

Fix bug

2ecb51e

edmofro requested changes Aug 14, 2017

View reviewed changes

andreievg added 15 commits August 14, 2017 21:28

Better check for outgoing sync

44d5b14

No need use update(.., 'sync') it's UIDatabase baby!

35c4d88

Create record handles the save

cf24e29

Update filtered to use ==

9a4b83c

setTotalQuantity minus one

9731ced

mSupplier = InternalSupplier

a445e80

Localisation todo's

f665e6f

Update deconstructed prop's use

f7a0f4e

Make it Edwin pretty :)

9b3c76f

To check (no need for finalise error check callback)

b319265

Change to getObject on name

2fcd5c8

Use toggle bar widget

c3fbde2

Remove extras

2b1325e

Change button name

6b276d8

Fix bug and refactor UIDatabase switch

4160de0

Chris-Petty reviewed Aug 15, 2017

View reviewed changes

andreievg and others added 7 commits August 15, 2017 16:53

Set Suggested and Requested quantities buttons working.

b1c9851

Save requisition after linkedTransaction update

5404f82

Use update to save linked requisition

5e1e808

Merge branch 'master' into inter-store-requisitions-current

3dda71f

Fix bug qith sync error to push new records

75ba00d

Merge pull request #469 from sussol/fix-sync-error

8a942b4

Fix bug qith sync error to push new records

Correct requisition statuses, change Null to null

1ece4b3

edmofro approved these changes Aug 17, 2017

View reviewed changes

edmofro merged commit 34a449a into master Aug 17, 2017

andreievg deleted the inter-store-requisitions-current branch September 8, 2017 03:13

Inter store requisitions current #456

Inter store requisitions current #456

Conversation

andreievg commented Aug 11, 2017

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

Chris-Petty Aug 14, 2017 • edited

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

edmofro Aug 14, 2017 • edited

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

Chris-Petty Aug 14, 2017 • edited

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

andreievg Aug 14, 2017 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chris-Petty Aug 15, 2017 • edited

Choose a reason for hiding this comment

edmofro Aug 15, 2017 • edited

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

Chris-Petty Aug 14, 2017 •

edited

edmofro Aug 14, 2017 •

edited

Chris-Petty Aug 14, 2017 •

edited

andreievg Aug 14, 2017 •

edited

Chris-Petty Aug 15, 2017 •

edited

edmofro Aug 15, 2017 •

edited