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.

Sign up for GitHub

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

Jump to bottom

feat: introduce new columns for session ratings & switch to ember table #3198

Merged
merged 2 commits into from
Aug 9, 2019
Merged

feat: introduce new columns for session ratings & switch to ember table #3198

merged 2 commits into from
Aug 9, 2019

Conversation

shreyanshdwivedi
Copy link
Member

@shreyanshdwivedi shreyanshdwivedi commented Jun 26, 2019 •
edited
Loading

Fixes #3195

Short description of what this resolves:

Adds columns - | Rating | Avg Rating | No. of ratings |

Changes proposed in this pull request:

  • Modified updateRating function
  • Introduced 2 new columns - Avg Rating and No. of ratings

Screenshot_2019-08-01 Confirmed Sessions AddOwnerEvent Events Open Event

Checklist

  • I have read the Contribution & Best practices Guide.
  • My branch is up-to-date with the Upstream development branch.
  • The acceptance, integration, unit tests and linter pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@fossasia fossasia deleted a comment Jun 29, 2019
niranjan94
niranjan94 suggested changes Jun 29, 2019
View reviewed changes
Copy link
Member

@niranjan94 niranjan94 left a comment

Choose a reason for hiding this comment

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

@shreyanshdwivedi read through ember docs ... don't reinvent the wheel

app/controllers/events/view/sessions/list.js Outdated Show resolved Hide resolved
@kushthedude
Copy link
Member

@shreyanshdwivedi Status?

@shreyanshdwivedi
Copy link
Member Author

Been busy with more pressing bugs. Will work on it once they are resolved.

niranjan94
niranjan94 suggested changes Jul 4, 2019
View reviewed changes
app/controllers/events/view/sessions/list.js Outdated Show resolved Hide resolved
app/routes/events/view/sessions/list.js Outdated Show resolved Hide resolved
@fossasia fossasia deleted a comment Jul 6, 2019
@shreyanshdwivedi shreyanshdwivedi changed the title [WIP] feat: introduce new columns for session ratings feat: introduce new columns for session ratings Jul 7, 2019
@auto-label auto-label bot added the feature label Jul 7, 2019
@fossasia fossasia deleted a comment Jul 7, 2019
@shreyanshdwivedi
Copy link
Member Author

I've added the final gif to the PR description. Please review
@niranjan94 @CosmicCoder96 @uds5501 @kushthedude @mrsaicharan1

@shreyanshdwivedi shreyanshdwivedi mentioned this pull request Jul 7, 2019
Closed
niranjan94
niranjan94 suggested changes Jul 8, 2019
View reviewed changes
app/controllers/events/view/sessions/list.js Outdated Show resolved Hide resolved
app/controllers/events/view/sessions/list.js Outdated Show resolved Hide resolved
@shreyanshdwivedi shreyanshdwivedi changed the title feat: introduce new columns for session ratings [WIP] feat: introduce new columns for session ratings Jul 8, 2019
@auto-label auto-label bot removed the feature label Jul 8, 2019
@fossasia fossasia deleted a comment Jul 8, 2019
niranjan94
niranjan94 suggested changes Jul 8, 2019
View reviewed changes
app/controllers/events/view/sessions/list.js Outdated Show resolved Hide resolved
app/controllers/events/view/sessions/list.js Outdated Show resolved Hide resolved
app/controllers/events/view/sessions/list.js Outdated Show resolved Hide resolved
app/controllers/events/view/sessions/list.js Outdated Show resolved Hide resolved
@shreyanshdwivedi shreyanshdwivedi mentioned this pull request Jul 9, 2019
Closed
@fossasia fossasia deleted a comment Jul 9, 2019
@fossasia fossasia deleted a comment Jul 9, 2019
@shreyanshdwivedi
Copy link
Member Author

There is a bug in this implementation. When I rate a session the refresh isn't working properly. It is recorded to the server but the rows aren't loaded and so when I rate it again, the FE is sending to create a session rather than updating it so new instance is created on server. @niranjan94 what could be the problem here?

@shreyanshdwivedi shreyanshdwivedi mentioned this pull request Jul 27, 2019
Merged
@kushthedude
Copy link
Member

This PR is ready for review. @CosmicCoder96 @kushthedude @uds5501 @mrsaicharan1

There is a general drawback of the ember table which I felt, that after triggering an action, the FE isn't updated. It happens everywhere we used ember-table. So, when feedback is created, the FE doesn't change and the same patch is rendered which is associated with createRating action. And so, if we don't manually refresh the page after creating feedback, duplicate feedbacks are created.

@shreyanshdwivedi Try triggering ‘RefreshRoute’ action after the ratings input is taken.

@fossasia fossasia deleted a comment Aug 4, 2019
@shreyanshdwivedi
Copy link
Member Author

@kushthedude in today's meeting, @CosmicCoder96 told that this issue is persistent in almost every table we are using for now. He'll handle this in another PR
@CosmicCoder96 please review this PR

kushthedude
kushthedude suggested changes Aug 6, 2019
View reviewed changes
Copy link
Member

@kushthedude kushthedude left a comment

Choose a reason for hiding this comment

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

@shreyanshdwivedi Sorting is not working, I suggest making the change or leave the sorting for now, As Sessions Table is broke on production now due to Ember Table in Speakers and Model Table in Sessions.
So let's make this PR merge asap.

@kushthedude
Copy link
Member

@shreyanshdwivedi Are the missing columns expected?
image

@shreyanshdwivedi
Copy link
Member Author

@kushthedude they are appearing for me. Can you try by clicking on All tab again? Maybe they are not loaded properly for you.

@kushthedude
Copy link
Member

@kushthedude they are appearing for me. Can you try by clicking on All tab again? Maybe they are not loaded properly for you.

@shreyanshdwivedi I did it, Its strange when I refresh the page Track and Type is not loaded but when I click all again they loads. Please test it

@shreyanshdwivedi
Copy link
Member Author

@kushthedude I tested this and encountered this earlier but to fix it I introduced new cell-name.hbs template
But I think it's a problem with ember table then because the data which is shown is not a computed data.
@CosmicCoder96 can you please review this PR once

@kushthedude
Copy link
Member

@shreyanshdwivedi One more thing, When I refresh the page at Sessions Table, I see the Manage Events Nav-Bar again, Something might be wrong at Routes, Please see the reproducibility.
image

@shreyanshdwivedi
Copy link
Member Author

@CosmicCoder96 please have a look now. There are a few glitches but it exist in all the tables for now. Can we move ahead with this PR as it blocks one of the enhancement?

@fossasia fossasia deleted a comment Aug 6, 2019
@shreyanshdwivedi
feat: introduce new columns for session ratings
e0757f4 
add ratedSessions to controllers

update ratedSessions and remove extra vars

introduce ember-table and es6

update template and session table

updates route and helper
@fossasia fossasia deleted a comment Aug 7, 2019
abhinavk96
abhinavk96 reviewed Aug 7, 2019
View reviewed changes
Copy link
Contributor

@abhinavk96 abhinavk96 left a comment

Choose a reason for hiding this comment

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

Get it peer reviewed first. Then you can merge it.

@shreyanshdwivedi
Copy link
Member Author

@uds5501 @mrsaicharan1 @kushthedude please review this. We need to move forward with this PR for further enhancement so review this on priority please.
Thank You

mrsaicharan1
mrsaicharan1 approved these changes Aug 7, 2019
View reviewed changes
Copy link
Member

@mrsaicharan1 mrsaicharan1 left a comment

Choose a reason for hiding this comment

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

I think this will do. LGTM. Let's continue with the other parts ASAP

@fossasia fossasia deleted a comment Aug 9, 2019
@abhinavk96 abhinavk96 merged commit b273045 into fossasia:development Aug 9, 2019
@shreyanshdwivedi shreyanshdwivedi deleted the rateColumns branch August 20, 2019 15:08
@mariobehling mariobehling mentioned this pull request Jan 29, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@niranjan94 niranjan94 niranjan94 requested changes

@abhinavk96 abhinavk96 abhinavk96 approved these changes

@uds5501 uds5501 uds5501 approved these changes

@mrsaicharan1 mrsaicharan1 mrsaicharan1 approved these changes

@kushthedude kushthedude kushthedude approved these changes

Assignees
No one assigned
Labels
feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add required columns to 'Manage Events/Sessions' for rating
6 participants
@shreyanshdwivedi @kushthedude @abhinavk96 @niranjan94 @uds5501 @mrsaicharan1