-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added feature to allow authors to restore deleted modules #1916
Added feature to allow authors to restore deleted modules #1916
Conversation
Looks like some files fail the prettier check - run prettier and push again. |
Yep. That was expected. Some unit tests are still missing as well. I'm pushing a commit with full coverage as soon as I can. |
Just running into some issues where Jest is not generating my lcov reports now. A full coverage commit will be submitted soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, it'll definitely be an awesome feature to have once it gets in.
I think the main thing I noticed was that since the menu button still shows up in the restore modules page, you can produce an error by pressing any of the buttons inside the menu for a deleted module. Either hiding the menu or having a special "Restore" button there would work, I think.
Also, the "Restore All" button text is pushed onto two lines which slightly moves things on the page while multiselect is active, probably from padding or something like that.
Good catch! I am going to work on that now. I also found some other bugs while fixing the ones you pointed out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works well and everything looks good - only thing I could see is that one of the tests had an empty string as a description.
oboEvents.emit(Draft.EVENT_DRAFT_RESTORED, { id, userId }) | ||
}) | ||
.catch(error => { | ||
logger.logError('Draft fetchById Error', error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably log a message starting with 'Draft restoreByIdAndUser Error', but I can see that 'Draft fetchById Error' is being used twice already. Unsure if that's intentional or if it was a copy/paste and never got corrected.
If any more changes are made to this branch before it's merged in, correct these as well - if not, make a new issue for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just corrected it. No new issue is needed for now :)
@@ -1147,4 +1147,107 @@ describe('Dashboard Actions', () => { | |||
expect(error.message).toBe('Failed to check lock for module with id mockDraftId.') | |||
}) | |||
}) | |||
|
|||
test('', () => {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test needs a description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Darn it. Totally overlooked that. A new commit is coming up soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job, this'll save a lot of potential support work down the line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work, LGTM!
Fixes #1852