|
| 1 | +# Code Review Checklist |
| 2 | + |
| 3 | +## Breaking Changes Checklist |
| 4 | + |
| 5 | +```markdown |
| 6 | +## Breaking Changes Review |
| 7 | +- [ ] No public method signatures removed or changed without [Obsolete] deprecation |
| 8 | +- [ ] No [JsonProperty] values changed (would break consumer deserialization silently) |
| 9 | +- [ ] No ContentstackOptions public property removed |
| 10 | +- [ ] New required options have defaults (don't break existing consumers who don't set them) |
| 11 | +- [ ] No namespace renames without backward-compatible type aliases |
| 12 | +- [ ] No IContentstackPlugin interface signature changed |
| 13 | +- [ ] Version bump planned if breaking change is intentional (Directory.Build.props) |
| 14 | +``` |
| 15 | + |
| 16 | +## HTTP Layer Checklist |
| 17 | + |
| 18 | +```markdown |
| 19 | +## HTTP Layer Review |
| 20 | +- [ ] All HTTP calls route through HttpRequestHandler.ProcessRequest |
| 21 | +- [ ] No HttpClient instantiation anywhere in the PR |
| 22 | +- [ ] New query params added to UrlQueries dict (not directly to URL string) |
| 23 | +- [ ] New field-level filters added to QueryValueJson dict |
| 24 | +- [ ] New headers added via Headers parameter to ProcessRequest |
| 25 | +- [ ] Branch header uses "branch" key, passed as separate Branch parameter |
| 26 | +- [ ] No hardcoded URLs — BaseUrl comes from Config.BaseUrl |
| 27 | +- [ ] Live preview URL resolved via Config.getBaseUrl() — not hardcoded |
| 28 | +- [ ] ProcessRequest result (string JSON) parsed, not further HTTP calls made |
| 29 | +``` |
| 30 | + |
| 31 | +## Exception Handling Checklist |
| 32 | + |
| 33 | +```markdown |
| 34 | +## Exception Handling Review |
| 35 | +- [ ] Domain-specific exception type used (QueryFilterException, AssetException, etc.) |
| 36 | +- [ ] No bare `throw new Exception(...)` or `throw new ContentstackException(...)` |
| 37 | +- [ ] All message strings sourced from ErrorMessages.cs constants |
| 38 | +- [ ] No string literals in throw statements |
| 39 | +- [ ] GetContentstackError(ex) called when catching WebException from HTTP calls |
| 40 | +- [ ] ErrorCode, StatusCode, Errors preserved when re-wrapping exceptions |
| 41 | +- [ ] New domain area has new exception class with factory methods |
| 42 | +- [ ] New error messages added to correct section in ErrorMessages.cs |
| 43 | +- [ ] FormatExceptionDetails(innerEx) used in ProcessingError factory methods |
| 44 | +``` |
| 45 | + |
| 46 | +## Serialization Checklist |
| 47 | + |
| 48 | +```markdown |
| 49 | +## Serialization Review |
| 50 | +- [ ] All public properties mapping CDA JSON fields have [JsonProperty("snake_case")] |
| 51 | +- [ ] No reliance on default Newtonsoft.Json camelCase or PascalCase matching |
| 52 | +- [ ] Custom deserialization uses [CSJsonConverter] + JsonConverter subclass |
| 53 | +- [ ] JsonConverter placed in Contentstack.Core/Internals/ (internal class) |
| 54 | +- [ ] No System.Text.Json usage |
| 55 | +- [ ] No JsonConvert.DeserializeObject with hardcoded type outside of converter |
| 56 | +- [ ] ContentstackCollection<T> used for list responses (not List<T> directly) |
| 57 | +- [ ] "entries" token used for entry collection, "assets" for asset collection |
| 58 | +``` |
| 59 | + |
| 60 | +## Fluent API Checklist |
| 61 | + |
| 62 | +```markdown |
| 63 | +## Fluent API Review |
| 64 | +- [ ] Every Query filter/operator method returns `return this;` |
| 65 | +- [ ] Null key validated at start of method → QueryFilterException.Create() |
| 66 | +- [ ] Empty string key validated → QueryFilterException.Create() |
| 67 | +- [ ] Operator value stored in QueryValueJson[key][$operator] nested dict |
| 68 | +- [ ] URL-level params stored in UrlQueries[key] |
| 69 | +- [ ] Method name follows verb+noun pattern (GreaterThan, ContainedIn, NotExists) |
| 70 | +- [ ] No mutation of QueryValueJson or UrlQueries outside of the Query class itself |
| 71 | +- [ ] And()/Or() accept Query[] (not raw dictionaries) |
| 72 | +``` |
| 73 | + |
| 74 | +## Configuration Checklist |
| 75 | + |
| 76 | +```markdown |
| 77 | +## Configuration Review |
| 78 | +- [ ] New options added to ContentstackOptions (public class), not Config (internal) |
| 79 | +- [ ] New property has XML <summary> doc comment |
| 80 | +- [ ] Default value set in ContentstackOptions() constructor or property initializer |
| 81 | +- [ ] ContentstackClient constructor reads new option and passes to Config |
| 82 | +- [ ] Config never exposed as public property |
| 83 | +- [ ] New option tested in ContentstackOptionsUnitTests.cs |
| 84 | +- [ ] ASP.NET Core binding works (IOptions<ContentstackOptions> path verified) |
| 85 | +``` |
| 86 | + |
| 87 | +## Test Coverage Checklist |
| 88 | + |
| 89 | +```markdown |
| 90 | +## Test Coverage Review |
| 91 | +- [ ] Unit test for each new public Query method (QueryValueJson assertion via reflection) |
| 92 | +- [ ] Unit test for null key input → QueryFilterException |
| 93 | +- [ ] Unit test for empty key input → QueryFilterException |
| 94 | +- [ ] Unit test for fluent return (Assert.Equal(query, result)) |
| 95 | +- [ ] Integration test file in Integration/{FeatureName}Tests/ subfolder |
| 96 | +- [ ] Integration test class extends IntegrationTestBase |
| 97 | +- [ ] Integration test constructor takes ITestOutputHelper output |
| 98 | +- [ ] CreateClient() used (not manual ContentstackClient construction) |
| 99 | +- [ ] LogArrange/LogAct/LogAssert used in correct order |
| 100 | +- [ ] TestAssert.* used (not raw Assert.*) |
| 101 | +- [ ] [Fact(DisplayName = "FeatureArea - Component Description")] present |
| 102 | +- [ ] Happy path test (valid params → expected response) |
| 103 | +- [ ] Error path test (invalid params or not found → expected exception) |
| 104 | +``` |
| 105 | + |
| 106 | +## Multi-Targeting Checklist |
| 107 | + |
| 108 | +```markdown |
| 109 | +## Multi-Targeting Review |
| 110 | +- [ ] No HttpClient (netstandard2.0 HttpClient has behavioural differences from net4x) |
| 111 | +- [ ] No System.Text.Json (not available without separate package in netstandard2.0) |
| 112 | +- [ ] No record types (C# 9, requires LangVersion setting for net47/net472) |
| 113 | +- [ ] No default interface implementations (C# 8, may affect net47) |
| 114 | +- [ ] No nullable reference types without #nullable enable guard |
| 115 | +- [ ] No top-level statements (not applicable to library projects but worth checking) |
| 116 | +- [ ] Tested compile against netstandard2.0 target (or verified via CI) |
| 117 | +``` |
| 118 | + |
| 119 | +## Plugin Lifecycle Checklist |
| 120 | + |
| 121 | +```markdown |
| 122 | +## Plugin Lifecycle Review |
| 123 | +- [ ] New feature that makes HTTP calls uses HttpRequestHandler (plugins run automatically) |
| 124 | +- [ ] No WebRequest.Create() called directly in new model classes |
| 125 | +- [ ] IContentstackPlugin interface not modified (breaking for all plugin consumers) |
| 126 | +- [ ] RequestLoggingPlugin still works with any new request/response changes |
| 127 | +- [ ] Plugin.OnRequest receives HttpWebRequest before send |
| 128 | +- [ ] Plugin.OnResponse receives response string (can mutate/inspect) |
| 129 | +``` |
| 130 | + |
| 131 | +## Internal Visibility Checklist |
| 132 | + |
| 133 | +```markdown |
| 134 | +## Internal Visibility Review |
| 135 | +- [ ] New utility/helper classes in Internals/ are marked `internal` |
| 136 | +- [ ] New model types intended for consumers are in Models/ and `public` |
| 137 | +- [ ] New configuration types are in Configuration/ and `public` |
| 138 | +- [ ] No public exposure of Config, HttpRequestHandler, or VersionUtility |
| 139 | +- [ ] InternalsVisibleTo not modified (already covers both test projects) |
| 140 | +- [ ] New internal methods accessible in unit tests without changes |
| 141 | +``` |
| 142 | + |
| 143 | +## Common Issues Found in Past PRs |
| 144 | + |
| 145 | +### Silent Deserialization Failures |
| 146 | +`[JsonProperty]` omitted → field is always null at runtime, no exception. Verify all properties that map CDA JSON fields. |
| 147 | + |
| 148 | +### Exception Message in Throw |
| 149 | +```csharp |
| 150 | +// Bad |
| 151 | +throw new QueryFilterException("Please provide valid params."); |
| 152 | + |
| 153 | +// Good |
| 154 | +throw QueryFilterException.Create(innerEx); |
| 155 | +// or |
| 156 | +throw new QueryFilterException(ErrorMessages.QueryFilterError); |
| 157 | +``` |
| 158 | + |
| 159 | +### Hardcoded Environment |
| 160 | +```csharp |
| 161 | +// Bad — breaks for consumers with different environments |
| 162 | +mainJson["environment"] = "production"; |
| 163 | + |
| 164 | +// Correct — already done in Exec() |
| 165 | +mainJson["environment"] = ContentTypeInstance.StackInstance.Config.Environment; |
| 166 | +``` |
| 167 | + |
| 168 | +### Returning void from Query Method |
| 169 | +```csharp |
| 170 | +// Bad — breaks fluent chaining |
| 171 | +public void SetMyParam(string value) { UrlQueries["my_param"] = value; } |
| 172 | + |
| 173 | +// Good |
| 174 | +public Query SetMyParam(string value) { UrlQueries["my_param"] = value; return this; } |
| 175 | +``` |
| 176 | + |
| 177 | +### Dictionary Not Initialized for QueryValueJson Entry |
| 178 | +```csharp |
| 179 | +// Bad — throws KeyNotFoundException or InvalidCastException |
| 180 | +((Dictionary<string, object>)QueryValueJson[key])["$op"] = value; |
| 181 | + |
| 182 | +// Good — guard with ContainsKey |
| 183 | +if (!QueryValueJson.ContainsKey(key)) |
| 184 | + QueryValueJson[key] = new Dictionary<string, object>(); |
| 185 | +((Dictionary<string, object>)QueryValueJson[key])["$op"] = value; |
| 186 | +``` |
0 commit comments