Page MenuHomePhabricator

'endorsementcomment' is required on jadeproposeorendorse. Shouldn't be.
Closed, DeclinedPublic

Description

action=jadeproposeorendorse requires "endorsementcomment" be set. But it shouldn't. This task is done when we no longer require that field.

Event Timeline

Halfak created this task.

Making this low because it's not a blocker. You can set a blank endorsementcomment and it should work. But we'll still want to get this cleaned up eventually.

Halfak raised the priority of this task from Low to High.Jun 1 2020, 5:00 PM
Halfak lowered the priority of this task from High to Medium.Jun 1 2020, 5:05 PM

See API spec here:

1jadeproposeorendorse -- Catch all, routing method that tries to *do the right thing*
2 @param title (str) The encoded entity name. (E.g. "Jade:Diff/123456") [required if entitytype or entityid are not set]
3 @param entitydata (json) The type of Jade entity (E.g., {"type": "diff", "id": 3245678}) [required if title not set]
4 @param facet (str) The facet of the entity being labeled [required]
5 @param labeldata (json) The relevant new label data [required]
6 @param notes (str) Notes to save when creating a new proposal (A warning will be raised if a proposal already exists. Notes will not be automatically overwritten.)
7 @param endorsementcomment (str) Comment to leave with the endorsement. Defaults to "As proposer" if not set and creating new proposal.
8 @param origin (str) A structured string representing what the user was looking at when they made this judgment [required]
9 @param comment (str) Revision summary
10 @param token (str) [required]
11 @warning endorsingnonpreferredproposal -- This action resulted in creating an endorsement for a proposal that is not flagged as preferred.
12 @warning nochange -- This action would result in no change. {user} has already endorsed {labeldata}.
13
14jadecreateandendorse -- Creates a new proposal and files an endorsement. If the user already endorsed another proposal within the facet, move the user's endorsement to the new proposal.
15 @param title (str) The encoded entity name. (E.g. "Jade:Diff/123456") [required if entitytype or entityid are not set]
16 @param entitydata (json) The type of Jade entity (E.g., {"type": "diff", "id": 3245678}) [required if title not set]
17 @param facet (str) The facet of the entity being labeled [required]
18 @param labeldata (json) The relevant new label data [required]
19 @param notes (str) Notes to save when creating a new proposal (A warning will be raised if a proposal already exists. Notes will not be automatically overwritten.)
20 @param endorsementcomment (str) Comment to leave with the endorsement. Defaults to "As proposer" if not set.
21 @param origin (str) A structured string representing what the user was looking at when they made this judgment [required]
22 @param comment (str) Revision summary
23 @param nomove (str) If set, do not move an endorsement. Instead throw an error.
24 @param token (str) [required]
25 @error proposalexists -- If a proposal with the specified data already exists
26 @warning endorsingnonpreferredproposal -- This action resulted in creating an endorsement for a proposal that is not flagged as preferred.
27
28jadeendorse -- Adds a new endorsement to a proposal
29 @param title (str) The encoded entity name. (E.g. "Jade:Diff/123456") [required if entitytype or entityid are not set]
30 @param entitydata (json) The type of Jade entity (E.g., {"type": "diff", "id": 3245678}) [required if title not set]
31 @param facet (str) The facet of the entity being labeled [required]
32 @param labeldata (json) The relevant label data [required]
33 @param endorsementcomment (str) Comment to leave with the endorsement. Defaults to "As proposer" if not set.
34 @param origin (str) A structured string representing what the user was looking at when they made this judgment [required]
35 @param comment (str) Revision summary
36 @param nomove (bool) If set, do not move an endorsement. Instead throw an error. If true, warn that the endorsement was moved.
37 @param token (str) [required]
38 @error proposalnotfound -- Could not find a proposal with matching "data"
39 @warning endorsingnonpreferredproposal -- This action resulted in creating an endorsement for a proposal that is not flagged as preferred.
40 @warning nochange -- This action would result in no change. {user} has already endorsed {labeldata}.
41
42jademoveendorsement -- Moves an endorsement to a new proposal. If "data" matches the currently endorsed proposal, make no change (except maybe to endorsement comment)
43 @param title (str) The encoded entity name. (E.g. "Jade:Diff/123456") [required if entitytype or entityid are not set]
44 @param entitydata (json) The type of Jade entity (E.g., {"type": "diff", "id": 3245678}) [required if title not set]
45 @param facet (str) The facet of the entity [required]
46 @param labeldata (json) The relevant new label data [required]
47 @param comment (str) Revision summary
48 @param endorsementcomment (str) Leave unchanged if unset.
49 @param token (str) [required]
50 @error proposalnotfound -- Could not find a proposal with matching "data"
51 @warning endorsingnonpreferredproposal -- This action resulted in an endorsement for a proposal that is not flagged as preferred.
52 @warning nochange -- This action would result in no change. {user} has already endorsed {labeldata}.
53
54jadedeleteproposal -- Removes a specific proposal
55 @param title (str) The encoded entity name. (E.g. "Jade:Diff/123456") [required if entitytype or entityid are not set]
56 @param entitydata (json) The type of Jade entity (E.g., {"type": "diff", "id": 3245678}) [required if title not set]
57 @param facet (str) The facet of the entity [required]
58 @param labeldata (json) The relevant label data [required]
59 @param comment (str) Revision summary
60 @param token (str) [required]
61 @error proposalnotfound -- Could not find a proposal with matching "data"
62 @error proposalispreferred -- Cannot delete a preferred proposal except when it is the only remaining proposal for a facet. You must set another proposal to be "preferred"
63
64jadedeleteendorsement -- Removes the user's endorsement
65 @param title (str) The encoded entity name. (E.g. "Jade:Diff/123456") [required if entitytype or entityid are not set]
66 @param entitydata (json) The type of Jade entity (E.g., {"type": "diff", "id": 3245678}) [required if title not set]
67 @param facet (str) The facet of the entity [required]
68 @param user_id (int) If set, remove this user's endorsement [default: self] [required one of (user_id, global_id, ip)]
69 @param global_id (int) If set, remove this user's endorsement (If centralauth is in use) [default: self] [required one of (user_id, global_id, ip)]
70 @param ip (str) If set, remove this user's endorsement [default: self] [required one of (user_id, global_id, ip)]
71 @param comment (str) Revision summary
72 @param token (str) [required]
73 @error endorsementnotfound -- Could not find an endorsement from the target user
74
75jadesetpreference -- Moves the preference bit from one proposal to another within a facet
76 @param title (str) The encoded entity name. (E.g. "Jade:Diff/123456") [required if entitytype or entityid are not set]
77 @param entitydata (json) The type of Jade entity (E.g., {"type": "diff", "id": 3245678}) [required if title not set]
78 @param facet (str) The facet of the entity [required]
79 @param labeldata (json) The relevant label data [required]
80 @param token (str) [required]
81 @error proposalnotfound -- Could not find a proposal with matching "data"
82 @warning nochange -- This action would result in no change. {labeldata} is already set as "preferred"
83
84jadeupdateproposal
85 @param title (str) The encoded entity name. (E.g. "Jade:Diff/123456") [required if entitytype or entityid are not set]
86 @param entitydata (json) The type of Jade entity (E.g., {"type": "diff", "id": 3245678}) [required if title not set]
87 @param facet (str) The facet of the entity [required]
88 @param labeldata (json) The relevant label data [required]
89 @param notes (str) The new notes field for the proposal [required]
90 @param token (str) [required]
91 @error proposalnotfound -- Could not find a proposal with matching "data"
92 @warning nochange -- This action would result in no change. Notes are identical.
93
94jadeupdateendorsement
95 @param title (str) The encoded entity name. (E.g. "Jade:Diff/123456") [required if entitytype or entityid are not set]
96 @param entitydata (json) The Jade entity (E.g., {"type": "diff", "id": 3245678}) [required if title not set]
97 @param facet (str) The facet of the entity [required]
98 @param user_id (int) If set, remove this user's endorsement [default: self] [required one of (user_id, global_id, ip)]
99 @param global_id (int) If set, remove this user's endorsement (If centralauth is in use) [default: self] [required one of (user_id, global_id, ip)]
100 @param ip (str) If set, remove this user's endorsement [default: self] [required one of (user_id, global_id, ip)]
101 @param endorsementcomment (str) The new comment field for the endorsement
102 @param token (str) [required]
103 @error endorsementnotfound -- Could not find an endorsement from the target user
104 @warning nochange -- This action would result in no change. Endorsement comments are identical.
105
106jadegetlabels
107 @param titles (str, multiple) The encoded entity name. (E.g. "Jade:Diff/123456") [required if entitytype or entityid are not set]
108 @param entitydata (json, multiple) The Jade entity (E.g., {"type": "diff", "id": 3245678}) [required if title not set]
109 @param facet (str, multiple) Which facets to include in the response [default: all]
110 @param props (str, multiple) Which facet parts to include endorsements, notes, nonpreferred. [default: notes]

ACraze closed this task as Declined.EditedSep 15 2020, 9:53 PM
ACraze added a subscriber: ACraze.

I think setting endorsementcomment to '' (empty string) is a decent workaround for now.

The reason this issue happens is due to the UpdateEndorsement api module, which can be called by action=jadeproposeorendorse. At some point we made UpdateEndorsement explicitly require an endorsementcomment, since that is the sole reason for that api module to exist (to update an endorsement's comment). That requirement bubbles up to ProposeOrEndorse and if we were to remove it, it will break other api modules that default the endorsementcomment to things like 'As proposer' , etc. We would need to refactor the way EntityBuilder handles the comments for different edge cases.

Going to mark this as 'Declined', as it will require a non-trivial refactor. Will make sure to revisit for Jade v2.

It might be a good idea to stick these details in a new task that can wait in the backlog until you're ready to spec out Jade v2