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.

Sign up for GitHub

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

Convert 0 linkTypeIDs to null for the relationship dialog #2868

Merged
merged 1 commit into from
Feb 27, 2023
Merged

Convert 0 linkTypeIDs to null for the relationship dialog #2868

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion root/static/scripts/relationship-editor/components/RelationshipPhraseGroup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,13 @@ const RelationshipPhraseGroup = (React.memo<PropsT>(({
[backward ? 'entity1' : 'entity0']: source,
attributes: newAttributesData,
linkOrder: maxLinkOrder > 0 ? (maxLinkOrder + 1) : 0,
linkTypeID: linkTypeId,
/*
* The `typeId` on `RelationshipLinkTypeGroupT` stores empty types as
* `0` (which isn't a valid relationship type row ID anyway) for easier
* sorting. `RelationshipStateT` stores empty types as `null`. Convert
* `0` back to `null` here.
*/
linkTypeID: linkTypeId || null,
Copy link
Member

Choose a reason for hiding this comment

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

Does this just work because 0 is seen as falsey in the comparison? If so, would it be more clear to make it explicit?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, 0 is falsey. It seems pretty clear to me. x || 'fallback' is a ubiquitous pattern in JS.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, it's just linkTypeId is a number, but we're using the number as a falsey boolean. But I guess the argument is that 0 is never a valid ID anyway so it's safe to treat it as just false? :)

Copy link
Member

Choose a reason for hiding this comment

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

I guess in any case the main issue I'd have here is that it's weird to turn null into 0 and then back into null because 0 is not null, and that makes everything confusing, lol. But probably not an issue that should block merging this improvement.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would adding this parenthesis make it more clear?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe, but it's probably not needed anyway. I'm surprised flow doesn't complain about the number vs boolean thing in the same way it complains about strings (hence all our using nonEmpty all around) but it's fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, 0 isn't valid on RelationshipStateT at least (or any falsey value except null, so even if linkTypeId were '' or undefined, the || would still be valid). The only reason we store it as 0 elsewhere is 'cause we sort the sections by link type ID a lot, and it makes it easier to just do a.typeId - b.typeId in the sort functions.

I'm not sure linkTypeID: linkTypeId === 0 ? null : linkTypeId, is clearer to me -- I kinda find it harder to parse a ternary than a || null fallback, but if you prefer the explicitness of the former I could change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

it's probably not needed anyway

If “it” is about the parenthesis, any comment that can prevent us or anyone else that reads this code in the future to ask “WTF?” is needed.
If “it” is about conversion back and forth from null to 0, it is needed for sorting by (component) sections as explained by @mwiencek in the comment. But this comment can be improved too.

Copy link
Member

Choose a reason for hiding this comment

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

It was about the parens :) But if @mwiencek is ok with those, then I think they're fine.

};
}, [
canBeOrdered,
Expand Down
12 changes: 12 additions & 0 deletions root/static/scripts/relationship-editor/hooks/useRelationshipDialogContent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
* later version: http://www.gnu.org/licenses/gpl-2.0.txt
*/

import * as Sentry from '@sentry/browser';
import * as React from 'react';

import {createCoreEntityObject} from '../../common/entity2.js';
Expand Down Expand Up @@ -75,6 +76,17 @@ export default function useRelationshipDialogContent(
user,
} = options;

if (relationship.linkTypeID === 0) {
/*
* Empty link types should be stored as `null` on `RelationshipStateT`.
* We store them as `0` on `RelationshipLinkTypeGroupT`, so check to
* make sure those don't wind up here. (See MBS-12931.)
*/
Sentry.captureException(
new Error('relationship.linkTypeID is 0, but should be null'),
);
}

return React.useCallback((closeAndReturnFocus) => {
if (targetTypeOptions != null && !targetTypeOptions.length) {
/*
Expand Down