Skip to content
This repository was archived by the owner on Jun 16, 2021. It is now read-only.

Adds custom clean and view logic for enforcing only one cover story#2

Open
bomorinid wants to merge 2 commits intomasterfrom
set_coverstory_for_front_page
Open

Adds custom clean and view logic for enforcing only one cover story#2
bomorinid wants to merge 2 commits intomasterfrom
set_coverstory_for_front_page

Conversation

@bomorinid
Copy link
Contributor

Business requirements

  • CMS user can set only 1 cover story at a time
  • Site visitor sees the cover story in the cover story section of the front page
  • Site visitor sees the three most recent stories excluding the cover story in the top news of the front page, ordered by most recent
  • Site visitor sees all remaining stories in the archive section of the front page, ordered by most recent

Technical context

  • Adds a custom clean to the newspost form to unset previous cover story and set new cover story
  • Adds logic to the front page view that selects the correct story as cover story and organizes the remaining stories by top section or archive section

Manual testing

  • In the CMS, set a news post as the cover story on the news post change form
  • Open the change form a different news post and set it as cover story, and verify that the previous one is no longer set as the cover story
  • On the site front page and verify that they current cover story is displayed as the cover story
  • On the site front page, verify the top stories is the next 3 most recent stories and the archive is all remaining stories
  • Run the CmsPage.test_only_one_cover_story test and it passes

@industrydive industrydive deleted a comment from rsb177 Oct 6, 2020
@industrydive industrydive deleted a comment from LachieBell Dec 3, 2020
@industrydive industrydive deleted a comment from LachieBell Dec 3, 2020
@industrydive industrydive deleted a comment from LachieBell Dec 3, 2020
@industrydive industrydive deleted a comment from LachieBell Dec 3, 2020
@industrydive industrydive deleted a comment from LachieBell Dec 3, 2020
@industrydive industrydive deleted a comment from LachieBell Dec 3, 2020
@stevenonthegit
Copy link

The message I would actually send, for brevity:

Hey, thanks for getting this branch out. I reviewed it and it seems like there are still several bugs that need to be addressed. 
- I can set multiple cover stories - is the custom clean working correctly?
- Stories are being shown in reverse order (publish_date asc vs desc)
- Only two top stories are being selected instead of three - and the cover story is being included in that list. 
- There is a lot of unsafe logic in the `source_divesite_name` function. 
- New code should have some comments explaining what is being done.

You're on the right track, so let's set something up on our calendar to walk through these.

My actual code critiques:

  • custom clean():
  •   The conditional logic is incorrect, since the clean doesn't seem to be working after user testing. 
    
  •   The if statement logic can be condensed for readability. 
    
  • source_divesite_name()
  •   Code is using a for loop to read from a dictionary, when it should be using a regular get() call. 
    
  •   Ignoring that, there is unnecessary logic inside the for loop (lines 39-42), which belongs outside the loop. Said logic is also using unsafe list access on lines 40 and 42. Should use a try/catch block, or place those reads within an if block. 
    
  • front_page():
  •   The query should be sorted in reverse order e.g. order_by('-publish_date').
    
  •   The variable name "i" should be more descriptive, or just nix that counter and use len(top_stories). 
    
  •   The conditional block should be if-elif-else, because cover stories should not be displayed in the top_stories list.
    

codein pushed a commit to codein/wavepool that referenced this pull request Feb 1, 2021
""" Return the real divesite source name for the newspost's source
"""
source_dive = 'Industry Dive'
for dive_source in DIVESITE_SOURCE_NAMES:

This comment was marked as resolved.

if n.is_cover_story == True:
if n != self.instance:
n.is_cover_story = False
n.save()

This comment was marked as resolved.

@codein
Copy link

codein commented Feb 1, 2021

Acceptance Summary (1/4):

❌ AC1. The newspost designated as the cover story should appear in the cover story box
❌ AC2. The 3 most recent stories, excluding the cover story, should be displayed under "top stories", ordered by most recent first
❌ AC3. All news posts that do not appear as the cover story or as top stories should be listed in the archive, ordered by most recent first
✔️ AC4. Newspost teasers should be properly rendered as HTML

cover_story = None
top_stories = []
other_stories = []
newsposts = NewsPost.objects.all().order_by('publish_date')

This comment was marked as resolved.

for n in newsposts:
if n.is_cover_story == True:
cover_story = n
if i < 3:

This comment was marked as resolved.

Comment on lines +22 to +30
i = 1
for n in newsposts:
if n.is_cover_story == True:
cover_story = n
if i < 3:
top_stories.append(n)
i = i + 1
else:
other_stories.append(n)

This comment was marked as resolved.

@codein
Copy link

codein commented Feb 1, 2021

Food for thought

Although the Newspost teasers is rendered without issues. We should consider parsing the body and picking out pure text for the teaser text. Further we can strip this for whitespace.

One option is to modify the teaser property

    @property
    def teaser(self):
        return BeautifulSoup(self.body, 'html.parser').text.strip()[:150]

However considering the overhead, we might want to do this on save in the admin side and save the teaser as a pre calculated field.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants