Cover image by Gabriel Valdez on Unsplash
The end is near! One week remains of the 12 week long MLH Fellowship - the best learning experience of my life so far. Over the last 3 months, I had the amazing opportunity to contribute to Forem - the very platform that DEV is built on. I thought about sharing my experience and work, and what better platform to do it on than the one I contributed to. Here I present to you my beautiful Forem Adventures!
My First PR
What does one do when they just begin contributing to a software? They look for good first issues of course! This issue seemed like it was the right fit!
Strikethrough markdown within a link is not rendered in live comments (but works fine elsewhere)
#13935
Describe the bug While using Strikethrough markdown within link text, I discovered the strikethrough is not rendered in live comment views only; this appears to work elsewhere (comment previews, posts, and post previews).
To Reproduce
Add strikethrough text within a link to a comment:
[~~strikethrough~~](https://www.example.com)
This looks fine in posts, post previews, and comment previews:
...but not in a submitted comment:
Expected behavior
Strikethrough markdown within link text is rendered correctly.
Desktop (please complete the following information):
- OS, version: Fedora Core 33
- Browser, version: Firefox 88.0.1 and Chromium 91.0.4472.77
This issue reports a bug in how the markdown is processed in the comments, the strikethrough text wouldn't render correctly as per markdown rules - ~~text~~ within a link. I looked into the code base to find where the problem was. This was really challenging, I had underestimated how huge Forem is and took time understanding the code base. That is why you must always pick up an easy issue as your first one, it's more about understanding the code base than writing code. This issue helped me a lot to explore a lot of corners and understand the Forem architecture. I eventually found out that a method that shortened URLs in the comments was causing html tags to be stripped out. I fixed that and raised PR 14103, my first PR to a huge repo, and my first bug fix.
Except the bug wasn't solved
Even after the PR got merged, it kept bugging me that my fix wouldn't resolve all edge cases, I tried it out in the production and indeed, I found more bugs. Which leads me to -
My First Issue
I raised an issue relating to the bug I found while working on the above PR. I found out that only the bug relating to strikethrough text was solved, but the comments were stripping out all sorts of text formatting, which leaves the user just plain text at disposal ;-; .
shorten_urls causing link text in comments to not render correctly
#14114
Describe the bug
In issue #13935, @andygeorge
reported that strikethrough text within links was rendered as normal text in live comments. I found out that the method shorten_urls in models/comment.rb was causing the issue and raised PR #14103 fixing it. However, I later discovered that no text formatting (bold, italics etc) is rendered within link text in comments.
To Reproduce Add text formatting within link text in comments:
[__bold-text__](https://www.example.com)
[~~strikethrough~~](https://www.example.com)
[_italic_](https://www.example.com)
[___bold-italic___](https://www.example.com)
This looks fine in posts and comment previews:
But isn't rendered in comments (Only strikethrough works because #14103 got merged):
Expected behavior Text within links should be formatted correctly in comments.
Desktop (please complete the following information):
- OS, version: POP_OS! 20.10
- Browser, version: Firefox
Additional context
The bug occurs because shorten_urls in models/comment.rb strips out all html tags within anchor tag in the html text processed from markdown. The method is used to truncate URLs longer than ~37 characters.
In my opinion, the best way to fix this issue is to limit shorten_urls to only truncate URLs and not custom link text. Doing so will also prevent any useful text from getting truncated.
potential bug: comment preview looks different from the final comment, because URLs aren't shortened in previews. This could be raised as another issue.
Since I had already worked on it earlier and had an idea how to solve this, I worked on the fix for it and raised PR 14240 . I tested it out, maintainers reviewed it, it got pushed to prod and I called it a day.
Except it wasn't solved yet, which leads me to -
The bug I pushed to Prod
Yeah, this is embarrassing. Even though I thought I had thoroughly tested my code, both manually and automatically but I still had to try it out in Production, on DEV.to. I happened to come across a bug, which was produced due to the changes I made. Fortunately, I noticed it soon enough, and had the PR reverted -
Comment for
#14240
@jgaskins
I was worried about the unexpected bugs, and it appears that a bug has occurred :( . Images in the comments don't get rendered as images but as plain links.
E.g
 gets rendered as -
Images were working fine when I tested it on local, but that would be because localhost urls are structured differently. I am extremely sorry about this, should've tested it thoroughly. I'll fix it as soon as possible. In the mean time, the changes should best be reverted.
Update: I have fixed the bug. Shall I revert the changes and create a new PR? Again, apologies for the inconvenience
Eventually, I had this fixed with PR 14262. This time I was sure to test it thoroughly and finally closed the issue.
If you were to comment anywhere on DEV now, you can use all sorts of formatting within links and it'll work its magic!
My Second Bug Fix
Oh boy, this was a tough one. And my favorite one to work on. I present to you the RSS Feed Dilemma -
Dashboard not updated correctly if two people have the same RSS feed
#13185
If two people share the same RSS Feed URL in Settings -> Publishing to DEV Community from RSS
Then new post will appear randomly only in 1 feed.
- Have two separated dev.to accounts
- Go to
Settings->Publishing to DEV Community from RSSand setup the sameRSS Feed URLin both - Create a new RSS entry
- The entry is show in one of the two dev.to account's (say account A) Dashboard and not in the other (account B)
- If the account who doesn't see the RSS feed (account B) tries creating a new post mentioning the new entry's canonical_url it gets a message
canonical_url: has already been taken - If the account who does see the RSS entry (account A) deletes the draft, then Account B can publish successfully the content
Expected behavior
Both dev.to accounts should see the new RSS entry in the dashboard
Screenshots
Desktop (please complete the following information):
- OS, version: MacOS Big Sur 11.2.3
- Browser, version: tried with Safari, Chrome and Firefox
Smartphone (please complete the following information):
- Device:
- OS, version:
- Browser, version:
Additional context
If you are unaware about RSS Feeds - they are xml files that summarize your webpages which lets you import your data to other webapps. They are a simple solution to achieve cross app communication.
Forem uses this feature to allow users to import their blogs from other blogging websites like medium, and other forem instances as well. This works by setting your Source RSS Feed URL in Settings -> Extensions.
It works fine when only one user is accessing a certain feed, but since RSS feeds are public, it is entirely possible for other users on a Forem instance to import blogs from the same feed as well. In this case, an article from the feed would get imported only to one of those users randomly, because of how Forem's post settings work. It made sense too, since multiple imports will lead to duplication. But there was no way to verify the true owner of the RSS feed, and no apparent way for the owner to know why some articles won't get imported. A fix was required, or just a way to let the users know what was going on. However, it was hard to decide what to do, whilst preserving content uniqueness.
I explained this issue to the maintainers, and together we decided that the best approach would be to allow duplicate draft posts, but only one of them be allowed to publish. If the post has already been published, another user who tries to do the same will get a useful error message and be suggested to contact the core team. Content uniqueness preserved, users not ghosted anymore, everyone happy. I closed this issue with PR 14160.
To learn more about the dilemma -
Comment for
#13185
tl;dr : I played around with two dev accounts and based on my observations, some issues will occur regardless of whether we fix this issue or not. My observations might be incorrect, correct me if I'm wrong.
Going through the PRs mentioned by @rhymes helped a lot. It's quite obvious now why canonical_url needs to be unique. I played around with importing from the same RSS feed from two dev accounts and this is what I observed:
- If an article from the feed was already imported in one account A, it won't be imported in account B regardless of whether the canonical_url was set or not, or if it was draft/published.
- If the article is deleted from account A, it can be fetched in account B.
Note: These observations are from using the Fetch feed now button, and not from periodic feed imports.
I can think of following issues that will occur:
If we allow both accounts to fetch the same article:
- None of them gets to publish the post if the canonical_url is same. They can be published by changing it, but since there is no way to know which account is using it, the canonical_url might remain unused.
If we allow only one account to fetch an article (as is the case right now):
- There's a UX issue where user won't know why articles aren't being fetched from the feed.
- Articles are being fetched on first come first serve basis, which defeats the purpose of allowing 2 accounts to import from the same feed.
I'll definitely be writing a more detailed post on this one, but before then I will find myself using the feeds import feature to import this post over to other Forems, hopefully someone doesn't beat me to it 👀.
Other super cool stuff
I wish to write detailed walk-throughs for all my PRs at Forem in the hopes that other contributors might find something of use from my adventures. But until then here are some honorable mentions -
Automatic Image Deletion
Working on this issue made us discover a security issue that would arise by deleting images, and even though this could not get merged, I learned a lot about data storage by going through the code base.
Automatically delete Cloudinary's user uploaded images in articles
#14141
What type of PR is this? (check all applicable)
- [x] Optimization
Description
Currently, for articles and collections, the user uploaded images persist in the database, even after the article/collection using that image is deleted. This PR adds the functions required to delete user uploaded images (to cloudinary) in the article if any.
Related Tickets & Documents
Resolves #13936
Added/updated tests?
- [x] Yes
Updating tags in articles on tag update
The issue was that if tag colors are updated by the admin, that change isn't reflected at other places because the colors are cached. I learned about how client side and server side caching works in Forem and was able to locate the bug. Easy peasy
Delete tag colors cache on updating tags.
#14427
What type of PR is this? (check all applicable)
- [x] Bug Fix
Description
- In reference to issue #12486, I raised a PR #14298 which focused on deleting article edge cache when tags are updated, so that the new tag colors maybe reflected. The issue however was not the client side cache, it was the server side cache key
view-helper-#{tag}that stores the colors of the tag. Deleting the cache at that key solves the issue.
Related Tickets & Documents
- Resolves #12486
- https://github.com/forem/forem/blob/de852bd466ef418ac07a6317de4af6f8b892338d/app/helpers/application_helper.rb#L89
Added/updated tests?
- [x] No, and this is why: This was a one line fix and is already tested by
spec/models/tag_spec.rb:119
What gif best describes this PR or how it makes you feel?
The Sponsors Image bug
Fix Sponsors Image display in homefeed and sponsors page.
#14494
What type of PR is this? (check all applicable)
- [x] Bug Fix
Description
- As discussed in #14291, there was an bug with rendering the organizations logo while featuring sponsors in the homefeed. While inspecting, I found that the same issue exists in
/sponsorspage as well. This PR fixes the bug.
Related Tickets & Documents
- Resolves #14291
QA Instructions, Screenshots, Recordings
Added/updated tests?
- [x] No, and this is why: a fallback has been added (profile image of org is displayed if they haven't uploaded a logo) and tests for the same aren't needed. Thought I should add tests anyway but realized that I won't be able to because of how the image uploader works.
What gif best describes this PR or how it makes you feel?
happy bug smashin everyone!
Refactoring and reorganizing code
I worked on a part of refactoring for StoriesController and highly recommend new contributors to try their hands on this issue. It's the best way to get familiarized with the code base and learn about what goes where and when a certain method is called.
StoriesController refactoring
#2914
Currently, StoriesController has a lot of responsibilities, such as:
- rendering homepage
- rendering articles by tag
- articles by time
- articles search
- article page
- user profile
- user comments page
- organization page
- podcast episode page
- podcast episodes list
These responsibilities should be separated and placed into their own controllers because:
- the code would be easier to understand
- easier to debug
- easier to reason about performance
Approach
Ideally we want to take each of the responsibilities this single controller has right now and break them off into their own controller with their own route. I would imagine the workflow would go something like
- Add new route for supporting new url (ie /users/:id/comments)
- Replace existing links all over the codebase
- Investigate: what happens with legacy url? should we remove or support it?
- Take into account that these urls could be bookmarked or shared through social media by many users. should we support legacy url and redirect to new url ??
I've started doing some work on it, e.g. reorganized the views (#1584, #1317), but most of the work needs to be done. It seems like a big non-urgent task to consider doing. I suppose, that rails route constraints can be used to deal with the routes.
Takeaway
- I drastically increased my knowledge on system architecture and Rails practices by going through Forem's codebase.
- I learned about some amazing tools used in the codebase like zonebie, sidekiq, and RSS feeds that I am definitely going to try out in future projects.
- Before working on the RSS feed conflict and auto image deletion, I had previously never had to think about the social implications of certain changes in code, and it's amazing to have learned that.
- The whole experience definitely made me signficantly better at exploring large codebases to pin point bugs and make appropriate changes.
Shoutouts
To @coffeecraftcode for helping me with all my troubles throughout the fellowship and the great conversations over maintainer meets.
To @khattakdev 2.0 for being an amazing pod lead. Our Standups and 1-on-1s will be missed 🥺.
To all the maintainers at Forem that closely reviewed my code and made sure I was giving my best. I learned a lot about the ins and outs of Rails and best coding practices through the reviews.
Oldest comments (0)