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

Fix #10055: Remove unneeded optional properties #10093

Merged
merged 9 commits into from Aug 13, 2020
Merged

Fix #10055: Remove unneeded optional properties #10093

merged 9 commits into from Aug 13, 2020

Conversation

nishantwrp
Copy link
Contributor

@nishantwrp nishantwrp commented Jul 28, 2020

Overview

  1. This PR fixes Get rid of unnecessary optional properties #10055
  2. This PR does the following: Remove question marks in customization-args-defs.ts

Essential Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The linter/Karma presubmit checks have passed locally on your machine.
  • "Allow edits from maintainers" is checked. (See here for instructions on how to enable it.)
    • This lets reviewers restart your CircleCI tests for you.
  • The PR is made from a branch that's not called "develop".

PR Pointers

  • Oppiabot will notify you when you don't add a PR_CHANGELOG label. If you are unable to do so, please @-mention a code owner (who will be in the Reviewers list), or ask on Gitter.
  • For what code owners will expect, see the Code Owner's wiki page.
  • Make sure your PR follows conventions in the style guide, otherwise this will lead to review delays
  • Never force push. If you do, your PR will be closed.

@oppiabot
Copy link

oppiabot bot commented Jul 28, 2020

Hi, @nishantwrp, this pull request does not have a "CHANGELOG: ..." label as mentioned in the PR checkbox list. Please add this label. PRs without this label will not be merged. If you are unsure of which label to add, please ask the reviewers for guidance. Thanks!

@oppiabot
Copy link

oppiabot bot commented Jul 28, 2020

Assigning @srijanreddy98 for the first-pass review of this pull request. Thanks!

Copy link
Member

@vojtechjelinek vojtechjelinek left a comment

LGTM! Just one nit.

@@ -85,6 +85,8 @@ describe('ContinueValidationService', () => {

expect(() => {
validatorService.getAllWarnings(
// TS ignore is needed here for testing purposes.
Copy link
Member

@vojtechjelinek vojtechjelinek Jul 28, 2020

Choose a reason for hiding this comment

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

I think that the comment should be more descriptive.

Copy link
Member

@vojtechjelinek vojtechjelinek Jul 28, 2020

Choose a reason for hiding this comment

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

Ditto below.

Copy link
Member

@seanlip seanlip Jul 29, 2020

Choose a reason for hiding this comment

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

Strongly agree. Please see comments in #10088.

Copy link
Contributor Author

@nishantwrp nishantwrp Aug 2, 2020

Choose a reason for hiding this comment

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

done

@vojtechjelinek vojtechjelinek removed their assignment Jul 28, 2020
aks681
aks681 approved these changes Jul 29, 2020
Copy link
Member

@aks681 aks681 left a comment

Lgtm as codeowner

@aks681 aks681 removed their assignment Jul 29, 2020
Copy link
Member

@seanlip seanlip left a comment

Please note that there seem to be quite a few "?"s in other files.

Try searching for ?:.

Copy link
Member

@nithusha21 nithusha21 left a comment

LGTM for codeowner files.

@nithusha21 nithusha21 removed their assignment Jul 29, 2020
@seanlip
Copy link
Member

seanlip commented Jul 31, 2020

Hi @nishantwrp -- any progress on this? It affects #9919.

@nishantwrp
Copy link
Contributor Author

nishantwrp commented Jul 31, 2020

@seanlip, This may take a day or at max two. I'm working on other places which have question marks. If it's urgent @shavavo can take the changes from this PR (Which removes question marks from customization-args-defs.ts.

@seanlip
Copy link
Member

seanlip commented Jul 31, 2020

Ah no worries. I think it's fine, #9919 might take a bit longer to get merged anyway so 1-2 days should be fine. Thanks @nishantwrp!

@brianrodri brianrodri removed their assignment Aug 6, 2020
Copy link
Member

@DubeySandeep DubeySandeep left a comment

LGTM for the codeowner files, I took a pass on the backend changes and left a few comments PTAL!

core/controllers/creator_dashboard.py Show resolved Hide resolved
@@ -324,6 +324,11 @@ def _round_average_ratings(rating):
self.values.update({
'topic_summary_dicts': topic_summary_dicts
})
else:
self.values.update({
Copy link
Member

@DubeySandeep DubeySandeep Aug 8, 2020

Choose a reason for hiding this comment

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

Add a unit test for this

Copy link
Contributor Author

@nishantwrp nishantwrp Aug 9, 2020

Choose a reason for hiding this comment

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

done

// This property is optional because this object factory is used for
// both 'recent_job_data' and 'unfinished_job_data' in admin page.
// 'recent_job_data' doesn't have this property.
'can_be_canceled'?: boolean;
Copy link
Member

@DubeySandeep DubeySandeep Aug 8, 2020

Choose a reason for hiding this comment

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

[Question unrelated to this PR] I see is_cancelable is that the same field?

Copy link
Contributor Author

@nishantwrp nishantwrp Aug 9, 2020

Choose a reason for hiding this comment

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

Nope!

job['can_be_canceled'] = job['is_cancelable'] and any([
klass.__name__ == job['job_type']
for klass in (
jobs_registry.ONE_OFF_JOB_MANAGERS + (
jobs_registry.AUDIT_JOB_MANAGERS))])

@DubeySandeep DubeySandeep removed their assignment Aug 8, 2020
@nishantwrp nishantwrp assigned seanlip and DubeySandeep and unassigned nishantwrp Aug 9, 2020
showChoicesInShuffledOrder: {
value: boolean;
Copy link
Member

@seanlip seanlip Aug 9, 2020

Choose a reason for hiding this comment

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

Wow good catch @nishantwrp. Why wasn't this detected before?

Copy link
Contributor Author

@nishantwrp nishantwrp Aug 12, 2020

Choose a reason for hiding this comment

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

It is because there is no usage of this interface that imply showChoicesInShuffledOrder is a bool. It was used in one place as

if (showChoicesInShuffledOrder) {
...
}

Now as you can see it's also valid if it were a string.

@@ -35,7 +35,8 @@ interface EnvDict {
}

export class ExpressionError extends Error {
// 'message' is optional beacuse we may not want a custom error message.
// 'message' is optional beacuse it is optional is the actual 'Error'
Copy link
Member

@seanlip seanlip Aug 9, 2020

Choose a reason for hiding this comment

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

This, as written, does not make sense. Please clarify it.

Copy link
Contributor Author

@nishantwrp nishantwrp Aug 12, 2020

Choose a reason for hiding this comment

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

updated! is it fine now?

Copy link
Member

@seanlip seanlip Aug 13, 2020

Choose a reason for hiding this comment

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

Yes, much better -- thanks!


interface CollectionPropertyChange {
interface CollectionCategoryChange {
Copy link
Member

@seanlip seanlip Aug 9, 2020

Choose a reason for hiding this comment

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

The changes here are much better -- and, as you've seen, they've caught some errors that would have gone undetected otherwise. Thanks @nishantwrp!

@@ -326,7 +326,7 @@ def _round_average_ratings(rating):
})
else:
self.values.update({
'topic_summary_dicts': None
'topic_summary_dicts': []
Copy link
Member

@seanlip seanlip Aug 9, 2020

Choose a reason for hiding this comment

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

How could this error be detected, I mean typescript doesn't know what's there in the backend?

Ah I didn't realize you could assign null to any type. I think that explains it (I would have thought something else in the frontend would error).

Also, I'm surprised there isn't a backend test coverage error for these lines. Any idea why?

There was a test for checking the response when the new structures are not enabled.

Could you please update the existing test, or add a new one, so that this error would have been caught? In general, always adopt that mentality when bug-fixing, i.e. try to make it such that the problem can never happen again in the future. Thanks!

Copy link
Contributor Author

@nishantwrp nishantwrp Aug 12, 2020

Choose a reason for hiding this comment

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

Could you please update the existing test, or add a new one, so that this error would have been caught? In general, always adopt that mentality when bug-fixing, i.e. try to make it such that the problem can never happen again in the future. Thanks!

I already did that.

Copy link
Member

@seanlip seanlip Aug 13, 2020

Choose a reason for hiding this comment

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

Oh yep, sorry, missed that. Thanks!

@seanlip seanlip assigned nishantwrp and unassigned seanlip Aug 9, 2020
Copy link
Member

@kevintab95 kevintab95 left a comment

LGTM for codeowner files, thanks @nishantwrp!

@kevintab95 kevintab95 removed their assignment Aug 10, 2020
Copy link
Member

@DubeySandeep DubeySandeep left a comment

LGTM for the codeowner files!

@DubeySandeep DubeySandeep removed their assignment Aug 10, 2020
@nishantwrp nishantwrp assigned seanlip and unassigned nishantwrp Aug 12, 2020
Copy link
Member

@seanlip seanlip left a comment

Thanks @nishantwrp, LGTM!

@seanlip seanlip removed their assignment Aug 13, 2020
Copy link
Contributor

@Showtim3 Showtim3 left a comment

LGTM for codeowners(concept-card-backend-api.service.ts)

@nishantwrp nishantwrp changed the title Fix #10055: Remove question marks in customization-args-defs.ts Fix #10055: Remove unneeded optional properties Aug 13, 2020
Copy link
Contributor

@bansalnitish bansalnitish left a comment

LGTM from codeowner's perspective.

@bansalnitish bansalnitish merged commit e8efb00 into oppia:develop Aug 13, 2020
6 checks passed
@nishantwrp nishantwrp deleted the remove-question-marks branch Aug 13, 2020
@@ -201,78 +201,78 @@ export interface GraphInputCustomizationArgs {


interface ImageClickInputCustomizationArgsBackendDict {
Copy link
Contributor

@shavavo shavavo Aug 13, 2020

Choose a reason for hiding this comment

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

@nishantwrp Hi Nishant, I have a question here.. why is highlightRegionsOnHover here a string for ImageClickInputCustomizationArgsBackendDict, but a boolean for ImageClickInputCustomizationArgs? Ditto for InteractiveMapCustomizationArgsBackendDict (zoom is a string on backend dict, but a number for frontend)

Copy link
Contributor

@shavavo shavavo Aug 13, 2020

Choose a reason for hiding this comment

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

For now, I've changed the backend dicts to match the frontend ones. Let me know if that's a bad change.

Copy link
Contributor Author

@nishantwrp nishantwrp Aug 13, 2020

Choose a reason for hiding this comment

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

yup, it should be boolean in both

Copy link
Contributor

@shavavo shavavo Aug 13, 2020

Choose a reason for hiding this comment

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

Gotchu- thanks for the quick response

shavavo pushed a commit to shavavo/oppia that referenced this pull request Aug 20, 2020
* Remove question marks in customization-args-defs.ts

* Other files

* reviews

* reviews

* Remove new question marks

* reviews

* Reviews
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment