Skip to content
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

feat(Identifier types): Identifier Types Page and Identifier Types Editor #1012

Merged

Conversation

the-good-boy
Copy link
Contributor

This PR deals with Identifier Types.

Now, there is link to access an Identifier Types Page from the 'Docs' Menu:
identifierTypesLink

The new Identifier Types Page has the tree structure similar to the Relationship Types page. It also has tabs for different entity types!
identifierTypesPage

We also have a link to go to an Identifier Type Editor:
identifierTypeEditorLink

Now, privileged users can add and edit identifier types!
identifierTypeEditor

@the-good-boy the-good-boy marked this pull request as ready for review August 19, 2023 19:39
Copy link
Member

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work, nice refactoring of the relationship type editor code, it's nice to see :)

First round of review

src/client/containers/layout.js Outdated Show resolved Hide resolved
src/client/containers/layout.js Show resolved Hide resolved
src/server/helpers/typeRouteUtils.ts Outdated Show resolved Hide resolved
test/test-helpers/create-entities.js Outdated Show resolved Hide resolved
Copy link
Member

@anshg1214 anshg1214 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work. Just a few changes.

Also I read your proposal again, are you planning to have a delete route for the identifiers?

@MonkeyDo
Copy link
Member

Also I read your proposal again, are you planning to have a delete route for the identifiers?

I didn't click during the proposal review process, but we probably don't want a delete endpoint.
We can however already set the "deprecated" column which is the type of soft-deletion that we want.

Otherwise, we would have to cascade modify all the identifiers that use that identifier type, probably not a good idea (I'd bet you 5 schmekels that's how we got that ID sequence issue…).
Same goes for relationship types.

Copy link
Member

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second round of reviews !

It's all working fairly well, and the code looks good too.

Comment on lines +148 to +149
function isValid() {
return Boolean(formData.entityType);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name of this function suggests more validation than just whether an entity type has been selected.
Is a portion of this validation function missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the outset, I wanted an isValid function to check those required fields on the form which cannot directly be checked by adding a required flag. In case of the relationship type editor, there were two such fields: a sourceEntityType and a targetEntityType. In this case, we only have one such field: entityType.

I agree the name isValid itself is a bit more general(and maybe misleading), perhaps it should have been something like checkRequiredFields. We can also get rid of this function here, but I went ahead with it just to make it similar to the relationship type editor code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally it should check all the required fields, but the combination of the required attribute on the inputs and the error thrown by the server is probably sufficient.
Trying to prove that point, I edited the html to remove the required attribute on all the inputs (but selecting an entity type to satisfy isValid) and I can submit with empty values, and get an error back from the server:
image

IMO this is sufficient for this type of interface, and if proper validation should be done it should be done on the server-side.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so what I mean by required fields is this: for non-select fields, there is prompt which occurs like this in case we try to submit the form without entering anything in that field:
Screenshot 2023-09-18 215839

For the select type fields, there is no such validation. In order to prevent sending incomplete data to backend, we should probably use this function to do this in frontend itself:
Screenshot 2023-09-18 at 22-02-12 BookBrainz – The Open Book Database

Copy link
Contributor Author

@the-good-boy the-good-boy Sep 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although you're right, ideally we should be doing for all the fields. The only reason I went for this is because for the non-select fields at least there is a prompt which tells us Please fill out this field., while for the select field, there is no such prompt.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so what I mean by required fields is this: for non-select fields, there is prompt which occurs like this in case we try to submit the form without entering anything in that field:

Yep, completely understand, tested and approved :)
I just tried to break it by manually modifying the HTML in my page to allow empty inputs, which would not normally happen in any reasonable circumstance I can imagine.

src/client/containers/layout.js Show resolved Hide resolved
src/server/routes/search.js Show resolved Hide resolved
@anshg1214
Copy link
Member

@the-good-boy I believe you should have a page for re-indexing the server.

@the-good-boy
Copy link
Contributor Author

the-good-boy commented Sep 18, 2023

@the-good-boy I believe you should have a page for re-indexing the server.

Yeah definitely it would be better. Perhaps, we should just redirect to some page after a successful reindex?

How about we redirect to the homepage, but there is a brief message which indicates that the reindex was successful? Let me think about this.

@MonkeyDo
Copy link
Member

Yes I think any UI would be better than the current state of it :)

IMO a page with a "start reindexing" button and a way to indicate the status (i.e. in progress // finished) would be perfect.
We're bound to end up with more options/actions in the future.

To start with, maybe even just a page with a button would be an improvement :)

@MonkeyDo MonkeyDo merged commit 5f35dbc into metabrainz:administration-system Sep 18, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants