feat: adding tick restriction for attempt type #444

PrithipalKhalsa · 2025-01-22T23:21:27Z

Addressing #411:
Restricting attemptType on bouldering to : Send, Flash or Attempt
Only Routes can be a style of: Lead, Solo, TR, Follow (aka no style for bouldering)
Solo, TR, and Follow style cannot be: Attempt, Redpoint, Pinkpoint, Flash, or Onsight

Addressing: #419
@Vichy97 I simply made this a non-required field. Which is how I interpreted this issue.

I also had to do some larger updates to the tick test files. Since adding and updating a tick requires verification of the climb style.

I am newer to graphql so if I've missed anything or broke convention please let me know :)

Vichy97 · 2025-01-23T17:16:16Z

This looks good to me from a functionality standpoint (I'll defer to others for the code review since I'm not a web developer). I think right now this can't merge until we fix MP imports though, otherwise import will always fail. I also wonder if we need a DB migration for existing ticks?

glassbead0 · 2025-01-23T21:00:41Z

src/graphql/schema/Tick.gql

+  TR
+  "Follow"
+  Follow
+}


Maybe we add 'Boulder'? It's a little counter-intuitive to me that this field is blank for boulders, IMHO setting the style to Boulder makes sense.

I don't know if I agree with that. The UI (at least on the mobile app) just won't show this field. So for boulders this field doesn't make sense at all. This is similar to how MP shows ticks for boulders

glassbead0 · 2025-01-23T21:01:36Z

@Vichy97 @PrithipalKhalsa

I'm hoping over here from OpenBeta/open-tacos#1259, which fixes MP imports and relates to this PR. I have a couple thoughts and would be happy to help out on this:

I'm going to update my PR (1259) to not use 'N/A' for style or attemptType, and just use null instead.
Regarding a migration to fix the incorrect attemptyType for all previously imported MP ticks: I don't see how to do that because whatever 'Lead Style' value came in from MP at time of import is gone (AFAICT). so right now, unless individual users have manually edited their ticks, the attemptStyle field for all MP imports is meaningless. Perhaps a migration to just null out all attemptTypes of Send that have a source of MP?
I have a strong opinion, weakly held (happy to shut up about this if nobody agrees :) ), that both MP and OB schema/naming for ticks aren't great. The 2 fields (style and attemptType), aren't consistently used between routes/boulders, and things are not named super intuitively. eg: one of the possible values for attemptType is Attempt. I know what that means, but it takes some cognitive overhead to understand it, I also wonder if we should set style to Boulder for boulders, instead of leaving it blank. I left some comments in the code in this PR with my thoughts.

glassbead0 · 2025-01-23T21:03:15Z

src/db/TickSchema.ts

-  style: { type: Schema.Types.String, required: true, default: '' },
-  attemptType: { type: Schema.Types.String, required: true, index: true, default: '' },
+  style: { type: Schema.Types.String, enum: ['Lead', 'Solo', 'TR', 'Follow'], required: false },
+  attemptType: { type: Schema.Types.String, enum: ['Onsight', 'Flash', 'Redpoint', 'Pinkpoint', 'Frenchfree', 'Fell/Hung', 'Send', 'Attempt'], required: false, index: true },


I'm not the biggest fan of the field attemptType having a possible value of Attempt. Perhaps something like Fell (for boulders), instead? We use Fell/Hung for routes, so I feel like Fell makes sense for boulders.

Also, this is perhaps an uncommon use case, but are there enough people who would want an Aid option? I like having FrenchFree as an option, and Aid can certainly be considered distinct from FrenchFree.

I'm not a boulderer so idk if fell or attempt makes more sense here. I would think some people might like attempt as opposed to fell if they got on a project just to work a particular move or sequence, but I'll defer to people who boulder more than I do.

but are there enough people who would want an Aid option

I could go either way on this. The route grade is already marked with an aid rating. Again I don't aid climb ever and I don't know if I would ever use "french free", but I am open to including it or not including it

I know attempt is pretty standard terminology, and MP certainly helped normalize it by using that term. I suspect people might have strong opinions about changing it, so I don't want to rock the boat.

I'm certainly not opposed to improving on what MP does, but I appreciate the simplicity of MP as opposed to something like 27 crags where theres like 20 options. I also know the majority of users probably will also be users of MP right now. I would lean towards mirroring MP for now, but I don't have a hard stance either way

glassbead0 · 2025-01-23T21:06:36Z

src/graphql/schema/Tick.gql

+  "Pinkpoint: Sending a route with no falls on pre-placed gear"
+  Pinkpoint
+  "French Free: Doing the route with pulling on gear or draws"
+  French_Free


In other places, you're using Frenchfree, but here's its French_Free. is that intentional? (I'm new to graphql, so maybe this is correct in this context)

That is not intentional, I'll fix that

glassbead0 · 2025-01-23T21:07:09Z

src/graphql/schema/Tick.gql

+  "French Free: Doing the route with pulling on gear or draws"
+  French_Free
+  "Fell/Hung: Falling or hanging on gear"
+  Fell_Hung


same as above Fell/Hung vs Fell_Hung

Vichy97 · 2025-01-23T21:15:22Z

Regarding a migration to fix the incorrect attemptyType for all previously imported MP ticks: I don't see how to do that because whatever 'Lead Style' value came in from MP at time of import is gone (AFAICT). so right now, unless individual users have manually edited their ticks, the attemptStyle field for all MP imports is meaningless. Perhaps a migration to just null out all attemptTypes of Send that have a source of MP?

I don't think we can migrate attemptStyle, but the attemptType fields that have already been migrated will not match what is required. New imports also won't match and will fail to import. That's my main concern WRT MP imports. Not matching fields also applies to existing ticks from OpenBeta

glassbead0 · 2025-01-23T21:20:42Z

src/model/TickDataSource.ts

+        throw new Error('Invalid attempt type for a non-lead style')
+      }
+    }
+  }


This really gets in the weeds pretty fast, but since we have the option of Fell/Hung for Lead climbs, isn't that redundant with Attempt?

On MP, Attempt can only be marked for boulders, and Fell/Hung can only be marked for roped climbs with 'Lead' style.

Actually I think we shouldn't support Fell/Hung. Keep in mind, these are not necessarily the user-facing values. I think attempt should map to Fell/Hung for climbs on the user-facing side, but stored as attempt in the server.

Vichy97 · 2025-01-23T21:31:25Z

src/db/TickTypes.ts

@@ -16,7 +16,12 @@ export type TickSource =
   */
  'MP'

+export type TickStyle = 'Lead' | 'Solo' | 'TR' | 'Follow'
+export type TickAttemptType = 'Onsight' | 'Flash' | 'Redpoint' | 'Pinkpoint' | 'Frenchfree' | 'Fell/Hung' | 'Send' | 'Attempt'


No need for Fell/Hung here. That can be the user-facing version of "Attempt" IMO

Fair.

Also If these don't need to be user facing, then i could argue we get rid of Send as well, and use Redpoint. For boulders a Redpoint would get rendered on the front end as a Send, since it's standard nomenlature not to use redpoint in bouldering (even though conceptually it's basically the same). And of course still leave 'Onsight' available for routes, and 'Flash' available for both routes and boulders.

Man.... I can see how 27 crags ends up with so many options... there actually are a lot of philosophical/linguistic/cultural issues at play here... lol.

All in all... whatever is done, I advocate for well documented code that makes it clear what means what.

I agree, although due to redpoint and send being used interchangeably with routes, I would suggest storing them both as send, and disregarding redpoint. Also since both dws, aid and tr would use 'send'.

Ya I agree. Both should be Send

I'm curious why you guys think 'send' is better to store in the db instead of 'redpoint'. I think send is generally an ambiguous term, and Onsight, Redpoint and Flash are all specific instances of a 'send' (like a hierarchy). Even in bouldering I might casually say I sent, regardless of whether I got it 1st go (flash), or 5th go (technically a redpoint). DWS and TR can also meaningfully distinguish between OS, RP and Flash. (Aid is a different story perhaps... ugh)

I do agree things should match commonly used terminology on the front end (pretty much matching MP).

In pursuit of simplicity on the backend though, I'll make one last case to not use send:

The 4 core attemptTypes of Onsight, Redpoint, Flash, and Attempt are applicable to almost everything with only one exception (no OS for boulders).

Frenchfree & Pinkpoint are slightly special cases, but straightforward to deal with.

I don't want to be the annoying opinionated guy (especially as a new contributor), but I do know that a lot of climbers care about tick data. I'll shut up about it after this comment though and I'll happily review the code 😄 Also happy to chat or pair. (I'm not actually that strongly opinionated, but I do nerd out about linguistics and databases 🤓 )

I think "send" only makes sense because "redpoint" and "pinkpoint" don't exist for boulders. I agree "send" is redundant for roped climbs and I don't plan to display it as an option in the mobile app, except for boulders.

I don't want to be the annoying opinionated guy (especially as a new contributor)

I prefer people to be opinionated! I don't want to make these decisions siloed.

If you would like to keep "RedPoint" in addition to "Send" as a valid option I'm definitely ok with that!

The PR now reflects both send and redpoint in the attempt type. I personally support both as often on multi-pitch routes a send comes with an asterisk or two. And a redpoint often implies a multisession project.

Works for me!

glassbead0 · 2025-01-23T22:30:27Z

Is there a separate issue for the corresponding front-end change? Looks like at least https://github.com/OpenBeta/open-tacos/blob/develop/src/components/users/TickForm.tsx#L36 needs to be updated to match whatever is done here.

PrithipalKhalsa · 2025-01-23T22:57:43Z

I was mostly trying to interpret #411 and I was mostly trying to reduce conflict from MP imports.
Looking at the thread here, I'll suggest this:

Type: trad, sport, alpine, ice, mixed, aid
possible Styles: 'Lead', 'Solo', 'TR', 'Follow', 'Aid'
possible attempt types for Lead:
'Onsight', 'Flash', 'Redpoint', 'Pinkpoint', 'Frenchfree', 'Send', 'Attempt'
possible attempt types for Solo, TR and Follow: 'Send', 'Flash', 'Attempt', 'Onsight'

Type: DWS, Bouldering
possible styles: no field
possible attempt types: 'Send', 'Flash', 'Attempt', 'Onsight'

Type: tr
possible Styles: TR
possible attempt types: 'Send', 'Flash', 'Attempt', 'Onsight'

Type: aid
possible Styles: aid, follow
possible attempt types: send attempt (or possibly leave this blank)

mikeschen

Just some code cleanup :)

mikeschen · 2025-01-24T14:45:55Z

src/model/TickDataSource.ts

    const rs = await this.tickModel.findOneAndUpdate(filter, updatedTick, { new: true })
    return await rs?.toObject() ?? null
  }

+  private async validateTick (tick: TickInput): Promise<void> {


Just a request, could we make code a bit more readable, i.e. :

private async validateTick(tick: TickInput): Promise<void> { const climbIdAsUUID = muuid.from(tick.climbId); const climb = await this.climbModel .findOne({ _id: climbIdAsUUID, _deleting: { $eq: null } }) .lean(); if (!climb) { throw new Error('Climb not found'); } // Check if the climb is exclusively a boulder const isBoulderingOnly = climb.type.bouldering === true && Object.keys(climb.type).every( key => key === 'bouldering' || climb.type[key] === false ); if (isBoulderingOnly) { // Validate bouldering-specific attempt type and style const validBoulderingAttempts = ['Send', 'Flash', 'Attempt']; const invalidStyles = ['Lead', 'Solo', 'Tr', 'Follow']; if ( !validBoulderingAttempts.includes(tick.attemptType) || invalidStyles.includes(tick.style) ) { throw new Error('Invalid attempt type or style for bouldering'); } } else { // Validate non-bouldering styles and attempt types const validRopedAttempts = ['Attempt', 'Redpoint', 'Pinkpoint', 'Flash', 'Onsight']; if (tick.style !== 'Lead' && validRopedAttempts.includes(tick.attemptType)) { throw new Error('Invalid attempt type for a non-lead style'); } } }

Vichy97 · 2025-01-24T16:18:05Z

I was mostly trying to interpret #411 and I was mostly trying to reduce conflict from MP imports. Looking at the thread here, I'll suggest this:

Type: trad, sport, alpine, ice, mixed, aid possible Styles: 'Lead', 'Solo', 'TR', 'Follow', 'Aid' possible attempt types for Lead: 'Onsight', 'Flash', 'Redpoint', 'Pinkpoint', 'Frenchfree', 'Send', 'Attempt' possible attempt types for Solo, TR and Follow: 'Send', 'Flash', 'Attempt', 'Onsight'

Type: DWS, Bouldering possible styles: no field possible attempt types: 'Send', 'Flash', 'Attempt', 'Onsight'

Type: tr possible Styles: TR possible attempt types: 'Send', 'Flash', 'Attempt', 'Onsight'

Type: aid possible Styles: aid, follow possible attempt types: send attempt (or possibly leave this blank)

I think this is ok, but I don't plan to display a picker for style for TR on the mobile client. This matches MP, but I also feel most people don't count TR as a "flash" or "onsight" or even really a "send"

Vichy97 · 2025-01-24T16:27:15Z

Addressing: #419
@Vichy97 I simply made this a non-required field. Which is how I interpreted this issue.

Previously these fields were a non-required string, but I believe if you submit a tick and omit either of these fields, there was an error that was thrown. I don't see anywhere in your PR that changes this logic. Is there another part of the codebase that is doing validation around this?

PrithipalKhalsa · 2025-01-24T16:37:25Z

Previously these fields were a non-required string, but I believe if you submit a tick and omit either of these fields, there was an error that was thrown. I don't see anywhere in your PR that changes this logic. Is there another part of the codebase that is doing validation around this?
that would be here src/db/TickSchema.ts
style: { type: Schema.Types.String, enum: ['Lead', 'Solo', 'TR', 'Follow'], required: false }, attemptType: { type: Schema.Types.String, enum: ['Onsight', 'Flash', 'Redpoint', 'Pinkpoint', 'Frenchfree', 'Fell/Hung', 'Send', 'Attempt'], required: false, index: true },

Vichy97 · 2025-01-24T16:44:49Z

Previously these fields were a non-required string, but I believe if you submit a tick and omit either of these fields, there was an error that was thrown. I don't see anywhere in your PR that changes this logic. Is there another part of the codebase that is doing validation around this?
that would be here src/db/TickSchema.ts
style: { type: Schema.Types.String, enum: ['Lead', 'Solo', 'TR', 'Follow'], required: false }, attemptType: { type: Schema.Types.String, enum: ['Onsight', 'Flash', 'Redpoint', 'Pinkpoint', 'Frenchfree', 'Fell/Hung', 'Send', 'Attempt'], required: false, index: true },

Ah ok makes sense. I'm not super familiar with graphql server so I missed that! Thanks

glassbead0 · 2025-01-24T21:57:40Z

I started looking at the tickForm on the frontend. Is anyone else working on matching that with the code here? if not, I'll keep looking at it, and maybe combine it with with the MP import https://github.com/OpenBeta/open-tacos/pulls/1259
@Vichy97 @PrithipalKhalsa ?

Vichy97 · 2025-01-24T21:59:59Z

Nobody is working on it. I don't even see a way to view or add ticks on the front end

glassbead0 · 2025-01-24T22:00:56Z

Also, maybe this is a separate issue, but I noticd there is a uniqueness constraint on ticks: E11000 duplicate key error collection: openbeta.ticks index: climbId_1_dateClimbed_1_style_1_userId_1_source_1

So I cant have multiple ticks on the same route, same style, same day. that means I can't log 2 lead attempts on the same route on the same day, which i i should be able to do. I get that 2 "onsights" doesnt make sense, but i think the current index is too restrictive, and maybe should just be removed?

glassbead0 · 2025-01-24T22:02:43Z

Nobody is working on it. I don't even see a way to view or add ticks on the front end

I see this on route pages: We're giving this page a facelift! Visit the previous version]to make edits. The 'previous version' does have a tick interface.... who has context on the 'new' vs 'old' route pages?

musoke · 2025-01-24T22:03:21Z

Also, maybe this is a separate issue, but I noticd there is a uniqueness

constraint on ticks: E11000 duplicate key error collection: openbeta.ticks index: climbId_1_dateClimbed_1_style_1_userId_1_source_1

So I cant have multiple ticks on the same route, same style, same day.

that means I can't log 2 lead attempts on the same route on the same day, which i i should be able to do. I get that 2 "onsights" doesnt make sense, but i think the current index is too restrictive, and maybe should just be removed? I _thought_ this was already relaxed.

…

On Fri, Jan 24, 2025, 17:01 Aaron Glasenapp ***@***.***> wrote: Also, maybe this is a separate issue, but I noticd there is a uniqueness constraint on ticks: E11000 duplicate key error collection: openbeta.ticks index: climbId_1_dateClimbed_1_style_1_userId_1_source_1 So I cant have multiple ticks on the same route, same style, same day. that means I can't log 2 lead attempts on the same route on the same day, which i i should be able to do. I get that 2 "onsights" doesnt make sense, but i think the current index is too restrictive, and maybe should just be removed? — Reply to this email directly, view it on GitHub <#444 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AD7ET7EOKWN2UEM6VQG6HUD2MKZ27AVCNFSM6AAAAABVWB42Y2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMMJTGQ2TIMZXGE> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

glassbead0 · 2025-01-25T00:13:16Z

Here's a draft of the front-end "Tick This Route" button. OpenBeta/open-tacos#1260

It doesnt' exactly sync up with the code here. I'll take another look tomorrow.

vnugent · 2025-01-25T04:14:24Z

Nobody is working on it. I don't even see a way to view or add ticks on the front end

I see this on route pages: We're giving this page a facelift! Visit the previous version]to make edits. The 'previous version' does have a tick interface.... who has context on the 'new' vs 'old' route pages?

The old climb page has inline editing (view and edit-mode are on the same page). The code is also convoluted. Last year we upgraded Next.js framework from v12 -> v13 and in the process, moving area editing into its own pages, simplifying the area view page. The climb page hasn't received the same upgrade pattern until recently. The new climb page is now view-only. We're currently missing the climb-edit page(s).

glassbead0 · 2025-01-25T21:11:15Z

src/model/TickDataSource.ts

+      if ((['Lead', 'Solo', 'Tr', 'Follow', 'Aid'].includes(tickStyle)) || ['Pinkpoint', 'Frenchfree', 'Redpoint'].includes(attemptType)) {
+        throw new Error('Invalid attempt type or style for DWS/Bouldering')
+      }
+    } else if (isTROnly) { // TopRope can only have attempt types: 'Send', 'Flash', 'Attempt', 'Onsight' and styles: 'TR'


This one doesn't quite make sense to me.

TR only routes can also be Soloed (so we should allow 'Solo' as a type in addition to TR)

I thought we weren't going to allow 'Onsight' and 'Flash' for TR routes, and only allow 'Send'? I guess I can restrict that on the front end only, but allow those types in the DB.

glassbead0 · 2025-01-25T23:34:05Z

src/graphql/schema/Tick.gql

+  "Pinkpoint: Sending a route with no falls on pre-placed gear"
+  Pinkpoint
+  "French Free: Doing the route with pulling on gear or draws"
+  FrenchFree


Frenchfree (instead of FrenchFree) to match above.

prithipal khalsa and others added 2 commits January 22, 2025 15:48

tick type enum and validation

b1f8288

remove console log

1113888

Vichy97 mentioned this pull request Jan 23, 2025

fix: use Lead Style from MP tick import instead of Style OpenBeta/open-tacos#1259

Open

6 tasks

glassbead0 reviewed Jan 23, 2025

View reviewed changes

Vichy97 reviewed Jan 23, 2025

View reviewed changes

mikeschen reviewed Jan 24, 2025

View reviewed changes

prithipal khalsa added 2 commits January 24, 2025 11:21

explicit tick check and testing

d3e4fa2

redpoint added for tick attemptType

931bb3c

glassbead0 mentioned this pull request Jan 24, 2025

feat: Restrict attemptType based on style selected for ticks OpenBeta/open-tacos#1260

Open

6 tasks

glassbead0 reviewed Jan 25, 2025

View reviewed changes

feat: adding tick restriction for attempt type #444

Are you sure you want to change the base?

feat: adding tick restriction for attempt type #444

Conversation

PrithipalKhalsa commented Jan 22, 2025

Vichy97 commented Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glassbead0 commented Jan 23, 2025 • edited Loading

glassbead0 Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vichy97 Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vichy97 commented Jan 23, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glassbead0 Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glassbead0 Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glassbead0 commented Jan 23, 2025

PrithipalKhalsa commented Jan 23, 2025 • edited Loading

mikeschen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vichy97 commented Jan 24, 2025

Vichy97 commented Jan 24, 2025

PrithipalKhalsa commented Jan 24, 2025

Vichy97 commented Jan 24, 2025

glassbead0 commented Jan 24, 2025

Vichy97 commented Jan 24, 2025

glassbead0 commented Jan 24, 2025

glassbead0 commented Jan 24, 2025

musoke commented Jan 24, 2025 via email

glassbead0 commented Jan 25, 2025

vnugent commented Jan 25, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vichy97 commented Jan 23, 2025 •

edited

Loading

glassbead0 commented Jan 23, 2025 •

edited

Loading

glassbead0 Jan 23, 2025 •

edited

Loading

Vichy97 Jan 23, 2025 •

edited

Loading

glassbead0 Jan 23, 2025 •

edited

Loading

glassbead0 Jan 24, 2025 •

edited

Loading

PrithipalKhalsa commented Jan 23, 2025 •

edited

Loading