Order: Displaying Notes #73

mindgraffiti · 2018-05-29T19:52:34Z

Fixes #13

To test:

Navigate to Orders
Choose Order No. 1107 or Order No. 1044. "Add a note" row should display and order notes should display.

Notes: The table cells are using default separator styles, which looks different compared to the mockups.

… error or order notes not available

houndci-bot · 2018-05-29T19:54:18Z

WooCommerce/Classes/ViewRelated/ReusableViews/AddItemTableViewCell.swift

+        super.awakeFromNib()
+        imageView?.tintColor = StyleManager.wooCommerceBrandColor
+    }
+


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

houndci-bot · 2018-05-29T19:54:18Z

WooCommerce/Classes/ViewRelated/Orders/OrderDetails/OrderNoteTableViewCell.swift

+
+        // Configure the view for the selected state
+    }
+


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

mindgraffiti · 2018-05-29T19:54:56Z

WooCommerce/Classes/ViewRelated/Orders/OrderDetails/OrderDetailsViewController.swift

+        case let cell as AddItemTableViewCell:
+            cell.configure(image: viewModel.addNoteIcon, text: viewModel.addNoteText)
+        case let cell as OrderNoteTableViewCell where row == .orderNote:
+            cell.configure(with: viewModel.orderNotes[indexPath.row - 1])


Not sure how to express this better. it's [indexPath.row - 1] because the first indexPath.row is the "Add a note" row.

mindgraffiti · 2018-05-29T19:56:04Z

WooCommerce/DerivedSources/ApiCredentials.swift

@@ -0,0 +1,32 @@
+/// WooCommerce API Credentials. Generated on May 29, 2018 at 11:16:32


Should this be in git ignore?

This is in git ignore. Not sure what happened, but please please 🔥

jleandroperez

UI looking great! Few incoming comments, thanks Thuy!!

jleandroperez · 2018-06-18T18:22:22Z

WooCommerce/DerivedSources/ApiCredentials.swift

@@ -0,0 +1,32 @@
+/// WooCommerce API Credentials. Generated on May 29, 2018 at 11:16:32


This is in git ignore. Not sure what happened, but please please 🔥

jleandroperez · 2018-06-18T18:23:12Z

WooCommerce/DerivedSources/InfoPlist.h

+
+/// WordPress.com Auth Callback Scheme (AKA Magic Link!)
+///
+#define DOTCOM_AUTH_SCHEME woocommerce


Same as above. The entire DerivedSources is in gitignore, please please, remove, and make sure not to commit that, never. It's all good for now, since the project is closed. But if this was OSS, this would probably not be good!

jleandroperez · 2018-06-21T19:12:55Z

WooCommerce/Classes/Model/Order.swift

@@ -20,7 +20,7 @@ struct Order: Decodable {
    let items: [OrderItem]
    let currency: String
    let total: String
-    let notes: [OrderNote]?
+    var notes: [OrderNote]?


(It's okay, no need to change anything here!!). We're using immutable data models, which means that the app itself should never update / modify fields directly.

Ideally speaking, the Service Layer ("Yosemite.framework") posts changes to the backend, and the data flows back from the Networking layer (OR service layer itself) to the UI .

Nevertheless, since we're removing this file in a little bit, and this is only for testing purposes, super OK to leave it this way!

jleandroperez · 2018-06-21T19:17:10Z

WooCommerce/Classes/ViewModels/OrderDetailsViewModel.swift

+            }
+        }
+
+        return array


Swifty snippet:

guard let notes = notes else { return [] } return notes.map { note in return OrderNoteViewModel(with: note) }

(If notes wasn't optional, the guard wouldn't even be needed)

jleandroperez · 2018-06-21T19:19:18Z

WooCommerce/Classes/ViewModels/OrderNoteViewModel.swift

+        // "date_created": "2017-03-21T16:46:41",
+        let format = DateFormatter()
+        format.dateFormat = "yyyy-MM-dd'T'HH:mm:ss"
+        date = format.date(from: orderNote.dateCreated)!


Please replace with DateFormatter.Defaults.dateTimeFormatter (which should do the same).

Perhaps the date field should be optional, itself? (it'd be a very very good idea to remove the !)

The dateTimeFormatter method is an extension only found in the Networking subproject. Do I copy it over to the main project or is there somewhere else I should add it?

This reverts commit 99a42f8.

mindgraffiti · 2018-06-25T13:46:36Z

@jleandroperez thank you for the review! It's ready for a second look.

This reverts commit 3c8140b.

Adds initial Fabric / Crashlytics integration

YosemiteTests: Fixing Runtime Errors

jleandroperez

Few nipticky comments, and one crash. Steps:

Launch WC
Press over the Orders TabBar button
Press over the first row

As a result, the app is crashing. Stack trace below!

706F150-66CA-4CB4-9CB5-697716718DE2/WooCommerce.app> (loaded)' with name 'AddItemTableViewCell''
*** First throw call stack:
(
	0   CoreFoundation                      0x0000000109a091e6 __exceptionPreprocess + 294
	1   libobjc.A.dylib                     0x0000000108b5c031 objc_exception_throw + 48
	2   CoreFoundation                      0x0000000109a7e975 +[NSException raise:format:] + 197
	3   UIKit                               0x0000000105d192e9 -[UINib instantiateWithOwner:options:] + 501
	4   UIKit                               0x00000001059d2cc4 -[UITableView _dequeueReusableViewOfType:withIdentifier:] + 613
	5   UIKit                               0x00000001059d31bb -[UITableView _dequeueReusableCellWithIdentifier:forIndexPath:usingPresentationValues:] + 148
	6   UIKit                               0x00000001059d30f3 -[UITableView dequeueReusableCellWithIdentifier:forIndexPath:] + 91
	7   WooCommerce                         0x00000001044ab7da _T011WooCommerce26OrderDetailsViewControllerC05tableE0So07UITableE4CellCSo0hE0C_10Foundation9IndexPathV12cellForRowAttF + 634
	8   WooCommerce                         0x00000001044aca0c _T011WooCommerce26OrderDetailsViewControllerC05tableE0So07UITableE4CellCSo0hE0C_10Foundation9IndexPathV12cellForRowAttFTo + 92
	9   UIKit                               0x00000001059ee567 -[UITableView _createPreparedCellForGlobalRow:withIndexPath:willDisplay:] + 783
	10  UIKit                               0x00000001059eeae4 -[UITableView _createPreparedCellForGlobalRow:willDisplay:] + 74
	11  UIKit                               0x00000001059b5eaa -[UITableView _updateVisibleCellsNow:isRecursive:] + 3168
	12  UIKit                               0x00000001059d67e0 -[UITableView layoutSubviews] + 176
	13  UIKit                               0x00000001059607a8 -[UIView(CALayerDelegate) layoutSublayersOfLayer:] + 1515
	14  QuartzCore                          0x0000000110b81456 -[CALayer layoutSublayers] + 177
	15  QuartzCore                          0x0000000110b85667 _ZN2CA5Layer16layout_if_neededEPNS_11TransactionE + 395
	16  QuartzCore                          0x0000000110b0c0fb _ZN2CA7Context18commit_transactionEPNS_11TransactionE + 343
	17  QuartzCore                          0x0000000110b3979c _ZN2CA11Transaction6commitEv + 568
	18  UIKit                               0x00000001058b9f2c _afterCACommitHandler + 272
	19  CoreFoundation                      0x00000001099ab607 __CFRUNLOOP_IS_CALLING_OUT_TO_AN_OBSERVER_CALLBACK_FUNCTION__ + 23
	20  CoreFoundation                      0x00000001099ab55e __CFRunLoopDoObservers + 430
	21  CoreFoundation                      0x000000010998fb81 __CFRunLoopRun + 1537
	22  CoreFoundation                      0x000000010998f30b CFRunLoopRunSpecific + 635
	23  GraphicsServices                    0x000000010f68aa73 GSEventRunModal + 62
	24  UIKit                               0x0000000105891057 UIApplicationMain + 159
	25  WooCommerce                         0x0000000104489387 main + 55
	26  libdyld.dylib                       0x000000010b111955 start + 1
	27  ???                                 0x0000000000000001 0x0 + 1
)
libc++abi.dylib: terminating with uncaught exception of type NSException
(lldb)

jleandroperez · 2018-06-25T17:26:17Z

Podfile

@@ -1,4 +1,4 @@
-source 'https://github.com/CocoaPods/Specs.git'
+


Mind nuking the two empty spaces at the top?

jleandroperez · 2018-06-25T17:28:38Z

WooCommerce/Classes/ViewModels/OrderNoteViewModel.swift

+            statusText = NSLocalizedString("Private note", comment: "Labels an order note to let the user know it's private and not seen by the customer")
+        }
+
+        // "date_created": "2017-03-21T16:46:41",


Can we nuke this one?

jleandroperez · 2018-06-25T17:29:12Z

WooCommerce/Classes/ViewModels/OrderNoteViewModel.swift

+
+        // "date_created": "2017-03-21T16:46:41",
+        let format = DateFormatter.Defaults.dateTimeFormatter
+        date = format.date(from: orderNote.dateCreated)!


It's probably a bad idea to use an Implicitly Unwrapped Optional here. Whenever there's a parsing issue, the app will crash.

Can we turn the date field into an Optional instead?

jleandroperez · 2018-06-25T17:29:22Z

WooCommerce/Classes/ViewModels/OrderNoteViewModel.swift

+        formatter.timeStyle = .short
+
+        let formattedDateString = formatter.string(from: date)
+        return formattedDateString


return formatter.string(from: date)

jleandroperez · 2018-06-25T17:31:59Z

WooCommerce/Classes/ViewRelated/Orders/OrdersViewController.swift

+            } catch {
+                print("error:\(error)")
+            }
+        }


No need to change this, since it's testing code, and it's going away supersoon!. But in general, you may wanna avoid nested levels.

A simple way to express this, in a "flattened fashion" would be:

guard let path = Bundle.main.url(forResource: resource, withExtension: "json") else { return basicOrder } do { let json = try Data(contentsOf: path) let decoder = JSONDecoder() let orderNotesFromJson = try decoder.decode([OrderNote].self, from: json) var order = basicOrder order.notes = orderNotesFromJson return order } catch { DDLogError("error:\(error)") }

(The lesser indentation levels, the better, since it really aids readability)

…), remove extra lines in podfile, make dateCreated an optional

jleandroperez

All clear!!!

mindgraffiti · 2018-06-25T20:03:55Z

Thanks Jorge!

mindgraffiti added 11 commits May 25, 2018 10:23

Add reusable item tableview cell

8389f4a

Remove unused variable

9ee795e

Load order notes if available and return order. Return basic order if…

c093447

… error or order notes not available

Add order notes for Order #1044

4243591

Add OrderNotesViewModel

a8cfce5

Create OrderNote table view cell, add to order details view controller

09b6f77

Merge branch 'develop' into issue/13-order-notes

4782b70

Update with credentials

ce068b8

Merge branch 'develop' into issue/13-order-notes

13734e1

Update app credentials after changes from develop

619d881

Date formatting for order note

1194987

mindgraffiti added the feature: order details Related to order details. label May 29, 2018

mindgraffiti added this to the External closed beta group milestone May 29, 2018

mindgraffiti self-assigned this May 29, 2018

mindgraffiti added this to MVLP Backlog in MVLP Kanban Board via automation May 29, 2018

houndci-bot reviewed May 29, 2018

View reviewed changes

mindgraffiti commented May 29, 2018

View reviewed changes

Delete unused methods and make hound happy. Woof!

89ae6de

mindgraffiti moved this from MVLP Backlog to Review/Testing in MVLP Kanban Board May 29, 2018

Merge branch 'develop' into issue/13-order-notes

3e95fc8

mindgraffiti requested a review from jleandroperez June 13, 2018 14:08

mindgraffiti added the type: enhancement A request for an enhancement. label Jun 13, 2018

mindgraffiti added 2 commits June 19, 2018 10:01

Merge branch 'develop' into issue/13-order-notes

a89a073

Fix merge conflicts

c20a51c

mindgraffiti requested a review from bummytime June 20, 2018 14:36

Fix merge conflicts in .pbxproj file

b4ef6f7

jleandroperez requested changes Jun 21, 2018

View reviewed changes

mindgraffiti added 2 commits June 21, 2018 14:24

Merge branch 'develop' into issue/13-order-notes

99a42f8

Delete generated files

6391572

mindgraffiti added 9 commits June 21, 2018 14:38

Fix file path for .gitignore'd resource

40c2e03

Swift-y method

68462b1

Revert "Merge branch 'develop' into issue/13-order-notes"

3c8140b

This reverts commit 99a42f8.

Code changes review

a300af8

Update CocoaPods to 1.5.3

15b3f28

Merge branch 'develop' into issue/13-order-notes

0deedc5

Remove duplicate dependencies warning

f326c03

Remove unknown file in xcode .proj file

60a2ae4

Add changes to podfile.lock

468ba14

mindgraffiti added 7 commits June 25, 2018 08:52

Buddybuild wants a shared scheme for Yosemite

334250b

Buddybuild wants all items as shared schemes

e34e1d4

Revert "Revert "Merge branch 'develop' into issue/13-order-notes""

99f8f5e

This reverts commit 3c8140b.

Fix merge conflicts

287c5ff

Merge pull request #111 from woocommerce/add-fabric

cfb4b67

Adds initial Fabric / Crashlytics integration

Merge pull request #112 from woocommerce/issue/fixing-breaking-tests

d1e0a7e

YosemiteTests: Fixing Runtime Errors

Merge conflicts - podfile checksum

87b4f33

jleandroperez requested changes Jun 25, 2018

View reviewed changes

jleandroperez changed the title ~~Issue/13 order notes~~ Order: Displaying Notes Jun 25, 2018

mindgraffiti added 3 commits June 25, 2018 13:47

Add target back to AddItemTableViewCell (erased during merge conflict…

003a5dc

…), remove extra lines in podfile, make dateCreated an optional

Flatten method - remove nested levels of code

0a175a5

Return the default JSON date string if the date formatting fails

c07e966

jleandroperez approved these changes Jun 25, 2018

View reviewed changes

mindgraffiti merged commit bdc4df6 into develop Jun 25, 2018

MVLP Kanban Board automation moved this from Review/Testing to Done/Merged Jun 25, 2018

mindgraffiti deleted the issue/13-order-notes branch June 25, 2018 20:01

astralbodies mentioned this pull request Jun 27, 2018

Order Details #21

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Order: Displaying Notes #73

Order: Displaying Notes #73

mindgraffiti commented May 29, 2018

houndci-bot May 29, 2018

houndci-bot May 29, 2018

mindgraffiti May 29, 2018

mindgraffiti May 29, 2018

jleandroperez Jun 18, 2018

jleandroperez left a comment

jleandroperez Jun 18, 2018

jleandroperez Jun 18, 2018

jleandroperez Jun 21, 2018

jleandroperez Jun 21, 2018

jleandroperez Jun 21, 2018

mindgraffiti Jun 21, 2018

mindgraffiti commented Jun 25, 2018 •

edited

jleandroperez left a comment

jleandroperez Jun 25, 2018

jleandroperez Jun 25, 2018

jleandroperez Jun 25, 2018

jleandroperez Jun 25, 2018

jleandroperez Jun 25, 2018

jleandroperez left a comment

mindgraffiti commented Jun 25, 2018

		@@ -0,0 +1,32 @@
		/// WooCommerce API Credentials. Generated on May 29, 2018 at 11:16:32

		@@ -1,4 +1,4 @@
		source 'https://github.com/CocoaPods/Specs.git'

Order: Displaying Notes #73

Order: Displaying Notes #73

Conversation

mindgraffiti commented May 29, 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

jleandroperez left a 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

mindgraffiti commented Jun 25, 2018 • edited

jleandroperez left a 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

jleandroperez left a comment

Choose a reason for hiding this comment

mindgraffiti commented Jun 25, 2018

mindgraffiti commented Jun 25, 2018 •

edited