Closed Bug 1038275 Opened 11 years ago Closed 10 years ago

Comprehensible documentation for the REST API

Categories

(Bugzilla :: WebService, defect, P1)

Product:
Bugzilla
Component:
WebService
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Bugzilla 5.0

People

(Reporter: mcote, Assigned: dkl)

References

(Blocks 1 open bug)

Details

(Keywords: bmo-goal)

Attachments

(2 files, 7 obsolete files)

5.0 1038275-5.0_1.patch
176.73 KB, patch
gerv
: review+
Details | Diff | Splinter Review
Trunk 1038275_8.patch
177.71 KB, patch
gerv
: review+
Details | Diff | Splinter Review
Our WebService docs are not the best. They are weirdly arranged, tangled in with internal object docs, and are targetted more for the older XML/JSON RPC APIs instead of REST. Since most people starting a new project would use REST, and since at some point we'll be deprecating XMLRPC and JSONRPC, we should write docs specific to REST in a nice format, perhaps tied to the user docs on readthedocs, or perhaps with some sort of shiny framework that allows you to try the APIs out right in the documentation (e.g. Swagger). Bonus for both. :)
Priority: -- → P1
Here is some notes from a recent BMO team work week that outlines a proposal for the new documentation structure. Please take a look and comment as much as you can. = api documentation = * need - api reference - tutorials/examples - faqs (?) * remove existing perldoc docs * use rst * ensure the self-hosting is possible * makedocs.pl can convert the rst to html/pdf/epub for self-hosting * readthedocs.org can checkout the code from git/bzr and host the docs * Standard extension documentation (not API related) will display in the Extensions section of the upstream docs. * API docs will be contained in a separate section from the normal docs and will have the following structure: - API o Core . v1 . v2 . v3 o Extension Foo . v1 . v2 * Under each extension directory there will be a api/v2/docs/en directory which will contain the rst files for the API methods. - Example: extensions/Review/api/v2/docs/en/reviews.rst * docs/makedocs.pl will search the extension directories for the rst files and include them in the upstream API docs under their respective extension subsections.
Note that if you have makedocs.pl do (pretty much) anything, that thing will not be done on readthedocs.org. Which is why makedocs.pl now does very little apart from invoke sphinx. We invented a mechanism to include extension docs (in bug 963120), but extensions are excluded from the copy on readthedocs, because the mechanism relies on things done by makedocs.pl. Gerv
(In reply to Gervase Markham [:gerv] from comment #2) > Note that if you have makedocs.pl do (pretty much) anything, that thing will > not be done on readthedocs.org. Which is why makedocs.pl now does very > little apart from invoke sphinx. > > We invented a mechanism to include extension docs (in bug 963120), but > extensions are excluded from the copy on readthedocs, because the mechanism > relies on things done by makedocs.pl. > > Gerv well would it be sufficient that we require makedocs.pl to be ran that will pull all of the API docs from the extensions directories into the main docs/ directory and then commit the rst files to git. Then RTD will be able to pick it up right. I don't think that is an issue as each bugzilla instance will be hosting their own docs anyway. For example bmo.readthedocs.org would be a build of the docs dir from the bmo repo. The core API docs will reside in the standard docs dir and should still be built from the beginning along with the other core docs. dkl
(In reply to David Lawrence [:dkl] from comment #3) > I don't think that is an issue as each bugzilla instance > will be > hosting their own docs anyway. For example bmo.readthedocs.org would be a > build of the docs dir from the bmo repo. OK. So the idea is that we use makedocs.pl to do the copy, which means that if an install compiles their own docs, they'll get docs for any APIs implemented by extensions, but they won't see those if referencing the public docs? This would mean that we can't ship any extensions by default which add API, unless we decide to keep their docs in the core docs dir and not in the extensions/ dir. But perhaps that's OK. > The core API docs will reside in the standard docs dir and should still be > built from the beginning along with the other core docs. OK. Gerv
Attached patch 1038275_1.patch (obsolete) — — Splinter Review
I am attaching a WIP mostly complete version of the API documentation in rst format. I have transferred the relevant docs from the Perl POD to a new section in the current Bugzilla doc tree. Please take some time to look over the rst text itself as there may be ways to better layout the information, or rst directives that will make the doc more useful. I have also placed a current version of the docs pre-built on my dropbox site that you can look at for feedback purposes. http://dkl.site44.com/bugzilla/docs/en/html/index.html Some feedback already from glob which I will work on for the next release: the one suggestion i would make is it's hard to make out the parameter and response fields in their current bullet point format. i think it would be clearer to read if they were in a table. eg. https://dev.twitter.com/docs/api/1.1/get/statuses/mentions_timeline https://developers.google.com/youtube/v3/docs/videos
Attachment #8485003 - Flags: feedback?(mcote)
Attachment #8485003 - Flags: feedback?(glob)
Attachment #8485003 - Flags: feedback?(gerv)
Comment on attachment 8485003 [details] [diff] [review] 1038275_1.patch >+++ b/Bugzilla/Install/Requirements.pm >@@ -166,6 +166,11 @@ sub REQUIRED_MODULES { > module => 'File::Slurp', > version => '9999.13', > }, >+ { >+ package => 'File-Copy-Recursive', >+ module => 'File::Copy::Recursive', >+ version => 0 >+ } I see no reason to make this module mandatory. It's not required to make Bugzilla work. You only need it to compile the doc. So it should be optional or even moved somewhere else, similar to bug 124391.
Comment on attachment 8485003 [details] [diff] [review] 1038275_1.patch what sort of feedback are you after? do you want us to just focus on the presentation, or is there content in particular you'd like us to pay attention to (i'm not sure how much of this is a straight transfer from the existing docs, and how much is new content). so far this looks like a massive improvement over the current documentation, well done :) - parameters and result fields may be clearer to read/scan if presented in a table - i agree that File::Copy::Recursive should be optional (i don't think this is the place to fix the requirements code) - the example extension's documentation should be explicitly excluded from the generated docs - while they are useful to extension developers, it's confusing for an api consumer to have it present in our standard documentation
Comment on attachment 8485003 [details] [diff] [review] 1038275_1.patch Review of attachment 8485003 [details] [diff] [review]: ----------------------------------------------------------------- Bunch of little changes. I'm sure there are more little mistakes, but we can fix those incrementally over time. Also I'm sure lots of my suggestions are for text that exists in the current docs, but we might as well fix it now. Finally, there are some recurring errors and inconsistencies across all files. I've marked some of them below, usually with a "and see below" or similar, but one big one that stands out is that sometimes we use "id" and sometimes "ID". We should pick one to use everywhere. Another one is that parameter descriptions sometimes start with capitals and sometimes small letters. There's also some trailing whitespace here and there. ::: docs/en/rst/api/Core/v1/bug.rst @@ +3,5 @@ > + > +The REST API for creating, changing, and getting the details of bugs. > + > +This part of the Bugzilla REST API allows you to file a new bug in Bugzilla, > +or get information about bugs that have already been filed. "allows you to file new bugs in Bugzilla and to get information about existing bugs" @@ +13,5 @@ > + > +Get information about valid bug fields, including the lists of legal values > +for each field. > + > +To get information about all fields. "fields:" There are other instances of this below (not going to explicitly call them all out). @@ +19,5 @@ > +.. code-block:: text > + > + GET /rest/field/bug > + > +To get information related to a single field. Ditto. @@ +28,5 @@ > + > +Request parameters: > + > +Note: If neither ``ids`` nor ``names`` is specified, then all > +non-obsolete fields will be returned. This is a bit complicated, since you can, I assume, *either* specify (id_or_name) *or* ids=/names= on the query string. Probably worth clearing this up early on, maybe at the top of this doc, since I imagine it applies later on as well. This is especially important to clarify where it says things like "one of ids or comment_ids is required", since that's not true if you provide it as part of the URL. @@ +35,5 @@ > +* names: (array) An array of strings representing field names. > + > +Response data: > + > +* field: (array of objects) Each objects contains the following items: "object" @@ +77,5 @@ > + is an empty array. > +* value_field: (string) The name of the field that controls whether or not > + particular values of the field are shown in the user interface. Can be null. > +* values: (array of objects) Represents the legal values for select-type\ > + (drop-down and multiple-selection) fields. This is also populated for the What's with the \? @@ +80,5 @@ > +* values: (array of objects) Represents the legal values for select-type\ > + (drop-down and multiple-selection) fields. This is also populated for the > + ``component``, ``version``, ``target_milestone``, and ``keywords`` fields, but > + not for the ``product`` field (you must use ``get_accessible_products`` for > + that. Missing ) @@ +95,5 @@ > + value is only shown if the ``value_field`` is set to one of the values listed > + in this array. Note that for per-product fields, ``value_field`` is set to > + ``product`` and ``visibility_values`` will reflect which product(s) this value > + appears in. > +* is_active: (boolean) This value is defined only for certain product specific "product-specific" @@ +97,5 @@ > + ``product`` and ``visibility_values`` will reflect which product(s) this value > + appears in. > +* is_active: (boolean) This value is defined only for certain product specific > + fields such as version, target_milestone or component. When true, the value is > + active, otherwise the value is not active. "active; otherwise" @@ +103,5 @@ > + for the ``keywords`` field. > +* is_open: (boolean) For ``bug_status`` values, determines whether this status > + specifies that the bug is "open" (true) or "closed" (false). This item > + is only included for the ``bug_status`` field. > +* can_change_to: For ``bug_status`` values, this is an array of obnjects that "objects" @@ +151,5 @@ > + > +Attachments > +----------- > + > +It allows you to get data about attachments, given a list of bugs "This allows" @@ +182,5 @@ > + > +An object containing two elements: ``bugs`` and ``attachments``. The return > +value looks like this: > + > +Example response: Only need one of "looks like this" and "example response". @@ +279,5 @@ > + argument, even if they are older than this date. > + > +Returns data: > + > +Two items are returned: Get rid of "Returns data". @@ +442,5 @@ > + * Date/Time Fields: (datetime) > + > +User detail objects: > + > +Each user detail object contains the following items: Hm this double-header thing ("User detail objects", "Each user detail object contains the following items") seems to be recurring and should be fixed everywhere. @@ +524,5 @@ > +The same as :ref:`rest_single_bug`. > + > +Note that you will only be returned information about bugs that you can see. > +Bugs that you can't see will be entirely excluded from the results. So, if you > +want to see private bugs, you will have to first log in and *then* call this method. Can you pass username & password instead of logging in explicitly? @@ +571,5 @@ > +In addition to the fields listed below, you may also use criteria that > +is similar to what is used in the Advanced Search screen of the Bugzilla > +UI. This includes fields specified by ``Search by Change History`` and > +``Custom Search``. The easiest way to determine what the field names are and what > +format Bugzilla expects, is to first construct your query using the s/, // @@ +576,5 @@ > +Advanced Search UI, execute it and use the query parameters in they URL > +as your key/value pairs for the WebService call. With REST, you can > +just reuse the query parameter portion in the REST call itself. > + > +* alias: (array> of strings) The unique aliases of this bug. An empty array will s/>// @@ +586,5 @@ > + don't want this, be sure to also specify the ``product`` argument. > +* creation_time: (datetime) Searches for bugs that were created at this time or later. > + May not be an array. > +* creator: (string) The login name of the user who created the bug. You can also > + pass this argument with the name C<reporter>, for backwards compatibility with I think this is leftover formatting from previous docs. Several similar instances below. @@ +640,5 @@ > + > +Note that you will only be returned information about bugs that you > +can see. Bugs that you can't see will be entirely excluded from the > +results. So, if you want to see private bugs, you will have to first > +log in and *then* call this method. Again, you can provide username and password parameters right? @@ +750,5 @@ > + > + POST /rest/bug/(bug_id)/attachment > + > +The params to include in the POST body, as well as the returned > +data format are the same as below. The ``bug_id`` param will be "format," @@ +1168,5 @@ > + > +* ids: (array of ints or strings) The ids or aliases of bugs that you want > + to modify. > +* add: (array of string) URLs to Bugzilla bugs. These URLs will be added to > + the See Also field. They must be valid URLs to ``show_bug.cg`` in a show_bug.cgi @@ +1169,5 @@ > +* ids: (array of ints or strings) The ids or aliases of bugs that you want > + to modify. > +* add: (array of string) URLs to Bugzilla bugs. These URLs will be added to > + the See Also field. They must be valid URLs to ``show_bug.cg`` in a > + Bugzilla installation or to a bug filed at launchpad.net. launchpad.net? What's that about? ::: docs/en/rst/api/Core/v1/buguserlastvisit.rst @@ +13,5 @@ > +.. code-block:: text > + > + POST /rest/bug_user_last_visit/(id) > + > +Tp add one or more bug ids at once: "To" @@ +25,5 @@ > +* ids: (array of ints) One or more bug ids to add. > + > +Response data: > + > +An array of objects containing the items: As mentioned elsewhere, these double "headers" are weird. ::: docs/en/rst/api/Core/v1/bugzilla.rst @@ +17,5 @@ > +.. code-block:: http > + > + GET /rest/version HTTP/1.1 > + Host: bugzilla.example.com > + Accept: application/json Do we give full example requests like this elsewhere? I haven't seen them yet, so this looks a bit out of place. ::: docs/en/rst/api/Core/v1/flagtype.rst @@ +1,4 @@ > +Flag Type > +========= > + > +This part of the Bugzilla API allows you to create new flags Missing period. It also allows you to get them, not just create them, and flag types, not just flags. @@ +48,5 @@ > + > +Create > +------ > + > +Create a new Flag Type Missing period, unnecessary capitalization (and below). ::: docs/en/rst/api/Core/v1/general.rst @@ +1,5 @@ > +General > +======= > + > +This is the standard REST API for external programs that want to interact > +with Bugzilla. It provides various REST call to various Bugzilla functions. "It provides REST interfaces to various Bugzilla functions." @@ +11,5 @@ > + > +If the Accept: header of a request is set to text/html (as it is by an > +ordinary web browser) then the API will return the JSON data as a HTML > +page which the browser can display. In other words, you can play with the > +API using just your browser and see results in a human-readable form. "to see results" @@ +17,5 @@ > +it for POST or PUT. > + > +**Data Format** > + > +The REST API only supports JSON input, and either JSON and JSONP output. "JSON or JSONP" @@ +25,5 @@ > +headers to the MIME type of the data format you are using to communicate with > +the API. Content-Type tells the API how to interpret your request, and Accept > +tells it how you want your data back. "Content-Type" must be "application/json". > +"Accept" can be either that, or "application/javascript" for JSONP - add a > +"callback" parameter to name your callback. Change 'for JSONP - add a "callback" parameter to name your callback.' to 'for JSONP. In the latter case, add a "callback" parameter to name your callback.' @@ +44,5 @@ > + > +Common Data Types > +----------------- > + > +The Bugzilla API takes the following various types of parameters: s/takes/uses/ @@ +61,5 @@ > + > + In example code, you will see the characters ``[`` and ``]`` used to > + represent the beginning and end of arrays. > + > + In our example code in these API docs, an array that contains the numbers "In the example code in this API doc" @@ +81,5 @@ > + and a "vegetable" key whose value is "lettuce" would look like: > + > + .. code-block:: js > + > + { "fruit" : "oranges", "vegetable" : "lettuce" } I wonder whether it makes sense to essentially document JSON here. I think JSON is ubiquitous enough that you only need to document conventions, like base64 for binary data, and datetime format. @@ +84,5 @@ > + > + { "fruit" : "oranges", "vegetable" : "lettuce" } > + > +Logging In > +---------- This is heavily duplicated in user.rst. I would put it there with a link and *maybe* a short summary. @@ +88,5 @@ > +---------- > + > +Some methods do not require you to log in. An example of this is > +:ref:`rest_single_bug`. However, authenticating yourself allows you to see non > +public information. For example, a bug that is not publicly visible. "information, for example," @@ +95,5 @@ > + > +* API key: > + > + You can specify ``Bugzilla_api_key`` as an argument to any call, and > + you will be logged in as that user if the key is correct, and has not been "correct and has not" @@ +114,5 @@ > + > + The ``Bugzilla_restrictlogin`` option is only used when you have also > + specified ``Bugzilla_login`` and ``Bugzilla_password``. This value will be > + deprecated in the release after Bugzilla 5.0 and you will be required to > + pass the ``Bugzilla_login`` and ``Bugzilla_password`` for every call. Somehow API keys should be mentioned here, perhaps "you will be required to use API keys or pass ``Bugzilla_login`` and ``Bugzilla_password`` to every call." Also this is a bit confusing because it sort of presupposes the section on the deprecated /rest/login/ interface below. @@ +118,5 @@ > + pass the ``Bugzilla_login`` and ``Bugzilla_password`` for every call. > + > + .. tip:: > + You may also use the ``login`` and ``password`` variable > + names instead of ``Bugzilla_login`` and ``Bugzilla_password`` as a "parameters" or "arguments" instead of "variable names" I think. @@ +133,5 @@ > + call, and you will be logged in as that user if the token is correct. > + This is the token returned when calling :ref:`rest_user_login` mentioned > + above. > + > + An error is thrown if you pass an invalid token and you will need to log "token; you will need to" @@ +137,5 @@ > + An error is thrown if you pass an invalid token and you will need to log > + in again to get a new token. > + > + Token support was added in Bugzilla 5.0 and support for login cookies > + has been dropped for security reasons. This is kind of funny since login was dropped for the same reason. Not sure how to address this, though... maybe not worth mentioning that it was added in Bugzilla 5.0? @@ +144,5 @@ > +----------------- > + > +Many calls take similar arguments. Instead of re-writing the documentation > +for each call, we document the parameters here, once, and then refer back to this > +documentation from the individual calls where these parameters are used. I don't think you need to justify why the parameters are here. I think just "Many calls take common arguments. These are documented below and linked from the individual calls where these parameters are used." @@ +153,5 @@ > +example, :ref:`rest_single_bug` returns a list of ``bugs`` that have fields like > +``id``, ``summary``, ``creation_time``, etc.) > + > +These parameters allow you to limit what fields are present in the objects, to > +possibly improve performance or save some bandwidth. s/possibly//. The fact that there's an "or" here implies that these params don't always improve performance. @@ +184,5 @@ > + > + If you specify all the fields, then this function will return empty > + objects. > + > + Some calls support specifying sub fields. If an call states that I think "subfields" and "subfield" here and below. Also "If a call states that it supports" @@ +189,5 @@ > + it support sub field restrictions, you can restrict what information is > + returned within the first field. For example, if you call > + :ref:`rest_product_get` with an ``include_fields`` of ``components.name``, > + then only the component name would be returned (and nothing else). You can > + include the main field, and exclude a sub field. Since this section applies to both include_fields and exclude_fields, it should probably be below both those sections. @@ +220,5 @@ > +* _default: These fields are returned if ``include_fields`` is empty or > + ``_default`` is specified. All fields described in the documentation are > + returned by default unless specified otherwise. > +* _extra: These fields are not returned by default and need to be manually > + specified in ``include_fields`` either by field name, or using ``_extra``. This is still a bit vague as to what _extra means exactly. It is used to specify fields in addition to the defaults, rather than using include_fields to specify exactly which fields you want, right? ::: docs/en/rst/api/Core/v1/group.rst @@ +68,5 @@ > + of users who are in this group. > + > +Response data: > + > +* groups: (array of objects) Group changes, each group object contains the s/contains/containing/ @@ +86,5 @@ > + > +Get > +--- > + > +Returns information about a Bugzilla Groups. "about Bugzilla groups" @@ +103,5 @@ > + GET /rest/group?ids=1&ids=2&ids=3 or GET /group?names=ProductOne&names=Product2 > + > +Request parameters: > + > +If neither ids or names is passed, and you are in the creategroups or "ids nor names are passed" @@ +124,5 @@ > +"editusers" group or have bless privileges to the groups they require > +membership information for, the is_active, is_bug_group and user_regexp values > +are not supplied. > + > +The return value will be an object containing group names as the keys, each s/,/;/ @@ +125,5 @@ > +membership information for, the is_active, is_bug_group and user_regexp values > +are not supplied. > + > +The return value will be an object containing group names as the keys, each > +group name will be an object that describes the group and has the following items: "value" instead of "group name"? @@ +131,5 @@ > +* id: (int) The unique integer ID that Bugzilla uses to identify this group. > + Even if the name of the group changes, this ID will stay the same. > +* name: (string) The name of the group. > +* description: (string) The description of the group. > +* is_bug_group: (int) Whether this groups is to be used for bug reports or is "group" @@ +136,5 @@ > + only administrative specific. > +* user_regexp: (string) A regular expression that allows users to be added to > + this group if their login matches. > +* is_active: (int) Whether this group is currently active or not. > +* users: (array of objects) User objects that are members of this group, only s/,/;/ @@ +137,5 @@ > +* user_regexp: (string) A regular expression that allows users to be added to > + this group if their login matches. > +* is_active: (int) Whether this group is currently active or not. > +* users: (array of objects) User objects that are members of this group, only > + returned if the user sets the ``membership`` parameter to 1, each user object "to 1. Each user object" @@ +150,5 @@ > + bugzilla. > + * email_enabled: (boolean) A boolean value to indicate if bug-related mail will > + be sent to the user or not. > + * disabled_text: (string) A text field that holds the reason for disabling a user > + from logging into bugzilla, if empty then the user account is enabled otherwise "into Bugzilla. If empty, the user account is enabled; otherwise" ::: docs/en/rst/api/Core/v1/user.rst @@ +1,5 @@ > +User > +==== > + > +This part of the Bugzilla API allows you to create User Accounts and > +log in/out using an existing account. "in/out" -> "in or out" @@ +8,5 @@ > + > +Login > +----- > + > +Logging in, with a username and password, is required for many Commas are superfluous here. "Logging in with a username and password is required" @@ +10,5 @@ > +----- > + > +Logging in, with a username and password, is required for many > +Bugzilla installations, in order to search for bugs, post new bugs, > +etc. This method logs in an user. "a user" @@ +53,5 @@ > +----------- > + > +This method will verify whether a client's cookies or current login > +token is still valid or have expired. A valid username must be provided > +as well that matches. "A valid username that matches must be provided as well." @@ +62,5 @@ > + > +Request parameters: > + > +* login: (string) The login name that matches the provided cookies or token. > +* token: (string) Persistent login token current being used for authentication. "currently" @@ +76,5 @@ > + > +**NEEDS REST METHOD** > + > +Sends an email to the user, offering to create an account. The user > +will have to click on a URL in the email, and choose their password s/,// @@ +108,5 @@ > +* email: (required) (string) The email address for the new user. > +* full_name: (string) The user's full name. Will be set to empty if not specified. > +* password: (string) The password for the new user account, in plain text. It > + will be stripped of leading and trailing whitespace. If blank or not specified, > + the newly created account will exist in Bugzilla, but will not be allowed to s/,// @@ +143,5 @@ > +* password: (string) The password of the user. > +* email_enabled: (boolean) A boolean value to enable/disable sending bug-related > + mail to the user. > +* login_denied_text: (string) A text field that holds the reason for disabling a > + user from logging into bugzilla, if empty then the user account is enabled "into Bugzilla. If empty, then the user account is enabled; otherwise, it is ..." @@ +156,5 @@ > + should be removed from. > + * set: (array of ints or strings) An exact set of group ids and group names > + that the user should be a member of. NOTE: This does not remove groups from > + the user where the person making the change does not have the bless privilege > + for. "when the person making the change does not have the bless privilege for the group." @@ +159,5 @@ > + the user where the person making the change does not have the bless privilege > + for. > + > + If you specify ``set``, then ``add`` and ``remove`` will be ignored. A group > + in both the ``add`` and ``remov`` list will be added. Specifying a group that "remove" @@ +162,5 @@ > + If you specify ``set``, then ``add`` and ``remove`` will be ignored. A group > + in both the ``add`` and ``remov`` list will be added. Specifying a group that > + the user making the change does not have bless rights will generate an error. > + > +* bless_groups: (object) This is the same as groups, but affects what groups a s/,// @@ +219,5 @@ > + contains any one of the specified strings. Users that you cannot see will > + not be included in the returned list. > + > + Most installations have a limit on how many matches are returned for > + each string, which defaults to 1000 but can be changed by the Bugzilla "each string; the default is 1000 but ..." @@ +227,5 @@ > + if they try. (This is to make it harder for spammers to harvest email > + addresses from Bugzilla, and also to enforce the user visibility > + restrictions that are implemented on some Bugzillas.) > + > +* limit: (int) Limit the number of users matched by the C<match> parameter. If Old markup (and below). @@ +228,5 @@ > + addresses from Bugzilla, and also to enforce the user visibility > + restrictions that are implemented on some Bugzillas.) > + > +* limit: (int) Limit the number of users matched by the C<match> parameter. If > + value is greater than the system limit, the system limit will be used. This "the value" @@ +230,5 @@ > + > +* limit: (int) Limit the number of users matched by the C<match> parameter. If > + value is greater than the system limit, the system limit will be used. This > + parameter is only used when user matching using the C<match> parameter is > + being performed. Not sure what "being performed" here means. The whole sentence is weird. @@ +243,5 @@ > + username doesn't fully match the input string. > + > +Response data: > + > +* users: (array of objects) Each object describes a user, and has the following s/,// @@ +257,5 @@ > + bugzilla. > +* email_enabled: (boolean) A boolean value to indicate if bug-related mail will > + be sent to the user or not. > +* login_denied_text: (string) A text field that holds the reason for disabling a > + user from logging into bugzilla, if empty then the user account is enabled. s/,/;/
Attachment #8485003 - Flags: feedback?(mcote) → feedback-
Attachment #8485003 - Flags: feedback?(glob)
Current WIP. Still working through mcotes punch items. But please take a look. Most of the request/response params have been converted to tables and some other changes. Will do a proper patch monday. http://dkl.site44.com/bugzilla/docs/en/html/index.html dkl
> ::: docs/en/rst/api/Core/v1/buguserlastvisit.rst > @@ +13,5 @@ > > +.. code-block:: text > > + > > + POST /rest/bug_user_last_visit/(id) > > + > > +Tp add one or more bug ids at once: > > "To" Not yet fixed. > ::: docs/en/rst/api/Core/v1/flagtype.rst > @@ +1,4 @@ > > +Flag Type > > +========= > > + > > +This part of the Bugzilla API allows you to create new flags > > Missing period. It also allows you to get them, not just create them, and > flag types, not just flags. Not yet fixed. > ::: docs/en/rst/api/Core/v1/general.rst > @@ +61,5 @@ > > + > > + In example code, you will see the characters ``[`` and ``]`` used to > > + represent the beginning and end of arrays. > > + > > + In our example code in these API docs, an array that contains the numbers > > "In the example code in this API doc" Reading this again, this is redundant; the two paragraphs should be merged into one (e.g. "... end of arrays. For example, an array that contains the numbers 1, 2, and 3 ..."). Though, as I mentioned in my comment about the section on objects, maybe the example is not necessary given how common JSON is nowadays. > @@ +114,5 @@ > > + > > + The ``Bugzilla_restrictlogin`` option is only used when you have also > > + specified ``Bugzilla_login`` and ``Bugzilla_password``. This value will be > > + deprecated in the release after Bugzilla 5.0 and you will be required to > > + pass the ``Bugzilla_login`` and ``Bugzilla_password`` for every call. > > Somehow API keys should be mentioned here, perhaps "you will be required to > use API keys or pass ``Bugzilla_login`` and ``Bugzilla_password`` to every > call." Also this is a bit confusing because it sort of presupposes the > section on the deprecated /rest/login/ interface below. This and all my subsequent comments on general.rst have not yet been fixed. > ::: docs/en/rst/api/Core/v1/group.rst > @@ +86,5 @@ > > + > > +Get > > +--- > > + > > +Returns information about a Bugzilla Groups. > > "about Bugzilla groups" This and all my subsequent comments on group.rst have not yet been fixed. No fixes applied to user.rst yet either. Also, I missed that the "Login" section in user.rst is not marked as deprecated. And maybe a link to "Logging in" in general.rst is a good idea, since those two sections have almost the same name and are somewhat related, and we can leave the main authentication docs in general.rst.
Attached patch 1038275_2.patch (obsolete) — — Splinter Review
New version with parameters converted to tables, mcotes suggested fixes implemented, and many other changes. Current version also up for viewing at: http://dkl.site44.com/bugzilla/docs/en/html/index.html dkl
Attachment #8485003 - Attachment is obsolete: true
Attachment #8485003 - Flags: feedback?(gerv)
Attachment #8496917 - Flags: review?(gerv)
Attachment #8496917 - Flags: review?(mcote)
Comment on attachment 8496917 [details] [diff] [review] 1038275_2.patch Review of attachment 8496917 [details] [diff] [review]: ----------------------------------------------------------------- A couple suggested changes to your changes below. You also continued to miss a few things; however, I'm giving it an r+ assuming you won't miss them this time, on commit. :) > ::: docs/en/rst/api/Core/v1/general.rst > @@ +133,5 @@ > > + call, and you will be logged in as that user if the token is correct. > > + This is the token returned when calling :ref:`rest_user_login` mentioned > > + above. > > + > > + An error is thrown if you pass an invalid token and you will need to log > > "token; you will need to" Still not fixed. > @@ +137,5 @@ > > + An error is thrown if you pass an invalid token and you will need to log > > + in again to get a new token. > > + > > + Token support was added in Bugzilla 5.0 and support for login cookies > > + has been dropped for security reasons. > > This is kind of funny since login was dropped for the same reason. Not sure > how to address this, though... maybe not worth mentioning that it was added > in Bugzilla 5.0? I think it's still worth mentioning that cookie support has been dropped; my comment was only about omitting the part about tokens being added in 5.0. > @@ +184,5 @@ > > + > > + If you specify all the fields, then this function will return empty > > + objects. > > + > > + Some calls support specifying sub fields. If an call states that > > I think "subfields" and "subfield" here and below. Also "If a call states > that it supports" Second part still not fixed. > ::: docs/en/rst/api/Core/v1/group.rst > @@ +103,5 @@ > > + GET /rest/group?ids=1&ids=2&ids=3 or GET /group?names=ProductOne&names=Product2 > > + > > +Request parameters: > > + > > +If neither ids or names is passed, and you are in the creategroups or > > "ids nor names are passed" Still says "is passed"; should be "are passed". > @@ +137,5 @@ > > +* user_regexp: (string) A regular expression that allows users to be added to > > + this group if their login matches. > > +* is_active: (int) Whether this group is currently active or not. > > +* users: (array of objects) User objects that are members of this group, only > > + returned if the user sets the ``membership`` parameter to 1, each user object > > "to 1. Each user object" You switched it to a semicolon, but that's two semicolons in one sentence, so as I say there should be a new sentence after "to 1". > ::: docs/en/rst/api/Core/v1/user.rst > @@ +10,5 @@ > > +----- > > + > > +Logging in, with a username and password, is required for many > > +Bugzilla installations, in order to search for bugs, post new bugs, > > +etc. This method logs in an user. > > "a user" Still not fixed. ::: docs/en/rst/api/Core/v1/general.rst @@ +83,5 @@ > +boolean True or false. > +base64 A base64-encoded string. This is the only way to transfer > + binary data via the API. > +array An array. There may be mixed types in an array. ``[`` and ``]`` are > + normally used to represent the beginning and end of arrays. Are arrays ever *not* represented with [ and ]? If not, remove "normally". @@ +86,5 @@ > +array An array. There may be mixed types in an array. ``[`` and ``]`` are > + normally used to represent the beginning and end of arrays. > +object A mapping of keys to values. Called a "hash", "dict", or "map" in > + some other programming languages. The keys are strings, and the > + values can be any type. ``{`` and ``}`` are normally used to represent Ditto.
Attachment #8496917 - Flags: review?(mcote) → review+
+++ b/docs/en/rst/extensions/Example/example.rst That's not supposed to be checked in, is it? (More review coming.) Gerv
Attached patch 1038275_3.patch (obsolete) — — Splinter Review
Fixed all (hopyfully) of mcotes list and also gervs mention of the extra files that should not have been in the patch.
Attachment #8496917 - Attachment is obsolete: true
Attachment #8496917 - Flags: review?(gerv)
Attachment #8498430 - Flags: review?(gerv)
Comment on attachment 8498430 [details] [diff] [review] 1038275_3.patch Moving forward mcote r+
Attachment #8498430 - Flags: review+
> Represented differently in different interfaces to this API. What does that mean in practice? Does the information given immediately after this sentence only refer to some interfaces, but not all? > True or false You should probably say ``True`` or ``False``, or similar. > [ and ] are normally used to represent the beginning and end of arrays. s/normally//, surely? > This value will be deprecated in the release after Bugzilla 5.0 and you will be required to pass the > Bugzilla_login and Bugzilla_password for or provide a valid Bugzilla_api_key value for every call. This is a non-sequitur. How does the deprecation of Bugzilla_restrictlogin lead to a requirement for username and password always to be passed? In other words, what's the connection between Bugzilla_restrictlogin and some unspecified login persistence mechanism? > GET /rest/login If this is that mechanism, it's not at all clear that it takes Bugzilla_login and Bugzilla_password parameters. > All fields described in the documentation are returned by default unless specified otherwise. Is that really true? The default for _default is that it's the same as _all? How does a reader know which fields are included in _extra? > Only custom fields are returned if _custom is specified in include_fields Surely not. Surely "Custom fields are _additionally_ returned if _custom etc."? > names array An array of strings representing field names. Do we do name conversion here, to hide the nastiness of some of our database field names? If we do, where does the user find a list of valid field names? Do we document that anywhere? > 1: Free Text > 10: Integer value I know these match the internal descriptions, but we should use better ones. "Free Text" -> "Single-Line String", "Integer Value" -> "Integer", that kind of thing. > Note: The bug_id value will be converted to ids internally. I don't know what that means or why it's important. Please clarify :-) > The id_or_alias value is converted to either ids in the list below. This isn't grammatical. Back to 7.1.1.4 for a moment: > Example request to Get: > > { "ids" : [1], "include_fields" : ["id", "name"] } How does that work? Get uses an HTTP GET, which IIRC doesn't have a body. Where does the JSON go? http://stackoverflow.com/questions/978061/http-get-with-request-body Do you mean URL parameters here? It's not actually clear from the docs how to pass params in (it's clear what format data comes out in). Relatedly, I think you overuse the "Note:" construct. If something is worth saying, it's usually worth saying in the body of the text itself. (I know the old Bugzilla docs were very guilty of this.) > If you want to return more than one bug, you can pass in multiple bug values to the Search Bugs > method. You mean "multiple bug IDs" You should define "bug ID" as meaning "bug ID or alias" somewhere near the beginning, so you don't have to keep repeating that caveat. This explanation: > If id is entirely numeric, it represents a bug_id from the Bugzilla database to fetch. If it contains > any non-numeric characters, it is considered to be a bug alias instead, and the data bug with that > alias will be loaded. is randomly included half way down the page. > NEEDS REST METHOD If it doesn't have one, don't document it. Once it has one, we can revive the documentation. The documentation is not a replacement for our bug tracker. > You will only be returned information about bugs that you can see. (etc.) This seems rather obvious; this needs to be explained in the intro stuff, if at all. > Product object: Would it make sense to document common objects in a <objectname>.inc.rst file, and use a ReST include to include them? That way, when an object grows a new field, we only have to update it in one place. > Booleans will be represented with the strings '1' and '0'. Ick. Something to fix in v2. :-) > The API for creating, changing, and getting information about Groups. > > This allows you to create a new group in Bugzilla. At the top of sections like this one, you should define the account permissions necessary to use the APIs documented. creategroups is mentioned on this page, but not until a long way down. > This method logs in an user. I thought we were deprecating and undocumenting this? If we aren't, you should link to it from the discussion of it in the introduction. Gerv
I reviewed v2; hope the comments are still useful. Gerv
(In reply to Gervase Markham [:gerv] from comment #17) > > NEEDS REST METHOD > > If it doesn't have one, don't document it. Once it has one, we can revive > the documentation. The documentation is not a replacement for our bug > tracker. I had told dkl to include this, but you're right: we should just file a bug (or multiple bugs) with all unRESTified APIs.
Attached patch 1038275_4.patch (obsolete) — — Splinter Review
Removed *NEEDS REST METHOD* sections for now and made note to file bugs for them to be added. dkl
Attachment #8498430 - Attachment is obsolete: true
Attachment #8498430 - Flags: review?(gerv)
Attachment #8498585 - Flags: review?(gerv)
dkl: did you address the other issues in my review yet? Gerv
(In reply to Gervase Markham [:gerv] from comment #21) > dkl: did you address the other issues in my review yet? > > Gerv Some were addressed by mcote's earlier review and would be in there. There is quite a bit that you have added that I am working on now with a new revision. Will hopefully get that up today. dkl
Comment on attachment 8498585 [details] [diff] [review] 1038275_4.patch This was an r-; eagerly awaiting the new version :-) Gerv
Attachment #8498585 - Flags: review?(gerv) → review-
Attached patch 1038275_5.patch (obsolete) — — Splinter Review
http://dkl.site44.com/bugzilla/docs/en/html/index.html
Attachment #8498585 - Attachment is obsolete: true
Attachment #8507008 - Flags: review?(gerv)
Comment on attachment 8507008 [details] [diff] [review] 1038275_5.patch Review of attachment 8507008 [details] [diff] [review]: ----------------------------------------------------------------- Looking pretty good... ::: docs/en/rst/api.rst @@ +7,5 @@ > +.. toctree:: > + :glob: > + > + api/Core/*/index* > + api/extensions/*/*/index* Doesn't this glob exclude filenames like "example.rst"? Is that intentional? ::: docs/en/rst/api/Core/v1/attachment.rst @@ +9,5 @@ > +-------------- > + > +This allows you to get data about attachments, given a list of bugs and/or > +attachment ids. Private attachments will only be returned if you are in the > +*insidergroup* or if you are the submitter of the attachment. The new docs have a set of roles for things like groups (:group:), parameters (:param:) and text in the UI (:guilabel:). Once they are checked in, we'll need to update these docs to use those. But I agree it doesn't make sense to do so now, and there's no need to do so pre-checkin even if the other new docs land before this. @@ +15,5 @@ > +**Request** > + > +To get all current attachments for a bug: > + > +.. code-block:: text There is an "http" Pygments lexer for HTTP sessions, but it doesn't seem to make much difference to this. @@ +17,5 @@ > +To get all current attachments for a bug: > + > +.. code-block:: text > + > + GET /rest/bug/(bug_id)/attachment This makes it look like you can only specify one bug_id you want attachments for, but below it talks about the "IDs you specify" in the plural. What's the story there? Same question for attachment_id. @@ +36,5 @@ > +================= ==== ====================================================== > + > +**Response** > + > +.. code-block:: js There is a JSON lexer (name: "json") in Pygments 1.5 and up. My Ubuntu has 1.6 installed, so it might be OK to depend on this. However, it doesn't seem to actually _highlight_ anything, so maybe not! @@ +75,5 @@ > +================ ======== ===================================================== > +data base64 The raw data of the attachment, encoded as Base64. > +size int The length (in bytes) of the attachment. > +creation_time datetime The time the attachment was created. > +last_change_time datetime The last time the attachment was modified. Does this include metadata? I assume so, but it's not clear. @@ +97,5 @@ > + Each flag object contains items descibed in the > + Flag object below. > +================ ======== ===================================================== > + > +Flag object: Surely you need to describe objects such as Flags more than once in the docs? To avoid repetition, could you put them in a separate file and include them? The new docs do this using: .. include:: installing-end.inc.rst and with exclude_patterns = ['**.inc.rst'] in the conf.py file. The only snag is that you can't put anchors in such a file, because the end up being duplicated and Sphinx complains. @@ +107,5 @@ > +name string The name of the flag. > +type_id int The type id of the flag. > +creation_date datetime The timestamp when this flag was originally created. > +modification_date datetime The timestamp when the flag was last modified. > +status string The current status of the flag. This is not a helpful description :-) What is a flag status? @@ +124,5 @@ > +This allows you to add an attachment to a bug in Bugzilla. > + > +**Request** > + > +To create attachment on a current bug. -> "To create attachment on a current bug:" @@ +165,5 @@ > + attachment to. The same attachment and comment will be > + added to all these bugs. > +**data** string The content of the attachment. If the content of the > + attachment is not ASCII text, you must encode it in > + base64 and declare it as the ``base64`` type. It doesn't say how to do this, and the example JSON above does not have a "type" member. @@ +167,5 @@ > +**data** string The content of the attachment. If the content of the > + attachment is not ASCII text, you must encode it in > + base64 and declare it as the ``base64`` type. > +**file_name** string The "file name" that will be displayed in the UI for > + this attachment. ...and which downloaded copies will be given. @@ +215,5 @@ > + > +==== ===== ========================= > +name type description > +==== ===== ========================= > +ids array Attachment id(s) created. If you attach the same attachment to multiple bugs, does the response tell you which new ID matches which bug? If not, should it? :-) @@ +227,5 @@ > +This allows you to update attachment metadata in Bugzilla. > + > +**Request** > + > +To update attachment metadata on a current attachmentr: Typo. @@ +341,5 @@ > + items: > + > + * added: (string) The values that were added to this > + field. Possibly a comma-and-space-separated list > + if multiple values were added. If the values contain commas and/or spaces, how are those escaped? ::: docs/en/rst/api/Core/v1/bug.rst @@ +14,5 @@ > +Gets information about particular bugs in the database. > + > +**Request** > + > +To get information about a particular bug using its id or alias: Are we using id or ID for a bug ID? "id" is already an English word; I think we should use ID. @@ +30,5 @@ > + > +================ ===== ====================================================== > +name type description > +================ ===== ====================================================== > +**id_or_alias** mixed An integer that can be a bug id or a bug alias. An integer can't be an alias. You mean: "An integer bug ID or string bug alias." @@ +31,5 @@ > +================ ===== ====================================================== > +name type description > +================ ===== ====================================================== > +**id_or_alias** mixed An integer that can be a bug id or a bug alias. > +ids array Array of integer bug ids. Not possible to specify multiple aliases? @@ +272,5 @@ > + requested to be granted or denied. Note, this field > + is only returned if a requestee is set. > +================= ======== ==================================================== > + > +Custom field object: Please check for trailing whitespace. @@ +300,5 @@ > +========= ======== ============================================================ > +name type description > +========= ======== ============================================================ > +**id** mixed An integer bug id or alias. > +new_since datetime A datetime timestamp to only show history since. Perhaps I missed where this is explained, but how is new_since specified on a GET? URL parameter? @@ +855,5 @@ > + will be the new component, not the old one.) > +reset_qa_contact boolean If true, the ``qa_contact`` field will be reset > + to the default for the component that the bug is > + in. (If you have set the component at the same > + time as using this, then the component used will\ Typo ::: docs/en/rst/api/Core/v1/buguserlastvisit.rst @@ +1,1 @@ > +Bug User Last Visited This file is named "buguserlastvisit.rst". The standard I've been establishing with the new docs is to hyphen-separate words (so "bug-user-last-visit.rst"). It would be awesome if you could change this before checkin... Also, is there a reason we are using upper-case "Core" rather than lower-case "core"? @@ +1,4 @@ > +Bug User Last Visited > +===================== > + > +.. _rest_buguserlastvisit_update: Similar to above, I've been using the equivalent of "_rest-bug-user-last-visit-update:" (hyphen-separated). @@ +19,5 @@ > +To add one or more bug ids at once: > + > +.. code-block:: text > + > + POST /rest/bug_user_last_visit There's no example of what goes in the POST body if you are doing multiple IDs at once. ::: docs/en/rst/api/Core/v1/bugzilla.rst @@ +1,4 @@ > +Bugzilla Information > +==================== > + > +These methods are to get general configuration information about this Bugzilla instance. "are _used_ to get"? @@ +66,5 @@ > + > +Timezone > +-------- > + > +Returns the timezone that Bugzilla expects dates and times in. "Expects"? You mean on the REST API? Surely we always and only accept times in UTC? That seems to make more sense than requiring people to ask Bugzilla and then do a conversion themselves. @@ +136,5 @@ > + case you should rely on the ``db_time``, not the > + ``web_time``. > +web_time_utc string Identical to ``web_time``. (Exists only for > + backwards-compatibility with versions of Bugzilla > + before 3.6.) When do we get to remove this stuff? @@ +258,5 @@ > + > +Last Audit Time > +--------------- > + > +Gets the latest time of the audit_log table. I don't understand this sentence. Does it mean "Gets the most recent timestamp among all the events in the audit_log table"? ::: docs/en/rst/api/Core/v1/comment.rst @@ +171,5 @@ > +To search for comment tags: > + > +.. code-block:: text > + > + GET /rest/bug/comment/tags/(query) Hmm. This doesn't have the same syntax as searching bugs... I would expect ?tag=query or something. ::: docs/en/rst/api/Core/v1/flagtype.rst @@ +1,4 @@ > +Flag Types > +========== > + > +This part of the Bugzilla API allows you get and create bug and attachment flags. you _to_ get... @@ +21,5 @@ > +To get information about flag_types for a product and component: > + > +.. code-block:: text > + > + GET /rest/flag_type/(product)/(component) What happens if the product name contains a "/"? URL-encode it? @@ +117,5 @@ > + or ``attachment``. > +description string The description of the flag type. > +values array String values that the user can set on the flag type. > +is_requesteeble boolean Users can ask specific other users to set flags of > + this type. No is_requestable? ::: docs/en/rst/api/Core/v1/general.rst @@ +33,5 @@ > +requests and will override any matching parameters in the request body. > + > +Example request which returns the current version of Bugzilla: > + > +.. code-block:: http Hmm. Seems like you are using 'http' for larger HTTP blocks. Perhaps use it everywhere for consistency? @@ +60,5 @@ > +The error contents look similar to: > + > +.. code-block:: js > + > + { "error": true, "message": "Some message here", "code": 123 } Is there a reason this example is not expanded like the others? @@ +75,5 @@ > +double A floating-point number. May be null > +string A string. May be null > +email A string representing an email address. This value, when returned, > + may be filtered based on if the user is logged in or not. May be > + null. When you say "may be null", do you mean on submission or on receipt? I seem to remember above that sometimes empty fields are not returned at all, rather than returned as "null"... @@ +98,5 @@ > +-------------- > + > +Some methods do not require you to log in. An example of this is > +:ref:`rest_single_bug`. However, authenticating yourself allows you to see non > +public information, for example, a bug that is not publicly visible. "non-public" @@ +128,5 @@ > +The ``Bugzilla_restrictlogin`` option is only used when you have also > +specified ``Bugzilla_login`` and ``Bugzilla_password``. > + > +There is also a deprecated method of authentication. This will be > +removed in the version after Bugzilla 5.0. It's not clear whether the deprecated method is the one immediately following, is documented elsewhere, or is not documented ("there's a deprecated method, but it's a secret! Ssh!" :-). @@ +151,5 @@ > +An error is thrown if you pass an invalid token; you will need to log in again > +to get a new token. > + > +Also starting with Bugzilla 5.0, login cookies are not longer by > +:ref:`rest_user_login` due to security concerns. This sentence doesn't quite make sense. ::: docs/en/rst/api/Core/v1/group.rst @@ +165,5 @@ > + > +Get Group > +--------- > + > +Returns information about a Bugzilla groups. Grammar-o. @@ +169,5 @@ > +Returns information about a Bugzilla groups. > + > +**Request** > + > +To return information about a specific group id name: id _or_ name ::: docs/en/rst/api/Core/v1/product.rst @@ +141,5 @@ > + } > + ] > + }, > + "default_qa_contact": "", > + "description": "This is a test component in the test product database. This ought to be blown away and replaced with real stuff in a finished installation of Bugzilla." Use shorter example to prevent ugly/confusing wrapping or scrollbars?
Attachment #8507008 - Flags: review?(gerv)
Attachment #8507008 - Flags: review-
Attachment #8507008 - Flags: feedback+
Attached patch 1038275_6.patch (obsolete) — — Splinter Review
Uploaded latest to: http://dkl.site44.com/bugzilla/docs/en/html/index.html Places where I have not replied to your review below means they are simply fixed in the new revision. (In reply to Gervase Markham [:gerv] from comment #25) > ::: docs/en/rst/api.rst > > + api/Core/*/index* > > + api/extensions/*/*/index* > > Doesn't this glob exclude filenames like "example.rst"? Is that intentional? My thought was rather than globbing all .rst files for a given extensions api docs, we require each have an index.rst file and then it can include whatever it wants. Similar to how I have api/Core/v1/index.rst structured. I am open to better method for this. > @@ +17,5 @@ > > +To get all current attachments for a bug: > > + > > +.. code-block:: text > > + > > + GET /rest/bug/(bug_id)/attachment > > This makes it look like you can only specify one bug_id you want attachments > for, but below it talks about the "IDs you specify" in the plural. What's > the story there? Same question for attachment_id. Reworded to match better the URL example. > @@ +75,5 @@ > > +================ ======== ===================================================== > > +data base64 The raw data of the attachment, encoded as Base64. > > +size int The length (in bytes) of the attachment. > > +creation_time datetime The time the attachment was created. > > +last_change_time datetime The last time the attachment was modified. > > Does this include metadata? I assume so, but it's not clear. > > @@ +97,5 @@ > > + Each flag object contains items descibed in the > > + Flag object below. > > +================ ======== ===================================================== > > + > > +Flag object: > > Surely you need to describe objects such as Flags more than once in the > docs? To avoid repetition, could you put them in a separate file and include > them? I would like to do this a separate bug later. Reason I have not done it this far is because some of the common objects differ when called from different methods in my experience. > @@ +215,5 @@ > > + > > +==== ===== ========================= > > +name type description > > +==== ===== ========================= > > +ids array Attachment id(s) created. > > If you attach the same attachment to multiple bugs, does the response tell > you which new ID matches which bug? If not, should it? :-) It does not unfortunately. We should file this as a separate bug to be fixed in API v2 as this is a breaking change. > @@ +341,5 @@ > > + items: > > + > > + * added: (string) The values that were added to this > > + field. Possibly a comma-and-space-separated list > > + if multiple values were added. > > If the values contain commas and/or spaces, how are those escaped? From what I can tell, we don't do that now for any values that are in the add/removed values as they are generally not expected to be parseable and just for visible reference in the UI, etc. We would need to file a bug for Bugzilla::Object::update, Bugzilla::Bug::update, Bugzilla::Attachment::update, etc. to escape commas in values where the values are allowed to have commas. > ::: docs/en/rst/api/Core/v1/buguserlastvisit.rst > @@ +1,1 @@ > > +Bug User Last Visited > > This file is named "buguserlastvisit.rst". The standard I've been > establishing with the new docs is to hyphen-separate words (so > "bug-user-last-visit.rst"). It would be awesome if you could change this > before checkin... > > Also, is there a reason we are using upper-case "Core" rather than > lower-case "core"? No special reason except to look like the extension names such as api/extensions/Example, etc. I can change it. > @@ +66,5 @@ > > + > > +Timezone > > +-------- > > + > > +Returns the timezone that Bugzilla expects dates and times in. > > "Expects"? You mean on the REST API? Surely we always and only accept times > in UTC? That seems to make more sense than requiring people to ask Bugzilla > and then do a conversion themselves. Again, this is verbatim from the current POD text. I will reword to be a little clearer. > @@ +136,5 @@ > > + case you should rely on the ``db_time``, not the > > + ``web_time``. > > +web_time_utc string Identical to ``web_time``. (Exists only for > > + backwards-compatibility with versions of Bugzilla > > + before 3.6.) > > When do we get to remove this stuff? Will need to file bug to have it removed. When we remove it from the code, we can remove it from the docs as well. > @@ +258,5 @@ > > + > > +Last Audit Time > > +--------------- > > + > > +Gets the latest time of the audit_log table. > > I don't understand this sentence. Does it mean "Gets the most recent > timestamp among all the events in the audit_log table"? Yeah. Changed. > ::: docs/en/rst/api/Core/v1/comment.rst > @@ +171,5 @@ > > +To search for comment tags: > > + > > +.. code-block:: text > > + > > + GET /rest/bug/comment/tags/(query) > > Hmm. This doesn't have the same syntax as searching bugs... I would expect > ?tag=query or something. We can change that to support both styles in another bug as well. > @@ +21,5 @@ > > +To get information about flag_types for a product and component: > > + > > +.. code-block:: text > > + > > + GET /rest/flag_type/(product)/(component) > > What happens if the product name contains a "/"? URL-encode it? Yeah. You would need to. I will add a note related to that. > @@ +117,5 @@ > > + or ``attachment``. > > +description string The description of the flag type. > > +values array String values that the user can set on the flag type. > > +is_requesteeble boolean Users can ask specific other users to set flags of > > + this type. > > No is_requestable? Good catch. We do not actually export that through the webservices API for some reason. Will need to file a bug to add it later and to the docs. > ::: docs/en/rst/api/Core/v1/general.rst > @@ +33,5 @@ > > +requests and will override any matching parameters in the request body. > > + > > +Example request which returns the current version of Bugzilla: > > + > > +.. code-block:: http > > Hmm. Seems like you are using 'http' for larger HTTP blocks. Perhaps use it > everywhere for consistency? I used http as I was displaying the relevant headers such as Accept and Content-type along with the REST path needed. The other places I used just text which seemed more appropriate without headers. > @@ +75,5 @@ > > +double A floating-point number. May be null > > +string A string. May be null > > +email A string representing an email address. This value, when returned, > > + may be filtered based on if the user is logged in or not. May be > > + null. > > When you say "may be null", do you mean on submission or on receipt? I seem > to remember above that sometimes empty fields are not returned at all, > rather than returned as "null"... I may just remove that altogether for each of the fields as you make a good point. For some calls it can be null on submission, such as the assignee for Bug.create. For some calls it can be null on receipt such as certain custom fields or qa_contact (which is an empty string). It is better to signify if a field can be null/empty in the params list of the each method.
Attachment #8507008 - Attachment is obsolete: true
Attachment #8519157 - Flags: review?(gerv)
Comment on attachment 8519157 [details] [diff] [review] 1038275_6.patch Review of attachment 8519157 [details] [diff] [review]: ----------------------------------------------------------------- My last review elicited a number: "we should file a bug for that" comments. Please can you file said bugs? :-) r=gerv, with or without the suggested changes below. Let's ship it and let people read it and send in bug reports. Gerv ::: docs/en/rst/api/core/v1/attachment.rst @@ +9,5 @@ > +-------------- > + > +This allows you to get data about attachments, given a list of bugs and/or > +attachment IDs. Private attachments will only be returned if you are in the > +*insidergroup* or if you are the submitter of the attachment. This talks about insidergroup but not groups in general. Instead: "Private attachments will only be returned if you are in the appropriate group or if you are the submitter of the attachment." @@ +23,5 @@ > +To get a specific attachment based on attachment ID: > + > +.. code-block:: text > + > + GET /rest/bug/attachment/(attachment_id) Is that right? Is it /rest/bug/attachment/ or /rest/attachment/? Here, "attachment" is now in the namespace of bug IDs. What happens if I had a bug with alias "attachment"? Do we support aliases here? ::: docs/en/rst/api/core/v1/bug-user-last-visit.rst @@ +9,5 @@ > +Update the last visit time for the specified bug and current user. > + > +**Request** > + > +To add a single bug id: "Add" is surely the wrong verb here? -> "To update the time for a single bug id:" @@ +15,5 @@ > +.. code-block:: text > + > + POST /rest/bug_user_last_visit/(id) > + > +To add one or more bug ids at once: And here. @@ +31,5 @@ > +======= ===== ============================ > +name type description > +======= ===== ============================ > +**id** int An integer bug id. > +**ids** array One or more bug ids to add. And here. @@ +61,5 @@ > +---------------- > + > +**Request** > + > +Get the last visited timestamp for one or more specified bug ids or get a mcote is the expert, but I think "last-visited" should by hyphenated here. @@ +76,5 @@ > +.. code-block:: text > + > + GET /rest/bug_user_last_visit/123?ids=234&ids=456 > + > +To return just the last 20 timestamps: last -> "most recent" ::: docs/en/rst/api/core/v1/bug.rst @@ +18,5 @@ > +To get information about a particular bug using its ID or alias: > + > +.. code-block:: text > + > + GET /rest/bug/(id_or_alias) Looks like we do support aliases. So my question above stands. @@ +273,5 @@ > +================= ======== ==================================================== > + > +Custom field object: > + > +You can specify only to return custom fields by specifying ``_custom`` or the "only to" -> "to only" ::: docs/en/rst/api/core/v1/bugzilla.rst @@ +6,5 @@ > + > +Version > +------- > + > +Returns the current version of Bugzilla. We should say a little bit about the format of the version number. @@ +67,5 @@ > + > +Timezone > +-------- > + > +Returns the timezone that Bugzilla expects dates and times in which is Reword: Returns the timezone in which Bugzilla expects to receive dates and times on the API. Currently hard-coded to UTC ("+0000"). This is unlikely to change. @@ +119,5 @@ > +============= ====== ========================================================== > +name type description > +============= ====== ========================================================== > +db_time string The current time in UTC, according to the Bugzilla > + database server. Nit: extra space. ::: extensions/Example/docs/en/rst/example.rst @@ +1,1 @@ > +Example Given that the files are copied elsewhere before being compiled, do we need the "rst" directory level here? Surely all docs in an extension will be ReST? An extension could theoretically ship with translated docs, although I've never seen it.
Attachment #8519157 - Flags: review?(gerv) → review+
(In reply to Gervase Markham [:gerv] from comment #27) > @@ +61,5 @@ > > +---------------- > > + > > +**Request** > > + > > +Get the last visited timestamp for one or more specified bug ids or get a > > mcote is the expert, but I think "last-visited" should by hyphenated here. Indeed, it should be hyphenated. We're not consistent with that everywhere in BMO, but we may as well aim to be.
(In reply to Gervase Markham [:gerv] from comment #27) > @@ +23,5 @@ > > +To get a specific attachment based on attachment ID: > > + > > +.. code-block:: text > > + > > + GET /rest/bug/attachment/(attachment_id) > > Is that right? Is it /rest/bug/attachment/ or /rest/attachment/? > > Here, "attachment" is now in the namespace of bug IDs. What happens if I had > a bug with alias "attachment"? Do we support aliases here? Yes it is /rest/bug/attachment/(attachment_id) and not /rest/attachment/(attachment_id). And this will work as if someone does ^/rest/bug/attachment$ it will look for a bug with the alias 'attachment' (GET) as there is no / at the end. The call will need to append /(attachment_id) to look up a specific attachment. And only attachment ids work and not bug aliases. Or maybe I do not fully understand what you are asking. > ::: docs/en/rst/api/core/v1/bug.rst > @@ +18,5 @@ > > +To get information about a particular bug using its ID or alias: > > + > > +.. code-block:: text > > + > > + GET /rest/bug/(id_or_alias) > > Looks like we do support aliases. So my question above stands. Yes we support aliases. The alias must be the last thing in the path and not contain any / characters. I realize we support aliases through the UI with / characters in them. For REST the caller will need to use the bug id instead for now. I will file a bug about this. I even tried converting the / to %2F for an alias that contained the character. The REST/JSONRPC backend code was converting it back to / before checking the regexes and so complains about illegal path. > ::: extensions/Example/docs/en/rst/example.rst > @@ +1,1 @@ > > +Example > > Given that the files are copied elsewhere before being compiled, do we need > the "rst" directory level here? Surely all docs in an extension will be ReST? > > An extension could theoretically ship with translated docs, although I've > never seen it. I was mirroring the current layout in the core docs/en/rst directories and feel it is harmless to leave it that way. If anything, it reminds the developer that it needs to be in the ReST format. Suppose in the future we could even have a txt and/or html dir in the extension's docs dir that gets copied over that the ReST docs could refer to. Dunno. dkl dkl
Attached patch 1038275_7.patch (obsolete) — — Splinter Review
Carrying forward r+.
Attachment #8519157 - Attachment is obsolete: true
Attachment #8526835 - Flags: review+
Flags: approval?
Blocks: 1102982
Flags: approval? → approval+
Target Milestone: --- → Bugzilla 6.0
> Target Milestone: --- → Bugzilla 6.0 I was kind of hoping this would make it into 5.0 as the docs match up with the REST API that is included in 5.0. Is there a reason why you feel it should not be included and should be pushed off til 6.0? dkl
Flags: needinfo?(glob)
(In reply to David Lawrence [:dkl] from comment #31) > I was kind of hoping this would make it into 5.0 as the docs match up with > the REST API that is included in 5.0. Is there a reason why you feel it > should not be included and should be pushed off til 6.0? you didn't request approval for 5.0, and the patch appears to be a trunk patch (5.0 doesn't include markdown, which is visible in the first chunk's diff context). looking at the docs, it appears that's the only reference to markdown, which means the document is incomplete with respect to trunk. i'm withdrawing my approval+ because markdown requires documentation on trunk. we'd need a separate 5.0-specific patch that version.
Flags: needinfo?(glob)
Flags: approval+
Comment on attachment 8526835 [details] [diff] [review] 1038275_7.patch as per comment 32, the comment object on trunk includes the is_markdown boolean however that is missing from these docs.
Attachment #8526835 - Flags: review-
Attachment #8526835 - Attachment filename: 1038275_7.patch → Trunk 1038275_7.patch
Attachment #8527765 - Flags: review?(glob)
Comment on attachment 8527765 [details] [diff] [review] 5.0 1038275-5.0_1.patch given gerv's review of the old patch and his familiarity with rst, redirecting the review to him. gerv - feel free to bounce it back to me if you aren't in a position to do this review.
Attachment #8527765 - Flags: review?(glob) → review?(gerv)
Attachment #8527765 - Flags: review?(gerv) → review+
Flags: approval?
Flags: approval5.0?
we need an updated patch for trunk which includes the markdown field(s).
Flags: approval?
Trunk patch with markdown fields added. dkl
Attachment #8526835 - Attachment is obsolete: true
Attachment #8528395 - Flags: review?(gerv)
Attachment #8528395 - Flags: review?(gerv) → review+
Flags: approval?
Flags: approval?
Flags: approval5.0?
Flags: approval5.0+
Flags: approval+
Target Milestone: Bugzilla 6.0 → Bugzilla 5.0
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git 649a389..ad031a1 master -> master To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git 45877e0..fd1625f 5.0 -> 5.0
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Blocks: 1105501
Blocks: 1158563
Blocks: 1159318
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: