Skip to content
This repository has been archived by the owner on Apr 24, 2024. It is now read-only.

Return 409 conflict when POST /maps with taken name. close #758 #1298

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

horenso
Copy link
Contributor

@horenso horenso commented Apr 17, 2024

Summary

Return 409 conflict when creating a new map with existing name instead of a 500 server error.
Also add a simple regression test.

Basics

  • The PR is rebased with current master
  • I added a line to changelog.md
  • Details of what I changed are in the commit messages
  • References to issues, e.g. close #X, are in the commit messages and changelog
  • The buildserver is happy

Checklist

  • I fully described what my PR does in the documentation
  • I fixed all affected documentation
  • I fixed the introduction tour
  • I wrote migrations in a way that they are compatible with already present data
  • I fixed all affected decisions
  • I added automated tests or a manual test protocol
  • I added code comments, logging, and assertions as appropriate
  • I translated all strings visible to the user
  • I mentioned every code or binary not directly written or done by me in reuse syntax
  • I created left-over issues for things that are still to be done
  • Code is conforming to our Architecture
  • Code is conforming to our Guidelines
  • Code is consistent to our Design Decisions
  • Exceptions to any guidelines are documented

First Time Checklist

Review

  • I've tested the code
  • I've read through the whole code
  • I've read through the whole documentation
  • I've checked conformity to guidelines
  • I've checked conformity to requirements
  • I've checked that the requirements are tested

@horenso horenso linked an issue Apr 17, 2024 that may be closed by this pull request
@horenso
Copy link
Contributor Author

horenso commented Apr 17, 2024

jenkins build please

@horenso horenso added the please review Review by unspecified person requested label Apr 18, 2024
@markus2330 markus2330 requested review from chr-schr and badnames April 19, 2024 09:00
Copy link
Contributor

@markus2330 markus2330 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! CI did not run through and not enough reviews here yet, though.

Copy link
Contributor

@chr-schr chr-schr left a comment

Choose a reason for hiding this comment

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

Looks good to me for the backend 👍

But the frontend should display the specific error message instead of a generic one.
image

I suggest to update #758 and add a frontend task or create a left over issue.

  • I've tested the code
  • I've read through the whole code
  • I've read through the whole documentation
  • I've checked conformity to guidelines
  • I've checked conformity to requirements
  • I've checked that the requirements are tested

@markus2330
Copy link
Contributor

markus2330 commented Apr 19, 2024

Thx for the review!

I suggest to update #758 and add a frontend task or create a left over issue.

It is better to create new issues (I created #1307), extending old ones is often confusing, especially if it involves different assignee.

@chr-schr
Copy link
Contributor

jenkins build please

@chr-schr chr-schr mentioned this pull request Apr 21, 2024
@horenso horenso force-pushed the feature/horenso/758-conflict-on-new-map-with-taken-name branch from c180740 to 886a9c4 Compare April 22, 2024 21:49
@horenso horenso requested review from markus2330 and danielsteinkogler and removed request for badnames April 23, 2024 20:09
@horenso horenso self-assigned this Apr 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
please merge please review Review by unspecified person requested
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

map error message+Diesel server status 500
4 participants