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

Allow everything but unicode emojis #1178

Merged
merged 5 commits into from
May 19, 2022
Merged

Conversation

i-oden
Copy link
Member

@i-oden i-oden commented May 18, 2022

Description

We previously disallowed a lot of characters from specific fields and decided to go with basically the same thing for the project description. However, the Order Portal allows for all characters, and those using this often collect the order information and use this when creating the data delivery project. This causes the DDS to refuse the project because the description for example contains "invalid characters".

When looking at this I at first couldn't find what had been producing the errors before, but finally I realised that it's unicode characters and emojis that product SQLAlchemyErrors. In this PR I have changed the description validation to allow all characters except the unicode emojis/signs/characters etc.

  • Summary of the changes and the related issue
  • Relevant motivation and context
  • Any dependencies that are required for this change

Fixes DDS-1293

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

Please delete options that are not relevant.

  • Any dependent changes have been merged and published in downstream modules
  • Rebase/merge the branch which this PR is made to
  • Changes to the database schema: A new migration is included in the PR

Formatting and documentation

  • I have added a row in the changelog
  • The code follows the style guidelines of this project: Black / Prettier formatting
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings

Tests

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@i-oden i-oden requested a review from a user May 18, 2022 14:52
@i-oden i-oden self-assigned this May 18, 2022
@codecov
Copy link

codecov bot commented May 18, 2022

Codecov Report

Merging #1178 (108fbdc) into dev (c3c7906) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev    #1178      +/-   ##
==========================================
+ Coverage   83.74%   83.76%   +0.02%     
==========================================
  Files          29       29              
  Lines        3641     3641              
==========================================
+ Hits         3049     3050       +1     
+ Misses        592      591       -1     
Impacted Files Coverage Δ
dds_web/api/schemas/project_schemas.py 90.09% <ø> (+0.43%) ⬆️
dds_web/api/project.py 84.86% <100.00%> (ø)
dds_web/utils.py 77.36% <100.00%> (+0.61%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3c7906...108fbdc. Read the comment docs.

@i-oden i-oden requested review from valyo and removed request for a user May 19, 2022 05:49
Copy link
Member

@valyo valyo left a comment

Choose a reason for hiding this comment

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

I did a test in gitpod with "dds project create -t test -d "test project 😀" -pi [email protected]" and it worked as expected:
Current user: unitadmin_1
ERROR ⚠️ Failed to create project: This input is not allowed:
😀 ⚠️

Didn't run the tests locally, though

@i-oden i-oden merged commit 466e0d7 into dev May 19, 2022
@i-oden i-oden deleted the allow-everything-but-unicode branch May 19, 2022 07:45
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.

2 participants