-
Notifications
You must be signed in to change notification settings - Fork 84
MTC mtc-deploy branch with dev #1032
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
base: mtc-deploy
Are you sure you want to change the base?
Conversation
…tion Add custom file and preserve fields transformations
…-time refactor: show csv upload time
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.
Tested and working!
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 had a look at this, then ran it through CodeRabbit and it found a bunch of things that I didn't see! I'm quite impressed with its ability to grok the whole codebase and generate high quality review comments.
|
||
publish (publishProprietaryFiles: boolean) { | ||
const { createFeedVersionFromSnapshot, feedSource } = this.props | ||
this.state.snapshot && createFeedVersionFromSnapshot(feedSource, this.state.snapshot.id, publishProprietaryFiles) |
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.
Are we worried about this failing silently if snapshot is null?
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.
There is another comment in this file explaining how it is triggered by a snapshot class.
return ( | ||
<Modal show={this.state.showModal} onHide={this.close}> | ||
<Body style={{display: 'flex'}}> | ||
<Title>{this.messages('exportPatterns')}</Title> |
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.
Should the title be in the body?
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.
// Check the input to make sure it's within the valid range. | ||
let input = +evt.target.value | ||
if (isNaN(input)) return null | ||
if (input > autoBlockIdIncrement) input = autoBlockIdIncrement |
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.
the starting block ID is limited to be at most the auto increment value, but the starting ID could be whatever the user wants. (from coderabbit)
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.
What is the requested fix here?
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 field is for the starting point for the block ID, right? Then it gets auto incremented by autoBlockIdIncrement
. But this line prevents the starting point from being greater than autoBlockIdIncrement
, which seems wrong. I would think the starting point could be anything you want as long as it's a number and positive, right?
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.
@daniel-heppner-ibigroup Want to open an issue to resolve in the dev branch?
docs/dev/development.md
Outdated
|
||
The e2e tests have been Dockerized, which allows them to be run easily anywhere `docker compose` works. To run them on localhost, first create a `.env` file in the `__tests__/e2e`. `docker compose` will alert you as to which variables must be present. | ||
|
||
To run the tests, run `docker compose -f docker compose.yml up --abort-on-container-exit` in the `__tests__/e2e/` directory. |
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.
extra space between docker
and compose
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 have only fixed the file reference in a2f26cb.
@@ -130,7 +129,7 @@ export default class OrganizationSettings extends Component<Props, State> { | |||
</FormGroup> | |||
<h4>{this.messages('subDetails')}</h4> | |||
<FormGroup> | |||
<ControlLabel style={{marginRight: '5px'}}>Usage tier</ControlLabel> | |||
<ControlLabel style={{marginRight: '5px'}}>this.messages('usageTier.title')}</ControlLabel> |
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.
missing {}
around the message!
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 think the code rabbit got a bit confused :) DT's language model doesn't need brackets
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.
uhhhh are you sure? I think it does. Look just above on line 130, it has {}
(this was a daniel comment not code rabbit)
In fact, on this line there is a closing bracket but no opening bracket.
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.
Oh I see what you mean now. Missing opening bracket!
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 one would have been tricky to find! => c2a7c03
lib/common/components/Login.js
Outdated
* in the `redirectOnSuccess` prop (or to the previously active URL or `/home` if none is specified). | ||
*/ | ||
export default function Login ({ | ||
redirectOnSuccess = window.location.path || '/home' |
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.
should be window.location.pathname
(or just remove that and let the default be /home)
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.
Yeah that window.location was incorrect, but we could remove the component entirely subsequently. (11e1879)
lib/common/components/StatusModal.js
Outdated
title: '' | ||
}) | ||
}, 500) | ||
} | ||
|
||
_handleReLock = () => { | ||
// Close modal before executing actions. | ||
this.close() | ||
// Calling removeEditorLock with null feedId will use current editor |
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.
outdated comment
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.
the feed ID won't be null because there's a null check on the next 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.
Fixed in 4d17f6f
The findings you indicated are probably on the |
Add mastarm eslint
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.
Great work! Thanks for the fixes. I'll leave the last one up to you if you think it's wrong logic.
Holding on merging this PR until client gets to review UI changes. |
Checklist
mtc-deploy
branch at this timeDescription
This PR brings changes from the
dev
branch to themtc-deploy
branch and resolves conflicts between the two branches.Many of the changes are just i18n, improved Flow types, or Bootstrap syntax updates, so when you see them you can just skim them. Weird indents are sometimes brought in from the
dev
branch, I have not really attempted to fix them.A few things that needed to be fixed in this PR for the MTC UI to work:
AppInfoRetriever
component is modified to wrap the app contents, so that nothing is rendered until the app info JSON is downloaded and customizations can be applied (e.g. title, logo, link to alerts module).