-
Notifications
You must be signed in to change notification settings - Fork 427
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make the "Create group" button actually work #8811
Conversation
f557f7e
to
cc21d42
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good as a first step. We may want to eventually extract some helpers for API calls as we have in LMS, but that can be done later to avoid a premature abstraction.
name: name, | ||
description: description, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name: name, | |
description: description, | |
name, | |
description, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
name: name, | ||
description: description, | ||
}), | ||
credentials: 'same-origin', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the default, so you shouldn't need to specify it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
const [name, setName] = useState(''); | ||
const [description, setDescription] = useState(''); | ||
|
||
const createGroup = async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can wrap this in useCallback
, to avoid a new function reference to be created on every render of this component, which would in turn re-render the Button
where this is passed as onClick
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Button
is re-rendered on each render of this component regardless. The main cost of callback arguments is when changing them triggers expensive downstream work such as re-running effects or recomputing values that depend on them. For event listeners on DOM elements specifically, Preact has an optimization to make changing them on each render cheap: it doesn't call addEventListener
/removeEventListener
each time.
In short, you certainly could use useCallback
here, but I think it is also fine not to.
I would actually do that right away, as it will make writing tests for the component easier. Plus it will introduce seanh to how we do mocking of this kind of helper. See the |
2c56936
to
14181ae
Compare
cc21d42
to
b77b509
Compare
e5be15f
to
1f8a552
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a look over this and left a few notes. One general thing I noted regarding tests is that I recommend avoiding deeply nested context
/describe
blocks. As a module grows, this makes it difficult to easily track the environment that a test has.
It would probably make sense in future for us to unify the various fetch
wrappers that we have for APIs different projects as part of the @hypothesis/frontend-shared
package or another @hypothesis
package. This will probably involve standardizing some aspects of API responses that currently vary in unnecessary ways.
const options = { | ||
method: method, | ||
headers: { 'Content-Type': 'application/json; charset=UTF-8' }, | ||
} as any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const options = { | |
method: method, | |
headers: { 'Content-Type': 'application/json; charset=UTF-8' }, | |
} as any; | |
const options: RequestInit = { | |
method: method, | |
headers: { 'Content-Type': 'application/json; charset=UTF-8' }, | |
}; |
This allows you to set additional properties on options
after construction and still have usage be checked. The RequestInit
type comes from the signature of fetch
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
}); | ||
}); | ||
|
||
context('when the API responds with an error', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would advise against deeply nested use of context
/describe
blocks. As tests grow it becomes difficult to see easily what the environment is (variables, setup functions etc.) for each test.
Also I don't think testing the full cross product of options is useful here. What I would suggest doing instead is defining a table of "interesting" cases. Something more like:
[{
status: 400,
body: { foo: 'bar' },
expected: APIError(400, { foo: 'bar' }),
},{
...
}].forEach(({ status, body, expected }) => {
it('returns expected error', () => {
});
});
To give each test a useful label you can add a label/description field to the table and include that in the test name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
try { | ||
responseJSON = await response.json(); | ||
} catch (error) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that Via's code does the same thing. This does have the unfortunate effect of hiding the parse error message. Instead it has to be inferred from the arguments to APIError
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up refactoring this so that it raises an APIError
with the error raised by response.json()
as the APIError
's cause
json: object | undefined; | ||
|
||
constructor(status: number, json?: object) { | ||
super(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One easy thing that can be done for better debug-ability is to generate a message and pass it as the argument to super()
. This will be exposed as the Error.message
property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
const [name, setName] = useState(''); | ||
const [description, setDescription] = useState(''); | ||
|
||
const createGroup = async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Button
is re-rendered on each render of this component regardless. The main cost of callback arguments is when changing them triggers expensive downstream work such as re-running effects or recomputing values that depend on them. For event listeners on DOM elements specifically, Preact has an optimization to make changing them on each render cheap: it doesn't call addEventListener
/removeEventListener
each time.
In short, you certainly could use useCallback
here, but I think it is also fine not to.
@@ -57,6 +60,8 @@ function TextField({ | |||
classes = '', | |||
}: { | |||
type: 'input' | 'textarea'; | |||
value: string; | |||
setValue: any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setValue: any; | |
onChangeValue: (newValue: string) => void; |
The convention for callbacks invoked when events happen, including edits to values of a control, is to start the name with on
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
const [description, setDescription] = useState(''); | ||
|
||
const createGroup = async () => { | ||
let response: any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the client and LMS we have a module that contains types for API request and response bodies (see api-types.ts
in the LMS app, types/api.ts
in the client). This is useful as it allows easy comparison of the frontend's understanding of a response's shape with what the backend generates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added: 3d7916e
Also for error responses: ee67ab0
These do seem to be of limited usefulness since I don't think the frontend can assume that the server's responses will always conform to the expected types so the frontend still has to code defensively, as in:
if (response.links && response.links.html) {
setLocation(response.links.html);
} else {
setErrorMessage('Invalid API response.');
}
(ideally that would also test that response.links.html
is a valid URL string or handle errors from setLocation()
).
Or:
if (responseJSON && responseJSON.reason) {
message = responseJSON.reason;
} else {
message = 'API request failed.';
}
} catch (error) { | ||
// TODO | ||
} finally { | ||
window.location.href = response.links.html; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obviously this is not finished yet, but note that response
will be undefined
here if callAPI
failed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the finally
is out of place here. I think what I might actually want is just multiple lines of code inside the try
block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -124,7 +150,9 @@ export default function CreateGroupForm() { | |||
|
|||
<div className="flex"> | |||
<div className="grow" /> | |||
<Button variant="primary">Create group</Button> | |||
<Button onClick={createGroup} variant="primary"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we probably want to make this a type="submit"
button and put the logic to create the group in the form's onSubmit
handler. This will allow various browser behaviors that interact with form submissions to work as normal. For example in the classic "create group" form, pressing "Enter" when the name field is focused will perform validation of the form and submit it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
By the way, a newish API that may come in useful when writing tests for forms - https://developer.mozilla.org/en-US/docs/Web/API/HTMLFormElement/requestSubmit. Unlike |
1f8a552
to
904ba79
Compare
class APICookiePolicy: | ||
"""Authenticate API requests with cookies.""" | ||
|
||
def __init__(self, cookie_policy: CookiePolicy): | ||
self.cookie_policy = cookie_policy | ||
|
||
@staticmethod | ||
def handles(request: Request) -> bool: | ||
"""Return True if this policy applies to `request`.""" | ||
return ( | ||
request.matched_route.name, | ||
request.method, | ||
) in COOKIE_AUTHENTICATABLE_API_REQUESTS | ||
|
||
def identity(self, request: Request) -> Identity | None: | ||
return self.cookie_policy.identity(request) | ||
|
||
def authenticated_userid(self, request: Request) -> str | None: | ||
return self.cookie_policy.authenticated_userid(request) | ||
|
||
def permits(self, request: Request, context, permission: str) -> Allowed | Denied: | ||
return self.cookie_policy.permits(request, context, permission) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with a separate APICookiePolicy
class rather than having APIPolicy
just delegate to CookiePolicy
itself for a couple of reasons:
- The
handles()
method is used byAPIPolicy
to decide when to delegate toAPICookiePolicy
, butTopLevelPolicy
does not usehandles()
when delegating non-API requests toCookiePolicy
, it just blindly delegates all non-API requests toCookiePolicy
. I thought it'd be weird forCookiePolicy
to have ahandles()
method that's only used in the context of API requests - API sub-policies don't need the
remember()
andforget()
methods andAPIPolicy
never calls these methods, so it's potentially a bit confusing for an API sub-policy to have these methods.
Using composition rather than inheritance here which I think leads to much clearer code and better, more isolated unit tests. Also allows APICookiePolicy
to use CookiePolicy
for identity()
, authenticated_userid()
and permits()
but not inherit CookiePolicy
's remember()
and forget()
. Also APICookiePolicy.__init__()
will not be affected by any changes to CookiePolicy.__init__()
ff5bf63
to
accb739
Compare
limit={250} | ||
label="Description" | ||
testid="description" | ||
classes="h-24" | ||
/> | ||
|
||
<div className="flex"> | ||
<div className="flex items-center"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add items-center
here so that the error message text is vertically-aligned with the text inside the Create group button.
accb739
to
b5f1b6d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionally this works as expected, except for an issue where the new form allows creation of groups with all-whitespace names whereas the old form did not.
The implementation and tests generally look good. I added a few notes on various things. There is a question I raised around whether the current security for the cookie (SameSite=Lax
) is adequate to prevent all potential cross-site requests we need to block, or whether we should implement some stronger medicine.
setLocation(response.links.html); | ||
} else { | ||
setErrorMessage('Invalid API response.'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the h and lms frontends the convention is that, if we get a 2xx status and a valid JSON body, we trust the shape of the server's response to match our expectations as codified in the API types. We could choose to be more defensive to guard against problems such as version skew (frontend version X encounters incompatible API response from backend version Y), but I would prefer to do that in a way that is not ad-hoc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now I've just removed this: 042d53a
Perhaps we can add a more formal validation step in a future PR.
The way we'd do this on the backend (for example when on of our servers is calling a third-party API like the Canvas API, or when LMS is calling the h API, etc) is that we have a pattern of validating responses that we receive by passing them through Marshmallow schemas. It's good for consistency and code reuse to have a consistent way of implementing validation, and things like error views can be written to handle Marshmallow's validation errors. Does the frontend have a similar pattern anywhere?
It doesn't have to be a proper validation, could just be JavaScript code, but with some sort of consistent pattern. For example callAPI()
could accept a validator()
function as an argument that either returns the validated (and possibly transformed) data or throws a validation error of some sort. That way callAPI()
could return the already-validated (and maybe transformed) data or raise its same APIError
class for any validation errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unlike the backend, the frontend has always been calling APIs that we control. Hence the risk has been low enough that we've been able to get away without any validation. Instead we've relied on type definitions to help ensure alignment between frontend and backend and help with refactoring (eg. if we change the type of a property, TypeScript will flag up all the consumers that need to change). In cases where the API is proxying data from a third party, the backend has been responsible for validation and transformation.
There are packages like ajv that can be used for validation using JSON schemas. Setting this up obviously adds complexity to the build toolchain and some runtime overhead. If we did introduce something like this then we would integrate it into the utilities for calling the API.
'../config': { | ||
default: () => config, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use '../module/path': someStub
, where someStub
is a function, as a shorthand for '../module/path': { default: someStub }
. This is useful if replacing a UI component with a fake for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
it("doesn't show an error message initially", async () => { | ||
const { wrapper } = createWrapper(); | ||
|
||
assert.equal(wrapper.find('[data-testid="error-message"]').exists(), false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert.equal(wrapper.find('[data-testid="error-message"]').exists(), false); | |
assert.isFalse(wrapper.exists('[data-testid="error-message"]')); |
wrapper.exists
takes a selector as a shorthand for wrapper.find(selector).exists()
. Using the more specific assert
s can produce slightly better error messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
), | ||
true, | ||
); | ||
assert.equal(fakeSetLocation.calledOnceWithExactly(groupURL), true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert.equal(fakeSetLocation.calledOnceWithExactly(groupURL), true); | |
assert.isTrue(fakeSetLocation.calledOnceWithExactly(groupURL)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -1,6 +1,14 @@ | |||
type APIConfig = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type APIConfig = { | |
export type APIConfig = { |
As a general rule, if a type is exported, all the types that it references should also be exported. TypeScript won't complain if you don't, but it may be inconvenient for users not to be able to reference the type. eg. Suppose a consumer wanted to write a helper that accepted an APIConfig
as an argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
url: string, | ||
method: string = 'GET', | ||
json: object | undefined, | ||
): Promise<object | Array<any>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be inclined to just require that all API responses return objects, something we recently decided to do for the LMS. I think that is the case for all methods in the h API. The rationale is that even if you have a list-like response, you will usually want to add non-list-item metadata fields (eg. total result count, pagination links), or have the capability to do so in future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, done.
try { | ||
response = await fetch(url, options); | ||
} catch (fetchError) { | ||
throw new APIError('Network request failed.', fetchError); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually omit periods at the end of error messages. The caller can then add them if appropriate in the context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whaat? So the UI layer would take the English-language error message from callAPI()
and then append a period to it? That seems a bit odd to me. What if callAPI()
's error message ends with another punctuation character like Authentication error. Are you logged in?
or something. Or what if the error contains multiple sentences? You aren't authorised to delete this group. Only the group's admin can do that.
Happy to make the change if you want but it seems odd to me
message = 'API request failed.'; | ||
} | ||
|
||
throw new APIError(message, null, response, responseJSON); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to use an options object for all the optional arguments to APIError
. This will make it obvious what the null
is here and will make refactoring easier if we need to add any other data in future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: d4fec84
return ( | ||
request.matched_route.name, | ||
request.method, | ||
) in COOKIE_AUTHENTICATABLE_API_REQUESTS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The h session cookie currently uses SameSite=Lax
. This means that the cookies will be set on requests coming from h pages, but also on navigations from other websites to h if the request uses a "safe" (non-mutating) HTTP method (per https://datatracker.ietf.org/doc/html/draft-west-first-party-cookies-07#section-4.1.1). This means that a website could create a link or form which triggers a GET
request that allows cookie auth, but not a POST/PATCH requests like the groups requests added here.
If we wanted to be more secure, we could use a separate SameSite=strict
cookie for this auth, or add a check on the Origin
header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I'd have to do some investigating to see what backend changes would be needed to:
- Set two cookies: the usual
SameSite=Lax
one and (at least on pages that have the Preact app) also a newSameSite=strict
cookie - Change
APICookiePolicy
to only accept authentication from theSameSite=strict
cookie.
It sounds like it's doable but perhaps it can be a follow-up PR? Added a note for it to the project board: https://github.com/orgs/hypothesis/projects/142/views/1?pane=issue&itemId=72776558
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds like it's doable but perhaps it can be a follow-up PR?
Yes, I think that makes sense.
/* A successful response from h's create-new-group API: | ||
* https://h.readthedocs.io/en/latest/api-reference/v2/#tag/groups/paths/~1groups/post | ||
*/ | ||
export type CreateGroupAPIResponse = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/* A successful response from h's create-new-group API: | |
* https://h.readthedocs.io/en/latest/api-reference/v2/#tag/groups/paths/~1groups/post | |
*/ | |
export type CreateGroupAPIResponse = { | |
/** | |
* A successful response from h's create-new-group API: | |
* https://h.readthedocs.io/en/latest/api-reference/v2/#tag/groups/paths/~1groups/post | |
*/ | |
export type CreateGroupAPIResponse = { |
JSDoc-style comments are either a single line of the form /** Short summary. */
or a multi-line comment of the form:
/**
* Short summary.
*
* More details...
*/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
How do you feel about leaving it up to the backend to validate this? I think it's a bit awkward to do on the frontend because we'd have to duplicate whatever validation the backend applies (e.g. strip leading and trailing whitespace before validation; or reject any group names with leading and trailing whitespace as invalid; whatever the backend ends up doing). It's one thing to re-implement minimum and maximum length validation on the frontend: that's relatively simple, and makes sense to have on the frontend given that we have the character counters anyway. But duplicating leading and trailing whitespace validation on the frontend might be a step too far? #8834 adds backend validation to prevent this. This currently results in the frontend showing the backend's error message in an ugly way because the frontend can't tell which field the error applies to. But that's a general problem that we'll want to fix at some point in the future anyway, and that we can probably put up with for now. Screencast.from.2024-07-30.17-33-05.mp4 |
I think this is fine. This reminds me though, we have a minor issue with the frontend and backend counting characters differently. The frontend's character counter is using If you paste in "😊" into the field it will be reported as two characters. A way to fix this is to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all worked as expected for me.
@@ -1,6 +1,9 @@ | |||
import { useId, useState } from 'preact/hooks'; | |||
|
|||
import { Button, Input, Textarea } from '@hypothesis/frontend-shared'; | |||
import readConfig from '../config'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed this before, but readConfig
should be a non-default export. The convention in our code is to use default exports only for Preact components. This tends to avoid accidentally "renaming" symbols (eg. defining the function as getConfig
inside config.ts
and importing it as readConfig
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
const [name, setName] = useState(''); | ||
const [description, setDescription] = useState(''); | ||
const [errorMessage, setErrorMessage] = useState(''); | ||
const config = readConfig(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const config = readConfig(); | |
const config = useMemo(() => readConfig(), []) |
This avoids re-reading the config on each render.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
{errorMessage ? ( | ||
<div | ||
className="text-red-error font-bold" | ||
data-testid="error-message" | ||
> | ||
{errorMessage} | ||
</div> | ||
) : null} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The convention for conditional rendering with no "else" block is to use {condition && jsx_expression}
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
}; | ||
}; | ||
|
||
/* An error response from the h API: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/* An error response from the h API: | |
/** | |
* An error response from the h API: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Count unicode code points rather than UTF-16 code units for a string's length, for consistency with how the backend does it. For example if you enter 😊 into one of the field's it will now be counted as one character not two. See #8811 (comment)
6f9a3e1
to
f139d80
Compare
f139d80
to
2145c55
Compare
Count unicode code points rather than UTF-16 code units for a string's length, for consistency with how the backend does it. For example if you enter 😊 into one of the field's it will now be counted as one character not two. See #8811 (comment)
Done: 47d907d |
Make the "Create group" button (or hitting Enter when the keyboard focus is in the Name field) actually call the API to create the group and then redirect the browser to the newly-created group's page. Also handle errors from the API by displaying error messages to the user. Known issues: * A min-length should be added to the name field so that it's not possible to submit a too-short name that the API will reject. This will be added in a future commit. * A spinner or loading state of some sort needs to be displayed when the API request is in flight. This will be added in a future commit.
e86004b
to
d79f8cc
Compare
Make the "Create group" button on the new, Preact-based version of the create-group page actually work: make it create the group (via an API call) then navigate the browser to the successfully-created group's page.
This PR ended up being on the large-ish side because it ended up having to do a few different things. Let me know if you want me to split it up into smaller PRs:
APICookiePolicy
and changedTopLevelPolicy
to use it (passing it intoAPIPolicy
)js-config
objectcallAPI()
helper for calling APIs<CreateGroupForm />
component to send the API request when the form is submitted, and to either redirect the browser to the new group's page (if the request is successful) or to display an error message.@hypothesis/frontend-testing
to the dev dependencies so that I could usewaitForElement()
in the testsKnown issues
Testing
devdata_admin
: http://localhost:5000/loginpreact_create_group_form
feature flagIf you try to create a group with a valid name and description but the API request fails then an error will be displayed. There are a number of ways to simulate this:
1. Error response received
Hack the code to make the backend return an API error response. You should see the error message from the backend displayed in the UI.
2. Invalid JSON error response
Hack it to respond with a JSON error response but without the expected JSON format. You should see API request failed.
3. Non-JSON error response
Hack it to use a URL that responds with an error HTTP status but without a valid JSON body. You should see API request failed.
4. Non-JSON successful response
Hack it to respond with a successful HTTP status but not a valid JSON body. You should see Invalid API response.
5. Invalid JSON successful successful response
Hack it to use a URL that does responds with a successful HTTP status but without the expected successful create-group API response body. You should see Invalid API response.
6. No API response received
Hack the code to make it use a non-responding URL for the create-group API. You should see a Network request failed error displayed.