Skip to content

Don't include certificate info message templ for unavailable cert status - #199

Merged
bryanlandia merged 2 commits into
appsembler/ficus/developfrom
appsembler/ficus/dashboard-no-message-for-unavailable-cert-status
Feb 9, 2018
Merged

Don't include certificate info message templ for unavailable cert status#199
bryanlandia merged 2 commits into
appsembler/ficus/developfrom
appsembler/ficus/dashboard-no-message-for-unavailable-cert-status

Conversation

@bryanlandia

Copy link
Copy Markdown

A previous PR changed the default certificate status for open-ended courses (courses with no end date) to 'unavailable', instead of 'processing'. This got rid of the confusing message 'Final course details are being wrapped up' under the course listing on the dashboard. This goes one step further and makes sure open-ended courses that have not been completed don't produce any kind of cert. message on the dashboard. Basically, this just gets rid of a styled grey <div> with no contents under each course.

For example,

screen shot 2018-01-16 at 2 28 17 pm

@bryanlandia
bryanlandia requested a review from tkeemon January 16, 2018 22:31
@bryanlandia

Copy link
Copy Markdown
Author

In master branch of edx/edx-platform there are some fairly big refactors of dashboard/certs/progress, including adding a whole new concept of certificate available dates and a new status for earned but not available certificates.

I was looking at making this and a couple other changes upstream, but the differences with upstream master are pretty large and I don't understand yet whether my changes will be necessary going forward. ... So it is possible that this will be a short-lived change, but for now, I don't see being able to merge all of the certificates changes from Ginkgo through HEAD of master.

@bryanlandia

Copy link
Copy Markdown
Author

@bryanlandia

Copy link
Copy Markdown
Author

% endif

% if course_overview.may_certify() and cert_status:
% if course_overview.may_certify() and cert_status and cert_status.get('status', None) != 'unavailable':

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@bryanlandia since you're anticipating a problematic merge with Ginkgo, it's worth adding a comment in the code to help the engineer who's merging get know what to do with that merge. Anything would suffice:

# TODO: Toss it out when merging with Ginkgo and wait for the customer to complain about it.

☝️ is just a suggestion, reword if you'd like.

@OmarIthawi

Copy link
Copy Markdown

@bryanlandia looks good to me. I left one comment.

@tkeemon

tkeemon commented Feb 9, 2018

Copy link
Copy Markdown

I have nothing to add beyond what Omar mentioned. Go ahead and make this change, then get it merged.

(dashboard template cert display solution may have to be refactored)
@bryanlandia
bryanlandia merged commit 306cade into appsembler/ficus/develop Feb 9, 2018
@bryanlandia
bryanlandia deleted the appsembler/ficus/dashboard-no-message-for-unavailable-cert-status branch February 9, 2018 19:53
@bryanlandia

Copy link
Copy Markdown
Author

I made the same change to the template override in edx-theme-codebase here appsembler/edx-theme-codebase#7

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.

3 participants