Skip to content

Peter fogg/notification stories - #415

Merged
peter-fogg merged 7 commits into
masterfrom
peter-fogg/notification-stories
Jul 23, 2013
Merged

Peter fogg/notification stories#415
peter-fogg merged 7 commits into
masterfrom
peter-fogg/notification-stories

Conversation

@peter-fogg

Copy link
Copy Markdown
Contributor

Studio YZ, F, and E -- component save/delete, course outline delete, and grading status change notifications.

@singingwolfboy and @cahrens are out for now, but @talbs, could you check out the interactions when you've got a chance?

@chrisndodge

Copy link
Copy Markdown
Contributor

@peter-fogg seems like this build status got stuck. Maybe check in with @wedaly or try to kick off another build?!?

@peter-fogg

Copy link
Copy Markdown
Contributor Author

@chrisndodge Just rebased onto master, that should start another build.

@talbs

talbs commented Jul 17, 2013

Copy link
Copy Markdown
Contributor

@peter-fogg, thanks for the good work.

  • I took a spin through and the deletion and saving/mini-deleting UI looks good.
  • I made some inpromptu changes to the copy and "type" (warning instead of confirm) to each prompt

Assuming, there are no objections to my quick changes, this looks good from my UX/design perspective. 👍

Are there any tests or other infrastructure that needs to be updated? Also, would you want to wait till @cahrens or @singingwolfboy are back for a technical review?

@peter-fogg

Copy link
Copy Markdown
Contributor Author

@talbs Thanks -- I was going to wait for one of them to review before merging.

@peter-fogg

Copy link
Copy Markdown
Contributor Author

@talbs Also, it seems like the asset delete confirmation should use the Warning type as well, no? https://github.com/edx/edx-platform/pull/432

@talbs

talbs commented Jul 18, 2013

Copy link
Copy Markdown
Contributor

@peter-fogg, yes - definitely. That was on my list to make a separate branch/pull request to fix since it didn't pertain to the notification stories you tackled here. Your call on whether to roll it in here or I can keep on with my separate plan.

@peter-fogg

Copy link
Copy Markdown
Contributor Author

OK. I've got a PR in already (#432) -- build is failing right now because master is broken, but I'll rebase with those changes when it's all fixed.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remove commented out line?

@cahrens

cahrens commented Jul 22, 2013

Copy link
Copy Markdown

I checked out the branch and tested it.

Just a minor cleanup-item and a request for an additional test. Otherwise 👍

@cahrens

cahrens commented Jul 22, 2013

Copy link
Copy Markdown

One other thing-- make sure all acceptance tests are passing. I was seeing some failures/skipped test points locally (including one of the new tests).

@cahrens

cahrens commented Jul 22, 2013

Copy link
Copy Markdown

When I run the test cms/djangoapps/contentstore/features/problem-editor.feature, I am getting server errors ("Studio is having trouble saving your work") showing on the page on Save (and I am not seeing "Saving..."). However, the tests all pass, so I guess the changes really are getting persisted.... Can you investigate this?

I do not see this issue when interactively doing the same thing.

@peter-fogg

Copy link
Copy Markdown
Contributor Author

Had to rebase onto master to fix rake. The acceptance tests seem OK to me -- I'm not seeing the "Studio is having trouble" message.

@cahrens

cahrens commented Jul 23, 2013

Copy link
Copy Markdown

👍 Good to merge. I'm seeing the same issue on master locally with problem-editor.feature.

peter-fogg pushed a commit that referenced this pull request Jul 23, 2013
@peter-fogg
peter-fogg merged commit 6eac259 into master Jul 23, 2013
@peter-fogg
peter-fogg deleted the peter-fogg/notification-stories branch July 23, 2013 16:05
chrisrossi pushed a commit to jazkarta/edx-platform that referenced this pull request Mar 31, 2014
Kelketek referenced this pull request in open-craft/openedx-platform May 15, 2015
Added threads read to export of discussion participation.
yokose-ks added a commit to nttks/edx-platform that referenced this pull request Oct 15, 2015
…nslation-for-gacco-cypress

Fix translation for enter-course button and help modal (openedx#409)
diegomillan pushed a commit to eduNEXT/edx-platform that referenced this pull request Sep 14, 2016
* stv/devstack/disable-django-pipeline:
  Disable Django pipeline compression in devstack
jfavellar90 pushed a commit to eduNEXT/edx-platform that referenced this pull request Apr 11, 2018
 add async view to recap instructor dash
dgamanenko referenced this pull request in raccoongang/edx-platform Jun 14, 2018
 add async view to recap instructor dash
xavierchan pushed a commit to xavierchan/edx-platform-1 that referenced this pull request May 24, 2019
andrey-canon added a commit to eduNEXT/edx-platform that referenced this pull request Jan 19, 2021
Sujeet1379 pushed a commit to chandrudev/edx-platform that referenced this pull request Nov 17, 2022
…edx#415)

Much of the logic is copied from the course exit certificate states.
AA-719
Danyal-Faheem pushed a commit to Danyal-Faheem/edx-platform that referenced this pull request Jul 15, 2025
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.

4 participants