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

Varghese robin newspost detail fixes#10

Open
codein wants to merge 7 commits intoindustrydive:masterfrom
codein:varghese_robin_newspost_detail_fixes
Open

Varghese robin newspost detail fixes#10
codein wants to merge 7 commits intoindustrydive:masterfrom
codein:varghese_robin_newspost_detail_fixes

Conversation

@codein
Copy link

@codein codein commented Feb 1, 2021

Comments and changes for Fixes #1, #2 and #4 are encapsulated in there individual commits.

Fix #3 is spread into three different commits in an attempt to pass test_newspost_page_content
The first one ads ids to templates, since it was previously not mentioned on the template.
Second modifies the test to compare text and not markedup text to rendered_body text as before.
Third there seems to be a typo in the fixture with missing paired <p> tag.

Lastly i have some minor verbiage changes to two typos in last commit

<link href="{% static 'bootstrap/css/bootstrap.min.css' %}" rel="stylesheet">
<link href="{% static 'app.css' %}" rel="stylesheet">
<title>Wavepool | Industry Dive</title>
<title>{{ newspost.title }} | Wavepool | Industry Dive</title>
Copy link
Author

Choose a reason for hiding this comment

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

This fixes was made to include the newspost.title in the template as required by test_newspost_page_title_attribute

Comment on lines +41 to +44

@property
def edit_url(self):
return reverse('admin:wavepool_newspost_change', args=[self.pk]) No newline at end of file
Copy link
Author

Choose a reason for hiding this comment

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

Here i Introduced a edit_url property that is eventually used in newspost.html to surface the edit URL to CMS users

Comment on lines +22 to +26
{% if is_staff %}
<div class="row">
<a id="edit-link" href="{{ newspost.edit_url }}"><button>edit</button></a>
</div>
{% endif %}
Copy link
Author

Choose a reason for hiding this comment

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

This conditional template shows edit_url property only when a logged in user has is_staff flag set. The is_staffflag is set into context in the view.py

context = {
'newspost': newspost
'newspost': newspost,
'is_staff': request.user.is_staff
Copy link
Author

Choose a reason for hiding this comment

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

Here we set the is_staff flag based on current requesting user

Comment on lines +35 to +38
# Implementation based on naive assumption that URL is in a
# <protocol>://www.<hostname>.xyz/asdf-asdf-asdf format
hostname = self.source.split('/')[2].split('.')[1]
return DIVESITE_SOURCE_NAMES[hostname]
Copy link
Author

Choose a reason for hiding this comment

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

source_divesite_name was hardcoded before. I have modified this to resolve the hostname from the URL and then return the lookup from DIVESITE_SOURCE_NAMES

Copy link
Author

@codein codein Feb 1, 2021

Choose a reason for hiding this comment

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

i really like and prefer an implementation similar to ref in branch set_coverstory_for_front_page

    def source_divesite_name(self):
        return 'Industry Dive'
        """ Return the real divesite source name for the newspost's source
        """
        source_dive = 'Industry Dive'

        url_parts = self.source.split('/')
        dive_domain = url_parts[2]
        domain_parts = dive_domain.split('.')
        dive_domain = domain_parts[1]
        if dive_domain in DIVESITE_SOURCE_NAMES:
            return DIVESITE_SOURCE_NAMES[dive_domain]
        return source_dive

However i didn't want to just copy it

@codein
Copy link
Author

codein commented Feb 1, 2021

Kindly find attached along is a PDF with the pseudo code, a diagram/sketch, and a list of technical sub-tasks needed to fullfill the request.

Varghese Robin Code Design - Customize News Post Ads.pdf

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.

2 participants