openstreetmap-website icon indicating copy to clipboard operation
openstreetmap-website copied to clipboard

Redaction title can be blank.

Open RoyallDesigns opened this issue 6 years ago • 10 comments

@1v0ryh4t and I found that when creating a new redaction under /redactions/new, the redaction title is allowed to be blank. When attempting to view said redactions under /redactions, the new redaction is not present or viewable under the list as its title or name is an empty string. I would like to add a validation to a redaction so that when creating or editing the redaction, the name can never be blank. (This seems like a decent good-first-issue for myself.)

Please see the attached images.

notitleredactioncreation redactionsscreen

All redactions (/redactions):

redactions_list

RoyallDesigns avatar Oct 24 '18 00:10 RoyallDesigns

I think it's reasonable to require a title, should we also consider making the description required too?

I checked the original pull request for adding redactions, and I couldn't find any discussion as to whether titles and/or descriptions should be optional. I'd be interested in feedback from any openstreetmap.org moderators, who have the most experience with creating redactions. Personally I think it would be fine to require both attributes to be present.

gravitystorm avatar Oct 24 '18 07:10 gravitystorm

I think it's clear a title should be required and I'd be inclined to say a description should be as well.

I'm surprised I didn't insist on it at the time - it must have slipped by me when I was reviewing this.

tomhughes avatar Oct 24 '18 07:10 tomhughes

Does this still need to be worked on?

rileythomp avatar Nov 11 '19 16:11 rileythomp

I'd be interested in feedback from any openstreetmap.org moderators

Creating new redactions isn't something that we need to do that often, thankfully -the last one I did was a couple of years ago I think - https://www.openstreetmap.org/redactions/102 . I'd always expect to use a title and description.

SomeoneElseOSM avatar Nov 11 '19 16:11 SomeoneElseOSM

is this issue open can i work on it

sonukushwaha403 avatar Sep 09 '20 06:09 sonukushwaha403

is this issue open can i work on it

Yes, the issue is open and can be worked on. Please also read through the previous pull request for more information.

gravitystorm avatar Sep 16 '20 09:09 gravitystorm

@BLSowmya Yes, the issue is open and you are welcome to work on it.

gravitystorm avatar Nov 09 '23 10:11 gravitystorm

Hey @gravitystorm @tomhughes If this issue has not been picked up by anyone, I'm going to work on it, after reading the previous PR comments I want confirmation about some things:

  1. We want ensure the presence of Title and Description both,
  2. For missing titles and description we need fill them up with the id of the redaction?
  3. In a single migration we have to backfill our data and change the column null to false right?
  4. Since we are using strong migrations do we need to change null first and then Validate it inside a seperate migration as stated by the strong migration documentation?

IshmeetSingh06 avatar Jan 19 '24 18:01 IshmeetSingh06

We want ensure the presence of Title and Description both,

Yes please

For missing titles and description we need fill them up with the id of the redaction?

I would suggest "Redaction #{id}" (i.e. the word "Redaction" followed by the id number) as the title, and "No description" as the description. But it's not super important since I don't think there are many (any?) on the osm.org servers. It might affect other deployments though.

In a single migration we have to backfill our data and change the column null to false right? Since we are using strong migrations do we need to change null first and then Validate it inside a seperate migration as stated by the strong migration documentation?

Yes, whichever approach works best with strong migrations. See https://github.com/ankane/strong_migrations?tab=readme-ov-file#setting-not-null-on-an-existing-column

Thanks for looking into this!

gravitystorm avatar Jan 31 '24 15:01 gravitystorm

@gravitystorm Thanks for the clarification's I'll start working on it

IshmeetSingh06 avatar Jan 31 '24 17:01 IshmeetSingh06

Model validations were added in 873ac155cabfb1aab8d887d07bc8389617d3f843 without realising that they fixed this.

tomhughes avatar Mar 07 '24 20:03 tomhughes