Skip to content
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

Instructor: home page: give menu option to edit a course #8541 #8725

Merged
merged 4 commits into from
Apr 10, 2018

Conversation

jacoblipech
Copy link
Contributor

Fixes #8541

Outline of Solution
In the instructor homepage, I added the logic of "edit/view" button for instructors to courses as both edit courses and edit instructors are in the same page with the same URL. If in the future the edit instructors and edit courses are in different pages, then we can edit the button for courses.

screen shot 2018-03-28 at 12 23 47 am

@jacoblipech
Copy link
Contributor Author

Hi anyone can help with the test cases? The fail test cases mainly come from StudentHomePageUiTest, StudentAdminPageUiTest, InstructorFeedbackQuestionCopyActionTest. But I only changed the instructor home page, any idea where the failing test case may be from so that I can make the changes?

@craaaa
Copy link
Contributor

craaaa commented Mar 28, 2018

You'll have to run God mode on the failing UI tests to ensure that the expected DOM structure is updated to match the changes you made.

InstructorFeedbackQuestionCopyActionTest should not fail; Travis shows that rerunning it allows it to pass.

@craaaa craaaa added the s.Ongoing The PR is being worked on by the author(s) label Mar 28, 2018
@craaaa craaaa self-assigned this Mar 28, 2018
@craaaa craaaa self-requested a review March 28, 2018 11:11
@jacoblipech
Copy link
Contributor Author

Thanks for your tip! I am trying to run with GodMode but the test cases causing the problem either failed or ignored and I cannot seem to make the necessary changes to UI.
I rebased to the latest version, run npm build, and do the tests and it is still the same problem. I do not have firefox installed. Is this the issue? Or am I missing out on something? Hope you can help thanks!

@bqnguyen94
Copy link
Contributor

@jacoblipech Can you give the stack trace of the failed tests? Also, do the test suites pass in master for you?

@jacoblipech
Copy link
Contributor Author

hi @bqnguyen94 I am really sorry for the delay but after a few days of trying, I have finally figured out the problem. GodMode works after I download firefox v46.0 to run the test. And I realised that I should not run it multiple times as the values for time will keep changing.

Now I have a much better idea on how Teammates work. Hopefully you can help me to review again? Thank you so much.

@wkurniawan07
Copy link
Member

GodMode works after I download firefox v46.0 to run the test.

@jacoblipech what browser did you use before (and fail)?

And I realised that I should not run it multiple times as the values for time will keep changing.

Are you sure? Godmode is supposedly immune to this kind of change. Which tests are you running and where does it fail?

@jacoblipech
Copy link
Contributor Author

@wkurniawan07 I ran GodMode using the latest version of Firefox which is v59.0. Basically the Firefox browser opens up and nothing changes after that. The UI tests are still ignored. After changing to Firefox v46.0, all the tests run and works well.

Actually, I ran the tests over all CI Tests using GodMode. Somehow, GodMode updated the UI to the actual time I ran the tests instead of being data-original-title="${datetime.now}".

@craaaa
Copy link
Contributor

craaaa commented Apr 2, 2018

Hi @jacoblipech, can you rebase your branch onto master and force push? There are unrelated commits being captured here.

@whipermr5
Copy link
Member

Actually, I ran the tests over all CI Tests using GodMode. Somehow, GodMode updated the UI to the actual time I ran the tests instead of being data-original-title="${datetime.now}".

Sounds like #8640

@wkurniawan07
Copy link
Member

@jacoblipech how exactly do you keep your fork in sync? You're not supposed to cherry-pick the commits one by one...

@jacoblipech
Copy link
Contributor Author

@wkurniawan07 I am so sorry. Actually I used my master to pull from the teammates repo, then I rebase my branch onto the master and push. Then it caused all the commits from other people shown as well. I will look into the issue and solve it. Thanks.

@jacoblipech jacoblipech force-pushed the 8541-add-course-menu-button branch 2 times, most recently from 692669d to 9711ea7 Compare April 3, 2018 13:56
@jacoblipech
Copy link
Contributor Author

Sorry for the delay. I think this PR is ready for review. Thank You.

@craaaa
Copy link
Contributor

craaaa commented Apr 5, 2018

@jacoblipech I think you reverted your change in 9ed045a.

I pulled your branch and the View/Edit button does not appear.
screen shot 2018-04-05 at 12 38 22 pm

At this point it might be best to start on a fresh branch since there are duplicate commits, commit hashes are no longer consistent and the original change can quite easily be replicated.

@craaaa
Copy link
Contributor

craaaa commented Apr 5, 2018

Also, you may want to double-check your git config settings since your identity looks a little weird to git and GitHub. It shows up in the patch as

Jacob Li Peng Cheng <JacobLI@229-41.priv23.nus.edu.sg>
Jacob Li Peng Cheng <JacobLI@Jacobs-MacBook-Pro-2.local>

Need to run this inside Git Bash

git config --global user.name "John Doe"
git config --global user.email johndoe@example.com

Change the fields to your name and email.

@jacoblipech
Copy link
Contributor Author

@craaaa and @darrenwee I see. Thanks a lot for the help. I will make the necessary changes.

@jacoblipech jacoblipech force-pushed the 8541-add-course-menu-button branch 2 times, most recently from 9d3b1ab to c33c53d Compare April 5, 2018 12:35
@jacoblipech
Copy link
Contributor Author

jacoblipech commented Apr 6, 2018

@craaaa I think this is ready for review. Thank you :)

Copy link
Contributor

@craaaa craaaa left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for contributing 👍

@craaaa craaaa added the s.FinalReview The PR is ready for final review label Apr 6, 2018
@craaaa craaaa removed the s.Ongoing The PR is being worked on by the author(s) label Apr 6, 2018
@craaaa craaaa requested a review from wkurniawan07 April 6, 2018 17:58
@wkurniawan07 wkurniawan07 added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.FinalReview The PR is ready for final review labels Apr 10, 2018
@whipermr5 whipermr5 added this to the V6.5.0 milestone Apr 10, 2018
@whipermr5 whipermr5 added the e.1 label Apr 10, 2018
@whipermr5 whipermr5 merged commit 25008d1 into TEAMMATES:master Apr 10, 2018
@whipermr5 whipermr5 added the c.Feature User-facing feature; can be new feature or enhancement to existing feature label Apr 14, 2018
jacoblipech added a commit to jacoblipech/teammates that referenced this pull request May 14, 2018
jacoblipech added a commit to jacoblipech/teammates that referenced this pull request Feb 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c.Feature User-facing feature; can be new feature or enhancement to existing feature s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants