Last Call Review of draft-ietf-jmap-quotas-07
review-ietf-jmap-quotas-07-artart-lc-tiloca-2022-11-13-00
Request | Review of | draft-ietf-jmap-quotas |
---|---|---|
Requested revision | No specific revision (document currently at 12) | |
Type | Last Call Review | |
Team | ART Area Review Team (artart) | |
Deadline | 2022-11-19 | |
Requested | 2022-11-05 | |
Authors | René Cordier | |
I-D last updated | 2022-11-13 | |
Completed reviews |
Secdir Last Call review of -07
by Wes Hardaker
(diff)
Artart Last Call review of -07 by Marco Tiloca (diff) Genart Last Call review of -10 by Thomas Fossati (diff) Secdir Telechat review of -10 by Wes Hardaker (diff) |
|
Assignment | Reviewer | Marco Tiloca |
State | Completed | |
Request | Last Call review on draft-ietf-jmap-quotas by ART Area Review Team Assigned | |
Posted at | https://mailarchive.ietf.org/arch/msg/art/TpJMnbZaxqTpo5bUBFPLlrXV914 | |
Reviewed revision | 07 (document currently at 12) | |
Result | Ready w/issues | |
Completed | 2022-11-13 |
review-ietf-jmap-quotas-07-artart-lc-tiloca-2022-11-13-00
Hi all, Please see my comments below. Best, /Marco -------------------------------- [General] * In order to improve readability, I suggest to consider a slightly different organization of sections, as below. 1. Introduction 1.1 Notational conventions 1.2 Terminology 2. Addition to the capabilities object 2.1 urn:ietf:params:jmap:quota 3. Data types 3.1 Scope 3.2 ResourceType 4 Quota 4.1 Quota/get 4.2 Quota/changes 4.3 Quota/query 4.4 Quota/queryChanges 5 Examples 5.1 Fetching quotas 5.2 Requesting latest quota changes 6 Security considerations 7 IANA considerations 7.1 JMAP Capability Registration for "quota" 8 Normative References Note that the following comments consider the section numbering as in the current version -07. [Section 1.1] * "Servers MUST support all properties specified for the new data types defined in this document." I would not expect a normative statement at this point, where the document is still providing context and terminology. This sentence better fits in the current Section 1.4 "Data types". [Section 1.2] * "The same terminology is used in this document as in the core JMAP specification." Please refer again to RFC 8620. * "The term Quota (with that specific capitalization) is used to refer to the data type defined in this document and instance of that data type." Please refer to Section 2 "Quota". [Section 1.5] * "Servers MUST support the JMAP push mechanisms, as specified in [RFC8620] Section 7, to receive notifications when the state changes for the Quota type defined in this specification." This sentence (i.e., the whole content of Section 1.5) better fits in the current Section 2 "Quota", e.g., after the bullet list with the fields of the quota object. [Section 2] * "It should respect the JMAP ID data type defined in section 1.2 of [RFC8620]" Do you mean "It SHOULD respect ..." ? Regardless, why not "It MUST respect ..." ? * "resourceType*: "ResourceType" The resource type of the quota." Please add a pointer to the current Section 1.4.2. * "*used*: "UnsignedInt" The current usage of the defined quota. Computation of this value is handled by the server." What is this actually counting, i.e., what is the unit of measurement? I suppose that this has to be unambiguously understood from the "resourceType". Correct? * "*limit*: ... It should be higher than the warnLimit and the softLimit." Do you mean "It SHOULD be higher ..." ? Also, it's better to conclude the sentence with "(if present and different than null)", since "warnLimit" and "softLimit" are optional to include. * "*limit*: ... Objects in scope may not be created or updated if we reach this limit." Who is "we"? Perhaps rephrase as impersonal, i.e., "if this limit is reached", thus abstracting away from the entity breaching the limit being the client or the server. * "*scope*: "Scope" The Scope of this quota." Please add a pointer to the current Section 1.4.1. * "*warnLimit*: ... If set, it should be lower than the softLimit and the limit." Do you mean "If set, it SHOULD be lower than the softLimit (if present and different than null) and the limit" ? * "*softLimit*: ... If set, it should be higher than the warnLimit but lower than the limit." Do you mean "If set, it SHOULD be higher than the warnLimit (if present and different than null) but lower than the limit" ? * At the end of the current Section 2.0, please add a paragraph introducing the following list of methods presented in the current Sections 2.1-2.4. [Section 2.1] * "The _ids_ argument may be null to fetch all at once." All what? All the Quota instances at the server? * Please provide a pointer to the example in the current Section 2.5.1. [Section 2.2] * I don't quite understand the semantics of "updatedProperties". - What is the "old state"? Do you mean the latest list of properties provided to the client during the immediately previous interaction? - The value will be different than null in case only the "used" Quota properties have changed. But that means that no other properties have changed (though they may have in general), hence the value will always be the empty array of strings. Perhaps do you mean the following? "If the "used" Quota properties have changed since the old state, this will be the list of properties that have also changed." [Section 2.4] * Please provide a pointer to the example in the current Section 2.5.2. [Section 2.5.2] * Shouldn't the response also include the argument "updateProperties" defined in Section 2.2? [Section 3] * "that extension" Do you mean "the extension specified in this document" ? [Nits] * In the document header, s/JMAP/JMAP Working Group * Section 1, s/– (U+2013) JSON/– JSON * Section 1.3.1, s/account' (U+2019)s/account's * Section 1.4.2, s/like an unit/like a unit * Section 2, s/Might be used/It might be used * Section 2.1, s/ Standard “ (U+201C)/get” (U+201D) / Standard "/get" * Section 2.2 --- s/ Standard “ (U+201C)/changes” (U+201D) / Standard "/changes" --- s/argument to the response/argument in the response --- s/ " (U+201C)used" (U+201D) Quota / "used" Quota --- s/properties has changed/properties have changed --- s/it MUST just be null./it MUST be null. * Section 2.3, s/ Standard “ (U+201C)/query” (U+201D) / Standard "/query" * Section 2.4, s/ Standard “ (U+201C)/queryChanges” (U+201D) / Standard "/queryChanges" * Section 3, s/could spam himself/could spam themselves * Section 4.1, s/this document, section 4./this document, section 3.