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

feat: add is_custom in social_link model #7256

Closed
wants to merge 1 commit into from

Conversation

snitin315
Copy link
Member

@iamareebjamal
Copy link
Member

iamareebjamal commented Sep 10, 2020

We don't want this. For the backend, there is no distinction between a custom and normal link. All that should be handled in the frontend. Simply check that the identifier is among the supported links [facebook, twitter, ...] and if it is not, then it is custom. A simple getter can implement that in the frontend model

@snitin315
Copy link
Member Author

I will update 👍🏻

@iamareebjamal
Copy link
Member

Besides this will create horrible inconsistencies. What if someone sends facebook link with isCustom true or boobalooba link with isCustom false. Then the backend would need to keep set of supported linkes to check if it is custom or not. And adding a new supported link will need change in both frontend and server. Even if it was needed in server, a column would not be needed at all because just a property in the model will be able to tell if the link is custom or not. We should never add columns for data which can be derived from other data. That's redundancy and causes integrity issues. In relational data model, there must be a single source of truth. Previous devs added such fields which cause problems till today.

@codecov
Copy link

codecov bot commented Sep 10, 2020

Codecov Report

Merging #7256 into development will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##           development    #7256   +/-   ##
============================================
  Coverage        63.55%   63.55%           
============================================
  Files              259      259           
  Lines            13034    13035    +1     
============================================
+ Hits              8284     8285    +1     
  Misses            4750     4750           
Impacted Files Coverage Δ
app/models/social_link.py 72.72% <100.00%> (+1.29%) ⬆️

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 97f3497...6859e52. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants